Skip to content

Copy JsonProperty annotation to getter#2915

Closed
XenoAmess wants to merge 2 commits intoprojectlombok:masterfrom
xenoamess-fork:copy_json_property_annotation_to_getter
Closed

Copy JsonProperty annotation to getter#2915
XenoAmess wants to merge 2 commits intoprojectlombok:masterfrom
xenoamess-fork:copy_json_property_annotation_to_getter

Conversation

@XenoAmess
Copy link
Copy Markdown

I strongly suggest copy JsonProperty annotation onto getter functions.
tests applied.

@janrieke
Copy link
Copy Markdown
Contributor

janrieke commented Aug 2, 2021

I don't understand why copying is necessary at all. IIRC Jackson considers field, getter, and setter as a unit; an annotation on one of them holds for all.
Shouldn't Lombok simply stop copying those annotations at all (except for builders)?

@XenoAmess
Copy link
Copy Markdown
Author

XenoAmess commented Aug 3, 2021

I don't understand why copying is necessary at all. IIRC Jackson considers field, getter, and setter as a unit; an annotation on one of them holds for all.
Shouldn't Lombok simply stop copying those annotations at all (except for builders)?

I provided the test classes. You can just run it to see.
In short if you don't copy it to getter,when it have no setter(means final),the serialized json will have two such fields,one original name one translated name. It is weird.

@XenoAmess
Copy link
Copy Markdown
Author

Hi. is there any news?

@janrieke
Copy link
Copy Markdown
Contributor

Jackson's behavior appears strange to me. Why should property annotation merging not occur when there is no setter?
I did not find a hint in the documentation that explains this. Thus, I consider this a Jackson bug. Could you file an issue at Jackson?

@XenoAmess
Copy link
Copy Markdown
Author

Jackson's behavior appears strange to me. Why should property annotation merging not occur when there is no setter? I did not find a hint in the documentation that explains this. Thus, I consider this a Jackson bug. Could you file an issue at Jackson?

issue created https://github.com/FasterXML/jackson-core/issues/735

@janrieke
Copy link
Copy Markdown
Contributor

janrieke commented Jan 2, 2022

So my conclusion is that this is (again) a result of the crappy bean-spec. Question is: Should lombok once again try fixing this by copying the annotation?

When lombok introduced/extended the copy-to-setter mechanism, that already had unintended side-effects: #2778, #2769
Right now, once lombok copies an annotation, there is no way for users to stop lombok from doing so. So users would have to remove the lombok annotation and implement the generated methods by themselves. That's a workaround I personally do not like.
In contrast, the workaround for this case is quite easy: @Getter(onMethod_=@JsonProperty("xYz"))

However, you may argue that if lombok copies an annotation to a setter, then it should also be put on a getter. I agree with that. Right now, I don't see a reason why this would cause further issues with Jackson, but I did not investigate further. However, I still vote for lombok to stop copying completely (except for builders). But that's a decision for the maintainers.

If the maintainers decide that lombok should also copy Jackson annotations to the getter, this PR is most likely not sufficient. There are several other Jackson annotations that we have to look at, and we need to make sure that this really does not break any more use-cases.

@XenoAmess
Copy link
Copy Markdown
Author

XenoAmess commented Jan 2, 2022

However, you may argue that if lombok copies an annotation to a setter, then it should also be put on a getter. I agree with that.

@janrieke

Yes, I really do.

I really hate inconsistency like this...

I can't give very much detail as that is somehow company secret, but it has really already bring me some trouble.

@rzwitserloot
Copy link
Copy Markdown
Collaborator

If @janrieke is not willing to call that the right move is unambiguously to copy it to the getter, well, my (and @rspilker's) thorough lack of experience with jackson means we can't call it either. No go.

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.

3 participants