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

Allow using @JsonPropertyOrder with any-property (@JsonAnyGetter) #4396

Closed
wants to merge 28 commits into from

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Feb 22, 2024

(For after Property Introspection Rewrite)

This PR resolves...

1.. #4388
2.. part of #2592
- AnyGetter with JsonInclude + JsonFilter combo

Modifications

This PR basically...

  • Starts to handle Any-Getter property as regular property (for serialization only).
  • Makes AnyGetterWriter extend BeanPropertyWriter.

notes

  1. If and only if direction is not off.... will clean up and refactor. Solution currently seems a bit of refactoring.
  2. This might allow "ordering" and "ignoring" within AnyGetter prop (I haven't found time to check yet tho)
  3. Change might be to big that maybe

@cowtowncoder
Copy link
Member

One quick note: I think ordering within "any-getter" group can be out of scope as users can use specific Maps to enforce iteration order, and since it is probably more important to be able to locate group itself.
But I think that's what is being done here, just mentioning it.

Otherwise, I'll have to read this couple of times. I guess it would make sense to try to make any-getter placeholder work similar to "simple" properties, so sorting works. But I wonder if there are complications wrt attempts to link/unlink, as well as ignoral. This because formerly "name" of any-getter hasn't had any effect and could not have accidentally ignored (for example) by a regular property with same name having @JsonIgnore.

@JooHyukKim
Copy link
Member Author

JooHyukKim commented Feb 24, 2024

But I wonder if there are complications wrt attempts to link/unlink, as well as ignoral. This because formerly "name" of any-getter hasn't had any effect and could not have accidentally ignored (for example) by a regular property with same name having @JsonIgnore.

Valid point with respect to @JsonIgnore and stuff, so I added tests...

  • On working @JsonIgnore and @JsonIgnoreProperties via 78eb9d5.
  • On @JsonPropertyOrder explicitly on any-getter property works via 8305a0b

Could you explain a bit more on link/unlink?

@cowtowncoder
Copy link
Member

Link/unlink: different getter/setter methods, fields, get "linked" if they have same logical name, either implicit (default) or explicit (rename), into a single logical property.
So if such name that an any-getter happens to have matches a regular property, regular property might be annotated with @JsonIgnore (or maybe any-getter) -- and so the whole property is ignored (none of accessors used).

So all I am wondering is if there was a case like:

class Stuff {
  @JsonAnyGetter
  Map<String, Object> metadata;

  @JsonIgnore
  public String getMetadata() { // unrelated to any-setter
     ...
  }
}

where any-setter would be ignored due to name match. Or something similar related to accidentally matching names.

@JooHyukKim
Copy link
Member Author

Fortunately, seems to work wrt link/unlink-ed properties 👍🏼. Added more tests via 85ae52e.

@@ -218,6 +218,8 @@ public void serializeAsField(Object pojo, JsonGenerator jgen,
writer.serializeAsField(pojo, jgen, provider);
} else if (!jgen.canOmitFields()) { // since 2.3
writer.serializeAsOmittedField(pojo, jgen, provider);
} else if (writer instanceof AnyGetterWriter) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP : Contemplating on how should we optimize this check...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried referring to other writer/serializers and it turns out instanceof here check seems sort of reasonable.

@JooHyukKim JooHyukKim marked this pull request as ready for review June 9, 2024 13:16
@JooHyukKim
Copy link
Member Author

Would this still be considered for 2.18, @cowtowncoder?

@cowtowncoder
Copy link
Member

@JooHyukKim potentially yes.

AnnotatedMember accessor, JsonSerializer<?> serializer)
{
super(parent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually safe? Doesn't it copy settings of "parent"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was intended actually. Copying seemed safer than holding reference to original BeanPropertyWriterpropert propert that is soon to be a any-getter.
I referenced same pattern in VirtualBeanPropertyWriter, UnwrappingBeanPropertyWriter, like below.
Do you think there would be side effect?

    public UnwrappingBeanPropertyWriter(BeanPropertyWriter base, NameTransformer unwrapper) {
        super(base);
        _nameTransformer = unwrapper;
    }

    protected UnwrappingBeanPropertyWriter(UnwrappingBeanPropertyWriter base, NameTransformer transformer,
            SerializedString name) {
        super(base, name);
        _nameTransformer = transformer;
    }

@cowtowncoder
Copy link
Member

Just a heads up: as much as I'd like to get this merged, I think it will have to wait for 2.19 -- I am currently trying hard to close up remaining issues and I think this is relatively high-risk change.

@JooHyukKim
Copy link
Member Author

Agreed👍🏼👍🏼. No need to risk things.

@cowtowncoder
Copy link
Member

@JooHyukKim Would need to get this rebased or recreated against 2.19 (I recall Github UI having some problems if trying to simply change base branch).

@JooHyukKim
Copy link
Member Author

Closing in favor of #4775

@JooHyukKim JooHyukKim closed this Nov 1, 2024
@JooHyukKim JooHyukKim deleted the fix-4388 branch November 1, 2024 23:33
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

Successfully merging this pull request may close these issues.

2 participants