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

String-based Map key deserializer is not deterministic when there is no single arg constructor #3143

Closed
hisener opened this issue May 5, 2021 · 4 comments
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@hisener
Copy link

hisener commented May 5, 2021

Describe the bug

StdKeyDeserializers#findStringBasedKeyDeserializer uses a static factory method when there is no single arg constructor. It gets the factory method using BasicBeanDescription#findFactoryMethod that returns the first single-arg static method. However, the behavior is not deterministic as java.lang.Class#getDeclaredMethods Javadoc states:

The elements in the returned array are not sorted and are not in any particular order.

The factory methods are collected using AnnotatedCreatorCollector#_findPotentialFactories -> ClassUtil#getClassMethods -> java.lang.Class#getDeclaredMethods.

So in the case of two static factory methods, one with @JsonCreator annotation and one named #valueOf or #fromString, Jackson may use either of them depending on the array order.

The second method needs to be named #valueOf or #fromString because of the following BasicBeanDescription#isFactoryMethod logic:

// 24-Oct-2016, tatu: As per [databind#1429] must ensure takes exactly one arg
if ("valueOf".equals(name)) {
if (am.getParameterCount() == 1) {
return true;
}
}
// [databind#208] Also accept "fromString()", if takes String or CharSequence
if ("fromString".equals(name)) {
if (am.getParameterCount() == 1) {
Class<?> cls = am.getRawParameterType(0);
if (cls == String.class || CharSequence.class.isAssignableFrom(cls)) {
return true;
}
}
}

Version information

2.12.2

To Reproduce

static class KeyTypeMultipleFactoryMethods {
    protected String value;

    private KeyTypeMultipleFactoryMethods(String v, boolean bogus) {
        value = v;
    }

    @JsonCreator
    public static KeyTypeMultipleFactoryMethods create(String v) {
        return new KeyTypeMultipleFactoryMethods(v, true);
    }

    public static KeyTypeMultipleFactoryMethods valueOf(String id) {
        return new KeyTypeMultipleFactoryMethods(id.toUpperCase(Locale.ROOT), false);
    }
}

public void testKeyWithCreatorAndMultipleFactoryMethods() throws Exception
{
    Map<KeyTypeMultipleFactoryMethods,Integer> map = MAPPER.readValue("{\"foo\":3}",
            new TypeReference<Map<KeyTypeMultipleFactoryMethods,Integer>>() {} );
    assertEquals(1, map.size());
    assertEquals("foo", map.keySet().iterator().next().value);
}
[ERROR] Failures: 
[ERROR]   MapDeserializationTest.testKeyWithCreatorAndMultipleFactoryMethods:486 expected:<[foo]> but was:<[FOO]>
[INFO] 
[ERROR] Tests run: 26, Failures: 1, Errors: 0, Skipped: 0

It's also available in this branch: 2.12...PicnicSupermarket:hsener/find-factory-method.

Expected behavior

@JsonCreator method has precedence over other static factory methods in String-based deserialization.

Additional context

We noticed this behavior in a custom key deserializer, which uses BasicBeanDescription#findFactoryMethod. The behavior was nondeterministic for an enum where we have Enum#valueOf and a static factory method with @JsonCreator annotation.

@hisener hisener added the to-evaluate Issue that has been received but not yet evaluated label May 5, 2021
@cowtowncoder
Copy link
Member

That does sound sub-optimal and should either throw an exception for ambiguous cases, or at least use stable tie-breaker -- possible latter for 2.12 (to prevent perceived regression for some usage), and exception for 2.13.

@cowtowncoder cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels May 7, 2021
@hisener
Copy link
Author

hisener commented May 7, 2021

Would that mean #findFactoryMethod in 2.13 will always throw an exception for enums with a @JsonCreator annotation? 🤔

@cowtowncoder
Copy link
Member

@hisener I'll have look into details but I think that rules should be similar to value deserializers:

  1. Annotated constructor/factory method overrides any implicit ones
  2. Only if there are multiple @JsonCreator annotated alternatives without precedence (I think there is a tie-breaker for factory vs constructor but I can't remember which is expected to win) -- that is, if there 2 annotated constructors, or 2 factory methods

For enums there is also @JsonValue to consider: there should be no exception, but I'd have to think of precedence here (since this is specific to Enums; for POJOs @JsonValue only affects serialization).

@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels May 27, 2021
cowtowncoder added a commit that referenced this issue Jun 11, 2021
@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Jun 11, 2021
@cowtowncoder
Copy link
Member

Added the failing test from above and can reproduce the issue. Method that selects Creator method is StdKeyDeserializers.findStringBasedKeyDeserializer() and needs to be changed to consider annotation(s) for precedence.

@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Jun 15, 2021
@cowtowncoder cowtowncoder changed the title String-based key deserializer is not deterministic when there is no single arg constructor String-based Map key deserializer is not deterministic when there is no single arg constructor Jun 15, 2021
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

2 participants