Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map deserialization results in different numeric classes based on json ordering (BigDecimal / Double) when used in combination with @JsonSubTypes #3133

Closed
mreiterer opened this issue Apr 28, 2021 · 15 comments
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@mreiterer
Copy link

mreiterer commented Apr 28, 2021

Describe the bug
When deserializing a jsonSubTye which contains a map with numeric values (floats) the order of the json string results in different classes constructed.

I have pasted a complete Junit5 Test which can easily be used to reproduce the issue.

Version information
Happens from 2.12.0 onwards, in 2.11.x its ok.

To Reproduce
If you have a way to reproduce this with:

package test;

import java.util.HashMap;
import java.util.Map;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestInstance.Lifecycle;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonSubTypes.Type;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;

@TestInstance(Lifecycle.PER_CLASS)
public class JacksonSubTypeBehaviourTest {

	private static final Logger log = LoggerFactory.getLogger(JacksonSubTypeBehaviourTest.class);

	private ObjectMapper mapper;

	@BeforeAll
	public void before() {
		mapper = new ObjectMapper();
		mapper.disable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS);
	}

	@Test 
	public void testDeserializeWithDifferentOrdering() throws Exception {
		
		String ordering1 = "{\n"
				+ "			\"type\": \"MAP\",\n"
				+ "			\"map\": {\n"
				+ "				\"doubleValue\": 0.1\n"
				+ "			}\n"
				+ "}";
				
		TestMapContainer model1 = mapper.readValue(ordering1, TestMapContainer.class);
		log.info("clazz: {}",  model1.getMap().get("doubleValue").getClass());
		Assertions.assertTrue(model1.getMap().get("doubleValue") instanceof Double);
		
		
		String ordering2 = "{\n"
				+ "	\"map\": {\n"
				+ "		\"doubleValue\": 0.1\n"
				+ "	},\n"
				+ "	\"type\": \"MAP\"\n"
				+ "	\n"
				+ "}";
		
		TestMapContainer model2 = mapper.readValue(ordering2, TestMapContainer.class);
		
		log.info("clazz: {}",  model2.getMap().get("doubleValue").getClass());
		Assertions.assertTrue(model2.getMap().get("doubleValue") instanceof Double);
		
	}
	
	@JsonTypeInfo(  
		    use = JsonTypeInfo.Id.NAME,  
		    include = JsonTypeInfo.As.PROPERTY,  
		    property = "type")

	@JsonSubTypes({  
	    @Type(value = TestMapContainer.class, name = "MAP"),
	     })  	
	private static interface TestJsonTypeInfoInterface {
		
	}
	
	private static class TestMapContainer implements TestJsonTypeInfoInterface {
		
		private Map<String, ? extends Object> map = new HashMap<>();

		public Map<String, ? extends Object> getMap() {
			return map;
		}

		public void setMap(Map<String, ? extends Object> map) {
			this.map = map;
		}
		
	}
}

Expected behavior
Numeric values (point numbers) should always be created as Double unless stated otherwise with DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS

@mreiterer mreiterer added the to-evaluate Issue that has been received but not yet evaluated label Apr 28, 2021
@cowtowncoder cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 28, 2021
@cowtowncoder
Copy link
Member

Interesting. Due to ordering difference I suspect this is due to one ordering requiring buffering to handle polymorphism (that is, type id must be available before POJO values can be instantiated); and then there is the challenge of TokenBuffer retaining equivalent information both about configuration (wrt coercion) and source information (since values are not JSON text but must be converted into Java values).

I agree that behavior should be identical; above is just talking through likely root cause that changes behavior between the cases.

Thank you for including all the information to reproduce the issue!

@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Jun 1, 2021
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@mreiterer
Copy link
Author

If you can give me a hint / entry point in the codebase i can try to fix / improve the logic and send a merge request.

@cowtowncoder
Copy link
Member

