Integer Validator improvement to support OpenAPI & AsyncAPI specs

[ankitk-me]

Description

• integer validation (text & binary)
  • int32, int64
  • max and min value
  • multiple of N

[jfallows]

When we use enum as a strategy object, the implementation needs to be stateless because the same instance of the enum constant is reused by all callers, so any state stored on the instance of the enum constant is also shared by all callers, causing a collision either across threads or due to overlapping fragmented validate calls even on the same thread.

Suggest moving the validation to a different class that can have state, and keeping just the remaining necessary behavioral parts here in the enum that can perform the checks needed by the stateful validator.

[jfallows]

Do these need to be explicitly specified, or are they the defaults?
If they are the defaults then they should be omitted, agree?

[jfallows]

Perhaps simpler to either assign defaults to fields initially (i think we do this elsewhere), or use boxed Integer and Boolean types for fields (but still pass primitives via methods) so that you can tell the difference between unassigned (null, method never called) vs assigned explicitly to a value matching the default value.

[jfallows]

This -1 return value seems like a magic value - should be defined as a constant on ConverterHandler instead perhaps.

[jfallows]

Instead of storing all these fields separately, we can simplify to a single IntPredicate called check that verifies the conditions, so later instead of calling

conditions(value, max, min, exclusiveMax, exclusiveMin, multiple);

we can instead call

check.test(value)

so something like this in the constructor to assign check field

int max = config.max;
int min = config.min;
int multiple = config.multiple;
IntPredicate checkMax = config.exclusiveMax ? v -> v < max : v <= max;
IntPredicate checkMin = config.exclusiveMin ? v -> v > min : v >= min;
IntPredicate checkMultiple = v -> v % multiple == 0;
this.check = checkMax.and(checkMin).and(checkMultiple);

[jfallows]

If we use MutableInteger, then enum can modify state without inadvertently sharing state across validations or workers.

Using decoded field to store incrementally decoded result across multiple calls to format.decode(...).
Using processed field to store total processed bytes across multiple calls to format.decode(...).

Suggested change

	@Override
	public boolean validate(
	int flags,
	DirectBuffer data,
	int index,
	int length,
	ValueConsumer next)
	{
	int position = 0;
	boolean valid = true;
	if ((flags & FLAGS_INIT) != 0x00)
	{
	value = 0;
	progress = 0;
	sign = false;
	number = false;
	byte digit = data.getByte(index);
	if (format.negative(digit))
	{
	position += BitUtil.SIZE_OF_BYTE;
	sign = true;
	}
	}
	for (int numByteIndex = index + position; numByteIndex < index + length; numByteIndex++)
	{
	byte digit = data.getByte(numByteIndex);
	if (format.digit(digit))
	{
	value = format.decode(value, digit);
	number = true;
	}
	else
	{
	valid = false;
	break;
	}
	progress += BitUtil.SIZE_OF_BYTE;
	}
	if (valid)
	{
	if ((flags & FLAGS_FIN) != 0x00 && format.validateFin(progress, number))
	{
	if (sign)
	{
	value *= -1;
	}
	valid = conditions(value, max, min, exclusiveMax, exclusiveMin, multiple);
	}
	else
	{
	valid = format.validateContinue(progress);
	}
	}
	return valid;
	}
	@Override
	public boolean validate(
	int flags,
	DirectBuffer data,
	int index,
	int length,
	ValueConsumer next)
	{
	if ((flags & FLAGS_INIT) != 0x00)
	{
	decoded.value = 0;
	processed.value = 0;
	}
	int progress = format.decode(decoded, processed, data, index, length);
	boolean valid = progress != IntegerFormat.INVALID_INDEX;
	if ((flags & FLAGS_FIN) != 0x00)
	{
	valid &= format.valid(decoded, processed);
	valid &= check.test(value);
	}
	return valid;
	}

where the enum becomes something like...

public enum IntegerFormat
{
    public static final int INVALID_INDEX = -1;

    TEXT
    {
        @Override
        public int decode(
            MutableInteger decoded,
            MutableInteger processed,
            DirectBuffer data,
            int index,
            int length)
        {
            int progress = index;
            int limit = index + length;

            decode:
            for (; progress < limit; progress++)
            {
                int digit = data.getByte(progress);

                if (digit < '0' || '9' < digit)
                {
                    if (processed.value == 0))
                    {
                        switch (digit)
                        {
                        case '-':
                            decoded.value = Integer.MIN_VALUE;
                            processed.value++;
                            continue decode;
                        case '+':
                            decoded.value = Integer.MAX_VALUE;
                            processed.value++;
                            continue decode;
                        default:
                            break;
                        }
                    }

                    progress = INVALID_INDEX;
                    break decode;
                }

                int multipler = 10;

                if (processed.value == 1))
                {
                    switch (decoded.value)
                    {
                    case Integer.MIN_VALUE:
                        decoded.value = -1;
                        multipler = 1;
                        break;
                    case Integer.MAX_VALUE:
                        decoded.value = 1;
                        multipler = 1;
                        break;
                    default:
                        break;
                    }
                }

                decoded.value = decoded.value * multipler + (digit - '0');
                processed.value++;
            }
        }

        @Override
        public boolean valid(
            MutableInteger decoded,
            MutableInteger processed)
        {
            return processed.value > 1 || decoded.value != Integer.MIN_VALUE || decoded.value != Integer.MAX_VALUE;
        }
    },

    BINARY
    {
        private static final int INT32_SIZE = 4;

        @Override
        public int decode(
            MutableInteger decoded,
            MutableInteger processed,
            DirectBuffer data,
            int index,
            int length)
        {
            int progress = index;
            int limit = index + length;

            decode:
            for (; progress < limit; progress++)
            {
                int digit = data.getByte(progress);

                if (processed.value >= INT32_SIZE)
                {
                    progress = INVALID_INDEX;
                    break decode;
                }

                decoded.value <<= 8;
                decoded.value |= digit & 0xFF;
                processed.value++;
            }

            return progress;
        }

        @Override
        public boolean valid(
            MutableInteger decoded,
            MutableInteger processed)
        {
            return processed.value == INT32_SIZE;
        }
    }

[jfallows]

These be removed now, right?

[jfallows]

These can be local variables, agree?

[jfallows]

This should be an exception instead of an assert, we need to know for sure things are working or not at runtime.

[jfallows]

Can we check the value type instead of instanceof?

[jfallows]

Suggest we add a separate Int32State class and use instead of these two mutable integers.

public final class Int32State
{
    public int decoded;
    public int processed;
}

then pass an instance here as a parameter called state, and refer to the fields as needed below.

[jfallows]

Suggested change

	if (processed.value == 0)
	if (state.processed == 0)

[jfallows]

Suggested change

	decoded.value = Integer.MIN_VALUE;
	processed.value++;
	state.decoded = Integer.MIN_VALUE;
	state.processed++;

[jfallows]

Suggested change

	decoded.value = Integer.MAX_VALUE;
	processed.value++;
	state.decoded = Integer.MAX_VALUE;
	state.processed++;

[jfallows]

Suggested change

	if (processed.value == 1)
	if (state.processed == 1)

[jfallows]

Suggested change

	switch (decoded.value)
	{
	case Integer.MIN_VALUE:
	decoded.value = -1 * (digit - '0');
	break;
	case Integer.MAX_VALUE:
	decoded.value = digit - '0';
	break;
	default:
	decoded.value = decoded.value * multipler + (digit - '0');
	break;
	}
	switch (state.decoded)
	{
	case Integer.MIN_VALUE:
	state.decoded = -1 * (digit - '0');
	break;
	case Integer.MAX_VALUE:
	state.decoded = digit - '0';
	break;
	default:
	state.decoded = state.decoded * multipler + (digit - '0');
	break;
	}

[jfallows]

Suggested change

	decoded.value = decoded.value < 0
	? decoded.value * multipler - (digit - '0')
	: decoded.value * multipler + (digit - '0');
	state.decoded = state.decoded < 0
	? state.decoded * multipler - (digit - '0')
	: state.decoded * multipler + (digit - '0');

[jfallows]

Suggested change

	processed.value++;
	state.processed++;

[jfallows]

Suggested change

	public boolean valid(
	MutableInteger decoded,
	MutableInteger processed)
	{
	return processed.value > 1 || decoded.value != Integer.MIN_VALUE || decoded.value != Integer.MAX_VALUE;
	}
	public boolean valid(
	Int32State state)
	{
	return state.processed > 1 || state.decoded != Integer.MIN_VALUE || state.decoded != Integer.MAX_VALUE;
	}