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

@JsonCreator ignored when deserializing map keys #2158

Closed
Trundle opened this issue Oct 17, 2018 · 14 comments
Closed

@JsonCreator ignored when deserializing map keys #2158

Trundle opened this issue Oct 17, 2018 · 14 comments
Labels
duplicate Duplicate of an existing (usually earlier) issue
Milestone

Comments

@Trundle
Copy link

Trundle commented Oct 17, 2018

I'm unsure whether this is a bug or expected behavior, but I found it a bit surprising. Given the following class:

class Key {

    private final String value;

    private Key(final String value) {
        this.value = value;
    }

    @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
    public static Key of(final String value) {
        return new Key(value);
    }
}

Then, when a Key value is deserialized, the factory method annotated with @JsonCreator is called as expected. If Key is used as a key in a Map though, the factory method is not called and Key's private constructor is used directly instead.

Key is of course reduced to demonstrate the issue and hence lacking all properties that would make it actually useful as a key in a map.

I tried the above with version 2.9.7. A repository demonstrating the issue can be found at https://github.com/Trundle/jackson-databind-oddity

@tbartley
Copy link

tbartley commented Nov 6, 2018

I have the same issue. I would like an explicitly annotated @JsonCreator to have precedence over any String constructor.

@cowtowncoder
Copy link
Member

Handling of Map keys is a completely separate code path from handling of regular values, for better or worse (different kinds of serializer, deserializer are used), so that is probably the reason.
I don't remember off-hand whether Creators are supported at all for Map keys yet, but I guess that since private constructor is used there must be some support in there. If so, it might be easy enough to improve handling to use explicitly annotated creator: I agree that this would be the proper handling.

@tbartley
Copy link

tbartley commented Nov 6, 2018

Thanks for the reply.