@mreiterer I wish there was an easy answer to that question, but handling of buffering is not exactly simple. Relevant code is in TokenBuffer, but it all depends on deserializer that handles buffering, method(s) called on JsonParser to access numeric value and so on. This may get bit more complicated as buffering is not necessarily done by value deserializer in this case, given polymorphic handling which then requires delegation to TypeDeserializer.
It may be possible to use debugger to try to find code paths relevant; ultimately deserializer for expected polymorphic type does get called.

@mreiterer
Copy link
Author

mreiterer commented Oct 8, 2021

I did some debugging here and this seems to be related to #2644
There is a behaviour change from 2.11.4 to 2.12.x related to getNumberValueExact.

When i look at the "_tokens" var in the segment of the TokenBuffer the value is already of type BigDecimal before the DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS is checked. The if in Vanilla.deserialize decides if getDecimalValue or getNumberValueis used, but this does not have any effect as the cast is already done. Mostly notes to myself for further investigation.

The difference about the ordering seems to be located here: AsPropertyTypeDeserializer.deserializeTypedFromObject.

If the typePropertyName is found first in the json structure:
a) then the deserializer is called directly converting the "0.1" to Double.
b) otheriwise the tokenBuffer is copied via copyCurrentStructure -> and in _copyBufferValue getNumberValueExact is called for VALUE_NUMBER_FLOAT.

@cowtowncoder
Copy link
Member

Correct: buffering is only used if absolutely required -- or put another way, if ordering happens to be optimal, buffering may be avoided, on assumption this is relatively common.

@mreiterer
Copy link
Author

mreiterer commented Oct 8, 2021

@cowtowncoder Why is getNumberValueExact called in case of buffer copy, and not in the more common case (UntypedObjectDeserializer line 743 - jackson-databind 2.12.5)

@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 9, 2021

The difference is meant to defer possible truncation/coercion in case of buffering (since we have no idea what the target type is); whereas when using UntypedObjectDeserializer -- which has target type of java.lang.Object -- we know what target type we should be using, based on configuration.

This matter more for binary formats where there is specific accurate representation, but is more problematic for textual formats that do not have distinction between various FP representations. And then there is the difference between streaming-level and databind-level configuration to consider too.

Ideally, I think, the whole decoding should be deferred in buffering cases: parser would simply validate that the textual representation is valid JSON (or xml, yaml, csv etc) Number, but would not try to get Java Number out of it until explicitly requested.

@mreiterer
Copy link
Author

Are there any plans to fix this issue in 2.14.x ?

@cowtowncoder
Copy link
Member

@mreiterer I don't think so due to timing constraints.

@ellebrecht
Copy link

Any news on this? Is there any workaround we can use in the meantime?

@JooHyukKim
Copy link
Member

JooHyukKim commented Nov 28, 2023

@ellebrecht which Jackson version are you using? Seems like this issue has been fixed in later versions, at least in 2.15. Check out #4231.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed 2.14 labels Nov 28, 2023
@cowtowncoder
Copy link
Member

@ellebrecht As per @JooHyukKim 's comment if you have to see whether 2.16.0 still has the issue, that'd be helpful. And if anyone else has time to look into this that'd help.

But as a general rule: if there is specific work towards fix for an issue, there will be comments here on this issue.
We are usually pretty good wrt such updates.

@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Nov 28, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 28, 2023

As per #4231 has been fixed for at least 2.15.0, but will include as fixed in 2.15.4 and 2.16.0 release notes (no need to backdate older releases)

@ellebrecht
Copy link

@ellebrecht which Jackson version are you using? Seems like this issue has been fixed in later versions, at least in 2.15. Check out #4231.

We are using 2.15.3, the version pulled in by the current Spring Boot version 3.2.0.

@pjfanning
Copy link
Member

@ellebrecht please open a new issue - it is not a good idea to continue the conversation on on a closed issue.

In the new issue, could you provide a new reproduction case? It appears #3133 works but you appear to have a case that is different from #3133. Please do not include any need to test with external dependencies like spring-boot. The new case should use plain Jackson code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

5 participants