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

configOverrides(boolean.class) silently ignored, wheres .configOverride(Boolean.class) works for both primitives and boxed boolean values #3080

Closed
asaf-romano opened this issue Mar 16, 2021 · 6 comments
Milestone

Comments

@asaf-romano
Copy link

Using version 2.12:

mapper.configOverride(Boolean.class).setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.NUMBER));

This works as expected. Boxed Boolean values are serialized as 1/0. Moreover, it also works for primitive booleans.

However,

mapper.configOverride(boolean.class).setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.NUMBER));

seems to do nothing, for either types.

I find this behavior quite surprising. I think it's reasonable to assume any of the following:

  1. boolean implies Boolean symmetrically
  2. boolean implies Boolean, but not the other way around (mirroring the current behavior)
  3. (1) + ability to override the format for the boxed type (to be clear, I've no use case for that)
  4. An error to be thrown by configOverride.
@asaf-romano asaf-romano added the to-evaluate Issue that has been received but not yet evaluated label Mar 16, 2021
@cowtowncoder
Copy link
Member

Quick check: the usual way to refer to primitive boolean is Boolean.TYPE -- is it possible that boolean.class might refer to wrapper type (Boolean.class)?

If not, this does sound like a bug, and if so possibly simple one.

@cowtowncoder cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 16, 2021
@cowtowncoder
Copy link
Member

Ok, yes, I can reproduce this.

Before looking at the fix, this does cause a problem since the way this should work (as per design) is as per (3): Jackson does (or at least attempts :) to) distinguish between primitive and wrapper types; and so current behavior is incorrect. Problem here is that changing this now is API changing as the problem was not caught before 2.12.0 release.
As a result, I don't think I can change this in 2.12.x, as patches should not cause behavior change (although obv there is grey area wrt bug fixes).
I think I will do this for 2.13.0, however, since behavior is unintended and other configuration settings (for numbers, f.ex) do take into account primitive/wrapper distinction.

In theory I could try to specify defaulting of some sort (so that setting for Boolean would be default for boolean, but could further be overridden for boolean), but while that could be more compatible it would be different from handling of all types -- there should ideally be a general logic to primitive<-wrapper defaulting for all options. This does not exist, nor do I want to write one.

So. I think I'll work towards fixing this for 2.13.0.

@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Mar 16, 2021
@cowtowncoder
Copy link
Member

The bug itself is simple; in BooleanSerializer.createContextual(), we have:

            JsonFormat.Value format = findFormatOverrides(serializers,
                    property, Boolean.class);

regardless of whether this is serializer for primitive or wrapper. Easy to fix in and of itself, but think I'll check to see how number serializers work. And perhaps even see how tricky defaulting might be; if simple enough, could add it only in here and remove from 3.0 (master).

@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Mar 16, 2021
@cowtowncoder
Copy link
Member

Fixed for 2.13.0: I would recommend setting shape for both wrapper and primitive, even with 2.12, for future-proofing.

@asaf-romano
Copy link
Author

wow, that was fast! Many thanks!

@cowtowncoder
Copy link
Member

@asaf-romano sometimes this happens, but it is not necessarily representative of the typical bug report experience :) (obv. disclaimer).

That said, thank you for reporting this!

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