Allow exposing CBOR Simple values as `JsonToken.VALUE_EMBEDDED_OBJECT` with a feature flag

[iifawzi]

Hello, currently, we're decoding Simple values from Major type (7) as VALUE_NUMBER_INT Token, where it's better to be decoded using a special type (I agree and assume VALUE_EMBEDDED_OBJECT would work and no need for additional type as commented)

jackson-dataformats-binary/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

Lines 3679 to 3705 in 63fdc54

	public JsonToken _decodeSimpleValue(int lowBits, int ch) throws IOException {
	if (lowBits > 24) {
	_invalidToken(ch);
	}
	if (lowBits < 24) {
	_numberInt = lowBits;
	} else { // need another byte
	if (_inputPtr >= _inputEnd) {
	loadMoreGuaranteed();
	}
	_numberInt = _inputBuffer[_inputPtr++] & 0xFF;
	// As per CBOR spec, values below 32 not allowed to avoid
	// confusion (as well as guarantee uniqueness of encoding)
	if (_numberInt < 32) {
	throw _constructError("Invalid second byte for simple value: 0x"
	+Integer.toHexString(_numberInt)+" (only values 0x20 - 0xFF allowed)");
	}
	}
	// 25-Nov-2020, tatu: Although ideally we should report these
	// as `JsonToken.VALUE_EMBEDDED_OBJECT`, due to late addition
	// of handling in 2.12, simple value in 2.12 will be reported
	// as simple ints.
	_numTypesValid = NR_INT;
	return (JsonToken.VALUE_NUMBER_INT);
	}

I'm writing this issue so we track it, where we can introduce that change with a feature flag default to false for earlier versions, and true for 3.0 onward.

[iifawzi]

Hey @cowtowncoder, not sure if you would like different behavior on how should we move with handling these values, i'm creating this initial issue to track it and to be able to create a PR against it

[cowtowncoder]

Ok so at high level that sounds good, wrt change to JsonToken.VALUE_EMBEDDED_OBJECT, opt-in for 2.x, default for 3.0.

One immediate idea is that although there is JsonToken.VALUE_EMBEDDED_OBJECT question then would be which Object type to expose it as. I think it'd make sense to specify new simple wrapper type and not use Integer (or Long, if need be).

I don't have strong opinion on naming but looks like term "tag" is used, so maybe SemanticTag.
Doing this would make things bit more type-safe, without adding much overhead.

EDIT: realized these are NOT Tags but specifically "Simple values". So something reflecting that, like SimpleValue, would probably make sense.

[iifawzi]

Ok so at high level that sounds good, wrt change to JsonToken.VALUE_EMBEDDED_OBJECT, opt-in for 2.x, default for 3.0.

One immediate idea is that although there is JsonToken.VALUE_EMBEDDED_OBJECT question then would be which Object type to expose it as. I think it'd make sense to specify new simple wrapper type and not use Integer (or Long, if need be).

I don't have strong opinion on naming but looks like term "tag" is used, so maybe SemanticTag. Doing this would make things bit more type-safe, without adding much overhead.

EDIT: realized these are NOT Tags but specifically "Simple values". So something reflecting that, like SimpleValue, would probably make sense.

That's a good question, actually, I overlooked what value would need to be used. Initially, I thought your idea would mean extending JsonToken enum, but now, after checking the code, I understand that it would be a matter of using it as a tokenType and introducing _simpleValue with all related public methods as we have for _numberInt, etc..

EDIT: The wrapper seems to be already implemented

jackson-dataformats-binary/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORSimpleValue.java

Line 26 in 63fdc54

public class CBORSimpleValue {

will check it while doing the implementation if any changes are needed

[cowtowncoder]

Whoa! Younger me was pro-active, but seems to have forgotten to close the loop here... :)

So yeah, that's exactly what I had in mind now and -- apperently! -- in November 2020 too! :-D

And on "extending JsonToken" yeah no, that would not be possible due to various things. But VALUE_EMBEDDED_OBJECT is designed as simple (-istic) extension point, and works ok here I think.