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

deserialized JsonAnySetter field is null #4508

Closed
1 task done
MaximValeev opened this issue Apr 30, 2024 · 22 comments
Closed
1 task done

deserialized JsonAnySetter field is null #4508

MaximValeev opened this issue Apr 30, 2024 · 22 comments
Labels
Milestone

Comments

@MaximValeev
Copy link

MaximValeev commented Apr 30, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

I have kotlin data class

data class Request(
    @JsonProperty("header")
    override val header: String? = null,
    @JsonIgnore
    @JsonAnySetter
    @JsonAnyGetter
    val additionProperties: Map<String, Any?>? = null,
) : Parent(header)

and json

      {
            "payloadType": "perform",
            "data":{
              "header": "lol",
              "type": "sometype",
              "any_other_data": {
                  "field1": "1",
                  "field2": "",
                  "field3": "",
                  "field4": {
                      "field5": "",
                      "field6": "",
                      "field7": "anytext"
                  }
              }
            }
        }

Before jackson 2.17.0 the json deserialized into class correctly. any_other_data put to additionalProperties map.
On 2.17.0 version i have additionalProperties == null after the deserialization.

Version Information

2.17.0

Expected behavior

i expected that field annotated with @JsonAnySetter will be filled with all the undeclared fields

@MaximValeev MaximValeev added the to-evaluate Issue that has been received but not yet evaluated label Apr 30, 2024
@JooHyukKim
Copy link
Member

JooHyukKim commented May 1, 2024

Since additionalProperties has default value of null, we can say either...

  • field is deserialized to null
  • field is not deserialized, so deafult to null

Could you provide Java-only reproduction to make sure it is jackson-databind issue, @MaximValeev ?
There is separate kotlin repo FYI.

@cowtowncoder
Copy link
Member

Right; either this needs to move to jackson-module-kotlin (I can transfer if so), or reproduction should be in Java.

But I think there is probably already an issue wrt @JsonAnySetter not working with Creator properties -- and even PR to potentially fix that.

@kotcrab
Copy link

kotcrab commented May 13, 2024

I also ran into this issue, it can be fixed by writing @field:JsonAnySetter. In 2.17.0 ElementType.PARAMETER was added to the @Target list of JsonAnySetter, this causes Kotlin to add this annotation to the constructor parameter instead of the field as described here if you don't specify explicit use-site target.

@JooHyukKim
Copy link
Member

Ah, thank u for sharing @kotcrab !

@JooHyukKim
Copy link
Member

Some background.

As @kotcrab described above, this is regression caused by annotations module change, the annotation change was meant to be pre-requisite for the databind-module's Allow 'JsonAnySetter' to flow through JsonCreator #562 issue.

I started writing PR #4366 to support the databind-module change, but stopped to allow room for Property Introspection Rewrite in #4515.

As per solution, either....

  • revert adding ElementType.PARAMETER to JsonAnySetter or...
  • ask @k163377 if it's possible to make changes so that Kotlin module can discover @JsonAnySetter without @field: declaration. Or maybe @cowtowncoder knows the proper extension point for JsonAnySetter discovery?

@cowtowncoder
Copy link
Member

Wait, what is that @JsonIgnore doing here??!

    @JsonIgnore
    @JsonAnySetter
    @JsonAnyGetter
    val additionProperties: Map<String, Any?>? = null,

wouldn't that mean "ignore this any setter/getter"? That should not be added there.

@MaximValeev
Copy link
Author

@cowtowncoder No, it wouldn’t. It means ignoring additionalProperties itself in serialization and deserialization. However, the other two annotations, @JsonAnySetter and @JsonAnyGetter, will be used for all unrecognized properties. All unrecognized properties will be added to the map for serialization and obtained from the map for deserialization.

@yawkat
Copy link
Member

yawkat commented Jun 1, 2024

JsonAnyGetter/Setter will already not appear in the serialization/deserialization as its own property. The JsonIgnore is not necessary and should not be there

@cowtowncoder
Copy link
Member

@MaximValeev it is possible @JsonIgnore does no harm, but in general it is to remove accessor (getter, setter, field, ctor parameter) from consideration (or if no inclusion, whole property).
And as @yawkat pointed out, it should not be necessary as JsonAnyGetter/Setter has priority over regular property detection.

But as to proper fix: I think #562 would be that (maybe via #4558) -- to may @JsonAnySetter work via Constructor parameter (not just Field or Method).

Alternatively, yes, Kotlin side should annotate Field to use.

@k163377
Copy link
Contributor

k163377 commented Jun 8, 2024

I understood that there is nothing to do in KotlinModule as it is handled by #4558 .

@cowtowncoder
Copy link
Member

@k163377 Yes, I think it's matter of getting #562 resolved (via #4558) and then Kotlin side probably wants to have a test to verify things work there as expected. But hopefully no other work needed (but you never know before testing :) ).

@JooHyukKim
Copy link
Member

Caution : this is not about what has caused the change in behavior in 2.17. Instead, this is about making it work in 2.18.

So I found out that StdValueInstantiator.createFromObjectWith(DeserializationContext, SettableBeanProperty[], PropertyValueBuffer) method is overridden by KotlinValueInstantiator. I think we need some modification either on kotlin-module or databind to support this.

My intuition is that, below implementation in PropertyValueBuffer.getParameters(SettableBeanProperty[]) method...

        // [databind#562] since 2.18 : Respect @JsonAnySetter in @JsonCreator
        if (_anyParamSetter != null) {
            Object anySetterParameterObject = _anyParamSetter.createParameterObject();
            for (PropertyValue pv = _anyParamBuffered; pv != null; pv = pv.next) {
                pv.setValue(anySetterParameterObject);
            }
            _creatorParameters[_anyParamSetter.getParameterIndex()] = anySetterParameterObject;
        }

... should be moved somewhere to make it work?

@cowtowncoder
Copy link
Member

Ideally we should remove the need for KotlinValueInstantiator overrides. But on short term, yes, may need to update override in question.

@mikethecalamity
Copy link

mikethecalamity commented Sep 27, 2024

I'm seeing this issue when I tried to upgrade to 2.18.0, but it seems to e working fine in 2.17.2.

My class has lombok in it, but it looks like this:

@Data
public class MyClass {
    private final int id;
    private final String name;

    @JsonAnyGetter
    @JsonAnySetter
    private final Map<String, Object> params;
}

@cowtowncoder
Copy link
Member

@mikethecalamity that is unfortunately not a full reproduction; would need to add calling code, as well as replace Lombok annotations with processed code (one JVM loads).

If this could be reproduced with plain-non-Lombok-Java that'd be great since so far assumption has been this is Kotlin-specific.
But right now we do not have such test.

@JooHyukKim
Copy link
Member

JooHyukKim commented Sep 28, 2024

As @kotcrab describes, this is Kotlin specific issue, but caused by FasterXML/jackson-annotations#242 in 2.17 which was to support a jackson-databind(#562) in 2.18.

@JooHyukKim
Copy link
Member

JooHyukKim commented Sep 28, 2024

Since this is regression, would you consider reverting FasterXML/jackson-annotations#242 in 2.17 version, @cowtowncoder? I know release is not easy, so no pressure tho.

And we may apply the annotations module change only to 2.18, try fixing things up in 2.18. If we try to fix things without change in Kotlin module, #4720 would do the fix?

@cowtowncoder
Copy link
Member

@JooHyukKim No; annotations are special case where we really cannot change things in patch release, so 2.17.x cannot be changed except in exceptional circumstances. Same now goes for 2.18.x, actually, since 2.18.0 is released.

But if #4720 can fix this issue let's work on getting that done.
I am not opposed to changes to Kotlin module but unfortunately don't really know what to change, where and how.

I am bit worried tho about Kotlin annotation handling, wrt replication of annotations to accessors: we seem to be hitting problems related to that.

@JooHyukKim
Copy link
Member

I am not opposed to changes to Kotlin module but unfortunately don't really know what to change, where and how.

I think we are better off not making changes to Kotlin module, as we do not strictly force using same version of databind and kotlin modules.

I am bit worried tho about Kotlin annotation handling, wrt replication of annotations to accessors: we seem to be hitting problems related to that.

Can you share some pointers that we can follow?

@cowtowncoder
Copy link
Member

@JooHyukKim I was thinking of FasterXML/jackson-annotations#258 that had to be reverted; not sure how closely related these things are.

As to making changes to Kotlin module: agreed that coordinated changes wouldn't work. But if there was Kotlin-module-only fix (requiring no changes here), that'd be tempting.
But I don't think we have such.

@cowtowncoder cowtowncoder added 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels Sep 29, 2024
@cowtowncoder cowtowncoder added this to the 2.18.1 milestone Sep 29, 2024
@cowtowncoder
Copy link
Member

Fix via #4720, will be in 2.18.1

@cowtowncoder
Copy link
Member

I may be wrong but it'd look like this might be causing/dup of:

FasterXML/jackson-module-kotlin#832

lberrymage added a commit to accrescent/parcelo that referenced this issue Oct 24, 2024
Running Parcelo in a container via our current Dockerfile revealed that
"uses-sdk" fields were not being parsed from applications' Android
manifests, effectively preventing app uploads since the targetSdk
property of uses-sdk is required by Parcelo. This bug wasn't caught
until now because it only seems to manifest itself when running via the
Dockerfile; running locally as in our recommended development
environment does not have the issue. The Jackson 2.18.0 upgrade has not
yet been included in a production release, so it's uncertain whether the
issue would have surfaced in our production environment.

We tracked the issue down to a regression in Jackson 2.18.0. The exact
cause is unknown. However, a number of seemingly related issues were
reported for Jackson 2.18.0 [1], so we plan to closely monitor those
issues and test upcoming Jackson releases carefully to prevent breakage.

[1]: See below:

- FasterXML/jackson-module-kotlin#841
- FasterXML/jackson-module-kotlin#842
- FasterXML/jackson-module-kotlin#843
- FasterXML/jackson-module-kotlin#832
- FasterXML/jackson-databind#4508
- FasterXML/jackson-databind#4752
lberrymage added a commit to accrescent/parcelo that referenced this issue Oct 27, 2024
Running Parcelo in a container via our current Dockerfile revealed that
"uses-sdk" fields were not being parsed from applications' Android
manifests, effectively preventing app uploads since the targetSdk
property of uses-sdk is required by Parcelo. This bug wasn't caught
until now because it only seems to manifest itself when running via the
Dockerfile; running locally as in our recommended development
environment does not have the issue. The Jackson 2.18.0 upgrade has not
yet been included in a production release, so it's uncertain whether the
issue would have surfaced in our production environment.

We tracked the issue down to a regression in Jackson 2.18.0. The exact
cause is unknown. However, a number of seemingly related issues were
reported for Jackson 2.18.0 [1], so we plan to closely monitor those
issues and test upcoming Jackson releases carefully to prevent breakage.

[1]: See below:

- FasterXML/jackson-module-kotlin#841
- FasterXML/jackson-module-kotlin#842
- FasterXML/jackson-module-kotlin#843
- FasterXML/jackson-module-kotlin#832
- FasterXML/jackson-databind#4508
- FasterXML/jackson-databind#4752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants