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

Serializing BigDecimal values inside containers ignores shape override #2519

Closed
Woodz opened this issue Oct 23, 2019 · 4 comments
Closed

Serializing BigDecimal values inside containers ignores shape override #2519

Woodz opened this issue Oct 23, 2019 · 4 comments
Milestone

Comments

@Woodz
Copy link

Woodz commented Oct 23, 2019

Using the config overrides described in #1911

Reproduction pseudocode:

class Foo {
  public BigDecimal value;
  public List<Object> values:
}

Foo foo = new Foo();
foo.value = new BigDecimal("1.23");
foo.values = ImmutableList.of(new BigDecimal("2.34"));

ObjectMapper mapper = new ObjectMapper();
mapper.configOverrides(BigDecimal.class). .setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.STRING));
mapper.writeValueAsString(foo);

Expected:
Both BigDecimals to be JSON Strings

Actual:
Only value is JSON String

Version: 2.9.6

@cowtowncoder
Copy link
Member

Interesting. Hmmh. Thank you for reporting the problem. This may be tricky to handle due to lack of typing.
One quick question: do you think this still occurs with 2.9.10?

@cowtowncoder
Copy link
Member

Verified, does occur with 2.10 too.
Unfortunately things go pretty deep: contextualization is called appropriately, but unfortunately bean property settings are lazily evaluated and in this case info is for container (List) and not elements -- so resolve information is actually wrong.

Will add a failing test first but then need to think careful of how this could be untangled...

cowtowncoder added a commit that referenced this issue Oct 23, 2019
@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 23, 2019

I think I have an idea of how this problem could be resolved (by not passing property when locating secondary serializers (and maybe deserializers?)), but... it seems like possibly moderate risk of regressions. Chances are fix will need to go in 2.11, not 2.10 patch.

Trying out minimal change only introduces 1 actual regression failure for existing test suite, which is a good sign. But still.

Minimal fix itself is within PropertySerializerMap, findAndAddSecondarySerializer() methods: passing null instead of property, to prevent main property information from being propagated.
This might not be the right place (possibly it should be in method being called), but first need to figure out what is with the new test failure (related to Object Id).

@cowtowncoder
Copy link
Member

Looking deeper, passing null for content serializers will not work as some information is actually used for content elements (like @JsonIgnoreProperties, content-converting serializer and something about Object Ids).

A much simpler short-term fix seems to be to remove lazy-caching of calculated JsonFormat.Value in ConcreteBeanPropertyBase -- it is speculative optimization, too, and although could guard it to verify given baseType matches that seems like a complication to preserve iffy optimization.

But I will probably also want to change how "secondary" serializers are fetched via SerializerProvider, for 2.11, to prepare for likely future work where we may need to start cleansing annotation information passed to secondary value serializers (compared to primary ones).

@cowtowncoder cowtowncoder changed the title Serializing BigDecimal values inside containers ignores shape override Serializing BigDecimal values inside containers ignores shape override Oct 23, 2019
@cowtowncoder cowtowncoder added this to the 2.10.1 milestone Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants