Add failing test for #4742 #4757
Conversation
| @@ -645,7 +645,7 @@ public void resolve(DeserializationContext ctxt) throws JsonMappingException | |||
| } | |||
|
|
|||
| // And now that we know CreatorProperty instances are also resolved can finally create the creator: | |||
| if (creatorProps != null) { | |||
| if (creatorProps != null && creatorProps.length > 0) { | |||
There was a problem hiding this comment.
So the idea is... it should not have create() method as PropertyBasedCreator, but instead make it default creator for Builder case.
There was a problem hiding this comment.
I do not follow the logic here. Why should we skip the case of 0-arguments Creator? (this would only apply to factory method, I think, with constructor we get separate "default constructor" case). It also does not seem to apply just to the Builder case (no checks to indicate that).
| if (_defaultCreator == null) { | ||
| _defaultCreator = withArgsCreator; | ||
| } |
There was a problem hiding this comment.
Same idea here. create() method should become the defaultCreator for Builder case. Idk how zero-arg create() method became withArgsCreator probably null-checking but not checking length?
There was a problem hiding this comment.
I also do not quite understand intended logic here: _defaultCtor is specifically expected to be 0-args Constructor; here no checking is done.
If such logic for defaulting was needed, it should be probably done by caller, not this method.
|
Ok I must admit I do not understand the idea. There are also a few unit test failures... |
|
It seems like this PR is quite explained incorrectly. I will convert this PR just adding test case. Your comments seem to already sort of reflect what I was trying to say 😅 like Builder's factory method should be default creator or skipping creating properties creator. PS: Changed to just adding a test case. |
This reverts commit 9c6c535.
JooHyukKim
changed the title
| @@ -0,0 +1,130 @@ | |||
| package com.fasterxml.jackson.databind.deser.creators; | |||
There was a problem hiding this comment.
I think this should go under tofix, right?
Like I mentioned in #4742, not for merge, just to get idea.Added failing test instead