They are supported, the code to choose a string deserialiser - and I (at least) only care about deserialising from a simple string value - is in StdKeyDeserializers:

    public static KeyDeserializer findStringBasedKeyDeserializer(DeserializationContext ctxt,
            JavaType type)
        throws JsonMappingException
    {
        // We don't need full deserialization information, just need to know creators.
        BeanDescription beanDesc = ctxt.introspect(type);

        // Ok, so: can we find T(String) constructor?
        Constructor<?> ctor = beanDesc.findSingleArgConstructor(String.class);
        if (ctor != null) {
            if (ctxt.canOverrideAccessModifiers()) {
                ClassUtil.checkAndFixAccess(ctor, ctxt.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
            }
            return new StdKeyDeserializer.StringCtorKeyDeserializer(ctor);
        }
        /* or if not, "static T valueOf(String)" (or equivalent marked
         * with @JsonCreator annotation?)
         */
        Method m = beanDesc.findFactoryMethod(String.class);
        if (m != null){
            if (ctxt.canOverrideAccessModifiers()) {
                ClassUtil.checkAndFixAccess(m, ctxt.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
            }
            return new StdKeyDeserializer.StringFactoryKeyDeserializer(m);
        }
        // nope, no such luck...
        return null;
    }

so it first looks for a single arg constructor from String, then looks for a factory method.

I'm trying a change where I just swap the order i.e. look for a factory method before the constructor against the current 3.0.0 snapshot but I'm blocked by an unrelated (at least, I can't imagine how it could be related) unit test failure atm so I've put my PR aside for a little while until I have time to debug that (SqlDateSerializationTest.testSqlDateConfigOverride:106->BaseTest.assertEquals:198 expected:<"1980+04+1[4]"> but was:<"1980+04+1[3]"> FWIW).

Would such an "swap the order" change be considered? Or is the risk of that breaking someone who's (unwittlingly) relying on a String constructor being preferred over a JsonCreator to high? Seems unlikely that would be the case as use of JsonCreator kind of implies coder wants that to be used for deserialisation.

Cheers,

Tim

@tbartley
Copy link

tbartley commented Nov 7, 2018

FYI, my use case is kind of like an extensible enum where I subclass CharSequence to make a kind of type safe String and only permit fixed set of values - same string value should resolve to same object instance so, while I have a constructor from String, it will fail when called a second time with the same value. So I have a valueOf creator that finds the existing instance if there is one and if not, only then constructs a new instance.

tbartley added a commit to anonyome/jackson-databind that referenced this issue Nov 7, 2018
Fixes FasterXML#2158

Simply swaps the order factory method and String
constructors are searched for in the key type.
Factory methods are now preferred.
tbartley added a commit to anonyome/jackson-databind that referenced this issue Jul 5, 2019
Fixes FasterXML#2158

Simply swaps the order factory method and String
constructors are searched for in the key type.
Factory methods are now preferred.
@cowtowncoder cowtowncoder added 2.10 and removed 2.9 labels Sep 12, 2019
@cowtowncoder cowtowncoder added 2.12 and removed 2.10 labels Apr 12, 2020
@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Oct 27, 2020
@zxdposter
Copy link

zxdposter commented Feb 28, 2021

I have same issues

@Stephan202
Copy link
Contributor

We hit this issue after enabling Immutables' interning functionality on some DTOs: lookups failed for deserialized maps with an interned key type.

One workaround that appears to work is to declare a custom KeyDeserializer with some code copied from StdKeyDeserializers#findStringBasedKeyDeserializer and StdKeyDeserializer, effectively skipping the constructor lookup. Example:

final class FactoryMethodKeyDeserializerModule extends SimpleModule {
    @Override
    public void setupModule(SetupContext context) {
        context.addKeyDeserializers(
                (type, config, beanDesc) -> {
                    Method factoryMethod = beanDesc.findFactoryMethod(String.class);
                    if (factoryMethod == null) {
                        return null;
                    }

                    if (config.canOverrideAccessModifiers()) {
                        ClassUtil.checkAndFixAccess(
                                factoryMethod,
                                config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
                    }

                    return new StdKeyDeserializer(-1, factoryMethod.getDeclaringClass()) {
                        @Override
                        public Object _parse(String key, DeserializationContext ctxt)
                                throws IllegalAccessException, InvocationTargetException {
                            // NB: `try/catch` can be dropped if FasterXML/jackson-databind#3109 is released.
                            try {
                                return factoryMethod.invoke(null, key);
                            } catch (InvocationTargetException e) {
                                if (e.getCause() != null) {
                                    Throwables.throwIfUnchecked(e.getCause());
                                }

                                throw e;
                            }
                        }
                    };
                });
    }
}

@cowtowncoder
Copy link
Member

@Stephan202 thank you for sharing this!

@cowtowncoder
Copy link
Member

Back to the very beginning, I will note that as of Jackson 2.12, there is introspection for 2 String-based Creators, neither of which actually considers @JsonCreator. Based on this, I would have thought original case simply worked:

  1. Constructor that takes single String argument
  2. Static factory method that takes single String argument

However, javadocs also suggest that for (2), name must be valueOf(). Code goes through so many paths that I got lost and while I could not find this limitation, I think I'd really need to write some unit tests to verify this.

I do think that @JsonCreator on 1-arg constructor or 1-arg factory method, taking CharacterSequence/String should work; this issue can remain for verifying it does and/or work to make it work.

@Stephan202
Copy link
Contributor

Tnx! I happened to have sent you an email just over an hour ago with an attachment that contains some test cases that can be used for this purpose :)

@cowtowncoder
Copy link
Member

@Stephan202 that definitely would! We can add them under src/test/java/..../failing to run manually & moved to other places after fixing.

@Stephan202
Copy link
Contributor

Ack! I can modify the code I sent by mail and open a PR. Won't have time for that right now; but can do tonight/tomorrow. 👍

@cowtowncoder
Copy link
Member

@Stephan202 that'll be awesome, whenever -- I probably won't have time to look into this particular issue immediately anyway. But will be good to have reproduction.

@Stephan202
Copy link
Contributor

Alright, I filed #3111.

@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Jun 25, 2021
@cowtowncoder
Copy link
Member

I think this was solved along with #3143 -- test now passes. Fix will be included in 2.13.0 (since it required bigger refactoring of things unfortunately, cannot safely backport).

@cowtowncoder cowtowncoder added the duplicate Duplicate of an existing (usually earlier) issue label Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Duplicate of an existing (usually earlier) issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants