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

Unannotated single-argument constructor / factory method not considered a creator #50

Closed
odrotbohm opened this issue Jan 30, 2018 · 26 comments

Comments

@odrotbohm
Copy link

odrotbohm commented Jan 30, 2018

Take the following domain class on Java 8 compiled with -parameters:

class SingleValueWrapper {

	private final Integer integer;

	public SingleOtherValue(Integer integer) {
		this.integer = integer;
	}
}

This test:

@Test
public void testname() throws Exception {

	ObjectMapper mapper = new ObjectMapper();
	mapper.registerModule(new ParameterNamesModule());

	SingleValueWrapper value = mapper.readValue("{ \"integer\" : 2 }", SingleValueWrapper.class);

	assertThat(value, is(notNullValue()));
}

fails with:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.example.LombokImmutablesTests$SingleValueWrapper` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{ "integer" : 2 }"; line: 1, column: 3]
	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1342)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1031)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1290)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:326)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:159)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4001)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2992)
	at com.example.LombokImmutablesTests.testname4(LombokImmutablesTests.java:73)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:538)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:760)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:206)

A couple of more observations:

  • Adding an explicit annotation (e.g. @ConstructorProperties) makes the test go green. Unfortunately I don't control the class so that I cannot add any annotations. Also, I thought adding the parameter names module should be sufficient as a name can now be derived from the class and additional annotations shouldn't be needed. I.e. the annotation would just redeclare what's already defined.
  • Adding an additional property makes the test pass. This is probably what strikes me most about the problem as it's complete unintuitive why a constructor wouldn't work for one parameter, but would for more than one.
  • The same applies if you rather expose a factory method than a constructor.
  • Even configuring a Mode explicitly doesn't change anything about the failing test.
  • The execution at some point invokes findNameForDeserialization(…) which ´ParameterNamesAnnotationIntrospector` does not override. Is that by design? Because if I go ahead and implement that method like this:
@Override
public PropertyName findNameForDeserialization(Annotated a) {

	if (a instanceof AnnotatedParameter) {
		return PropertyName.construct(findParameterName((AnnotatedParameter) a));
	}

	return super.findNameForDeserialization(a);
}

things start to work again. I can see that this method is supposed to explicitly obtain a name from an annotation but it feels like that path is favored for a one-argument constructor.

@odrotbohm odrotbohm changed the title Single-argument constructor not considered a creator Un annotated single-argument constructor / factory method not considered a creator Jan 30, 2018
@odrotbohm odrotbohm changed the title Un annotated single-argument constructor / factory method not considered a creator Unannotated single-argument constructor / factory method not considered a creator Jan 30, 2018
odrotbohm added a commit to odrotbohm/sos that referenced this issue Jan 30, 2018
We now configure Lombok to explicitly add @ConstructorProperties annotations to generated constructors as Jackson otherwise doesn't deserialize value objects with single-argument constructors properly.

Related ticket: FasterXML/jackson-modules-java8#50
@cowtowncoder
Copy link
Member

One thing to verify: I assume you are doing this already but... is this against 2.8.11 and/or 2.9.4?

@odrotbohm
Copy link
Author

As described on 2.9.4. On 2.8.11, not even @ConstructorProperties helps. So we're on the right track 🙃.

@cowtowncoder
Copy link
Member

Most likely challenge is that heuristics to determine properties-vs-delegating creator choose differently from your expectations.

One other clarification: does "with -parameters" mean that you explicitly disabled inclusion of parameter names from byte code? If so, I do think this should fail since due to lack of annotations, parameter names, heuristics should assume use of "delegating" mode. And in that case, input of JSON Number only would match: not JSON Object.

So: I am not sure what the flaw here would be?

@odrotbohm
Copy link
Author

No, compiling with -parameter means you compile with parameter name information. That's what puzzles me. The ParameterNamesAnnotationIntrospector kicks in and discovers the name properly but it ends up not getting used as a few steps after the discovery findNameForDeserialization(…) which in the current implementation just returns null as the method is not overridden.

@cowtowncoder
Copy link
Member

Ah. I get confused since some options use plus and minus for on/off. So this makes more sense.

@lpandzic
Copy link
Contributor

AFAIK this is a duplicate of FasterXML/jackson-databind#1498.

@nish-hu
Copy link

nish-hu commented Jun 10, 2018

Hi,

I'm having a similar issue (unannotated single-arg costructor, compiled with -parameters, ParameterNamesModule created with Mode.PROPERTIES)

I beleive this was the commit that broke the expected behavior for me:
c7e62df
(essentially requiring a Mode.DEFAULT annotation to prevent returning null instead of the module default creatorBinding)

As in Oliver's case, the parameter gets discovered properly, but returning a null Mode prevents its usage.

Changing the return value of findCreatorAnnotation (in debug) to the value in creatorBinding (Mode.PROPERTIES in my case) restored the expected behavior.

@claudenobs
Copy link

claudenobs commented Oct 18, 2018

Having the same issue.

I believe i've traced it to :
BasicDeserializerFactory#_addDeserializerConstructors(...) line 430
where if it's a single-arg constuctor without any property information (either explicit or supplied by the ParameterNamesAnnotationIntrospector as suggested by the OP) a DelegatingDeserializer is created instead of a PropertyDeserializer (as expected by people having this issue...)

added a few more checks to the OP's solution to handle this specific case:

@Override
public PropertyName findNameForDeserialization(Annotated a) {
    if (a instanceof AnnotatedParameter) {
        Annotated o = ((AnnotatedParameter) a).getOwner();
        if (o instanceof AnnotatedConstructor) {
            Constructor<?>[] ctors = o.getRawType().getDeclaredConstructors();
            if (ctors.length == 1 && ctors[0].getParameterCount() == 1) {
                return PropertyName.construct(findParameterName((AnnotatedParameter) a));
            }
        }
    }
    return super.findNameForDeserialization(a);
}

@cowtowncoder
Copy link
Member

One thing that may not be mentioned above: findNameForDeserialization() is NOT designed to find implicit property name, but ONLY explicit annotated name. This is by design, to allow overrides.
Method findImplicitPropertyName() is to return introspected name from Java 8+ bytecode, if and when avaialble.

@nish-hu I would need (refinement of) exact reproduction, since it sounds like you are creating module with different parameter(s)?

@bernstein82 make sure to use description of preferred interpretation instead of "illegal" since different users tend to have different expectations.
But as to solution: to ensure proper mode is selected, method:

public JsonCreator.Mode findCreatorAnnotation(MapperConfig<?> config, Annotated a) { }

is the one to override: return Mode.PROPERTIES will prevent use of heuristics.
This is better than forcing implicit name to be returned as explicit, although in the end both may work.

@claudenobs
Copy link

claudenobs commented Oct 22, 2018

@cowtowncoder thanks for clarification on the issue and how to properly handle it (overriding findCreatorAnnotation() was actually what i came up before finding this issue, but after a single debugging session i wasn't exactly sure this was the "correct" solution :-) )

edited my comment above to clarify about DelegatingDeserializer instead of "illegal", by now i understand why it doesn't work as i expected. in FasterXML/jackson-databind#1498 a "fix" was implemented and deferred to release 3.x. So is 3.x imminent?
otherwise it might be prudent to offer a solution for those using the ParameterNamesModule, currently i had to choose from either making a carbon copy or using same package to subclass and override

    public Mode findCreatorAnnotation(MapperConfig<?> config, Annotated a) {  }

it get's even harder if one wants to serialize & deserialize using the implicit property names from the implicit jsonconstuctor. (project with way too many classes where field names don't match constuctor param names... surprisingly gson handled that without any configuration)

@cowtowncoder
Copy link
Member

Ok. After thinking this through, once again, I think I can come up with something that would resolve the issue:

  1. I do not think that the default behavior can be changed, to start detecting 1-arg creators with default settings: this would be problematic for existing users (almost guaranteed to sometimes pick up creator that was not intended), but even more so problem from security perspective (introspection CVEs have been bane of Jackson wrt polymorphic deser, but this could open a new vector).
  2. Use of existing "default mode" setting would be problematic as well, since it has bit different semantics (specify mode in case of base @JsonCreator)
  3. Addition of a new specific configurable feature, on module, would be acceptable, however.

So, for 2.11, I think what I could add:

  • Setting that when enabled WOULD indicate that non-annotated @JsonCreator constructor (but not factory methods) would be indicated as equivalent of @JsonCreator(mode = PROPERTIES)

However, even while writing this I realized that there are couple of aspects:

  • What if there are multiple constructors? They all would be considered creators, leading to failure
  • Should there be a setting to limit visibility? ("only indicate public constructors)

Solving first problem would not be something that AnnotationIntrospector implementation should be concerned about, but at the same time I don't see an easy way to make it handled by jackson-databind either without full rewrite (due in 3.0).
Second problem would ideally also not be up to AI, but I'd be ok with that -- and one could easily pass minimum level needed (there are also value types used with @JsonAutoDetect).

WDYT @odrotbohm @lpandzic @bernstein82 @WellingR

@WellingR
Copy link

WellingR commented Feb 19, 2020

What if there are multiple constructors? They all would be considered creators, leading to failure

I did a quick test to see what the current behaviour is with parameter names module enabled and there are multiple unannotated constructor:
In this case I see the following error:
Cannot construct instance of 'com.mypackage.MyClass' (no Creators, like default construct, exist): cannot deserialize from Object value (no delegate- or property-based Creator)
In other words, jackson refuses to make a choice which makes sense to me. If there are multiple constructors (with parameters) then you must tell jackson which one to use.

Should there be a setting to limit visibility? (only indicate public constructors)

What is the reason why we would want this?
This issue is pretty much a request to ensure that the behaviour of a single-arg constructor is the same as the behaviour of multi-arg constructors.
I guess that limiting the visibility could be a separate feature. It does not seem like a must-have to resolve this issue. (or am I missing something)

Addition of a new specific configurable feature, on module, would be acceptable, however.

Adding a setting could be good, but keep in mind that we already have a setting related to this behaviour: see https://github.com/FasterXML/jackson-modules-java8/tree/master/parameter-names#delegating-creator

@lpandzic
Copy link
Contributor

I agree with @WellingR.
Other than that, the sooner we can get this fixed the better.

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 20, 2020

Obviously I have failed to explain anything here, so I will just make a statement: Single-Argument Constructors CAN NOT act the same as multi-argument ones because of the ambiguity.

The setting in question CAN NOT solve this problem as it does not induce equivalent of @JsonCreator, and futher, SHOULD NOT do so. All it does -- and was added to do -- was to add equivalent of default "mode" property for @JsonCreator.

At this point I will revert my earlier thinking of working on this: without credible path forward there is nothing to do for Jackson 2.x. Specifically:

  1. No change made in this module (only) can change behavior in meaningful way
  2. No simple change to databind can do that either.

@odrotbohm
Copy link
Author

Some feedback regarding the disambiguation algorithm: in Spring Data we use the following rules:

  1. If there's an annotated constructor, use that.
  2. If there's a no arg constructor, use that, even if there are others declared. This one is basically for code that the users don't control, like JDK types that usually declare a no arg constructor
  3. If there are multiple, no-arg constructors: reject and expect disambiguation using an annotation so that we'll ultimately end up with 1.

@cowtowncoder
Copy link
Member

@odrotbohm This is not unlike what Jackson does actually.

One valid case (in principle) that unfortunately can not be supported without major rewrite is that of 1-arg constructor (and no others). Even if that could be detected (databind may be in position to do so), there is no current way to indicate choice between Delegating / Properties-based choice: setting in module is not something databind knows (or can know about) without some additional hook.
Databind itself could have a feature I suppose.

I suppose there is one way I could see potentially working. Hypothetical:

MapperFeature.INTROSPECT_SINGLE_PROPERTY_BASED_CONSTRUCTOR

which would detect un-annotated constructed, if (and only if)

  1. Exactly one Constructor, with public visibility (or whatever @JsonAutoDetect, alternatives, dictate)
  2. Constructor has name OR @JacksonInject for all parameters

Even this might be a reach since introspection logic in databind does separate handling of 0- and 1-arg constructors.

@lpandzic
Copy link
Contributor

So if I understood you correctly, the only way to really fix this is in 3.x.y timeline (FasterXML/jackson-databind#1498)?

@WellingR
Copy link

WellingR commented Feb 21, 2020

FYI, my specific use case is lombok @Value classes. Which are classes with all-arg constructors and all fields final. For the cases where there are multiple fields everything works fine by default. However for the classes with a single field, this behaviour surprises me.

Single-Argument Constructors CAN NOT act the same as multi-argument ones because of the ambiguity.
I understand, which is why a setting has been proposed for this

The setting in question CAN NOT solve this problem as it does not induce equivalent of @JsonCreator, and futher, SHOULD NOT do so
You mean the the setting I mentioned before?
Looking at the code it configures a default for @JsonCreator constructor, I did not completely get from the documentation that this setting applied to JsonCreators only.

However what about a setting HANDLE_SINGLE_ARG_CONSTRUCTOR_AS_PROPERTY_CREATOR? Is there any reason why adding such a setting would not be possible (or would not be a good idea)?

@cowtowncoder
Copy link
Member

@lpandzic Unless a very specific combo-fix (one part in jackson-databind, other [possibly] in java8 module), yes. So challenge would be that of restricting scope to -- I think? -- case of 1-argument, non-annotated constructor (and no other constructors).

Thinking about this aloud, addition of Builder-style construction in 2.10 and later would allow defining a new configuration setting, which would allow Enum for something like CreatorDiscovery or such...and that could change settings to, say:

  1. Use current heuristics for non-mode-defining JsonCreator, introspection [current setting]
  2. No heuristics: only explicit @JsonCreator (but what about mode?)
  3. Heuristics, use Mode.PROPERTIES if not specified (no annotation or no mode) -- most likely option you would prefer?
  4. Heuristics, use Mode.DELEGATING if mode not specified, for 1-arg constructor

and with that actually java8 module would not need anything.
Alternatively, bit more complicated, would be to define something like CreatorConfiguration POJO, with accessors. But that might be more useful for case of trying to select from multiple constructors, something not yet supported.

Now that I wrote this, it actually seems much more doable. WDYT? One thing I lack right now is time to work on things but I sort of like this approach.

@cowtowncoder
Copy link
Member

@WellingR I think you are right that drilling into specific problem case could be the way forward. But perhaps my previous comment's suggestion would work the way you are suggesting (although cover couple of related aspects).

@WellingR
Copy link

Sounds like a good idea to be able to configure the behaviours.

Is there a difference between 1. (current heuristics) and 4. (Heuristics with Mode.DELEGATING if mode not specified)?

@cowtowncoder
Copy link
Member

@WellingR It would have slight difference in the specific case of @JsonCreator annotated (but NO mode property) 1-arg constructor case: instead of trying to determine "property or delegating" based on implicit name and existence of property with same name, it would simply consider it delegating.

Actually I think (3) and (4) would also differ in that auto-discovery of 1-arg constructor would be enabled without annotation, in addition to case of @JsonCreator with no mode. That seems like a reasonable overload although for sake of completeness one could also add 2 more options. But that seems unnecessary.

@WellingR
Copy link

WellingR commented May 7, 2020

@cowtowncoder Any progress on this?

Mode 3 would be perfect for my use-case (lombok value classes). As I would always expect property-based (or actually parameter name based) constructors for those.

@claudenobs
Copy link

i think my usecase for this feature is the same as @WellingR:

  • simple value classes with a single arg constructor

basically my ideal solution would be a configuration option to disable the delegation constructor handling completely (which looks completely obsolete anyway) then it would as simple as looking for a single arg constructor with a matching parameter name. this would even allow handling cases where there are multiple single-arg constructors (don't see a need for that either ;-))

@cowtowncoder
Copy link
Member

@bernstein82 I am not sure why you think delegating constructor is obsolete (although it'd often be used with something like Map or intermediate helper type) -- even with String value it's rather common for scalar types. But I guess I'd agree that expectation of un-annotated delegating constructor may be rare; i.e. that's not the "Lombok use case".

But my working idea right one is to define an enum (to allow extending set of default strategies), backed up by to-be-designed interface for logic to get a set of visible Creator candidates, at least in cases decision is not possible without further info (counter-example being a single applicable annotated Creator method).

@cowtowncoder
Copy link
Member

Now that this issue:

FasterXML/jackson-databind#1498

has been resolved, to be in Jackson 2.12(.0), I'll close this as a duplicate.

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

6 participants