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

Test and fix inheritance propagation of @JsonIgnoreProperties #385

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

tkalmar
Copy link

@tkalmar tkalmar commented Jul 2, 2020

After fixing #374 a colleague pointed out, that this fix isn't working. So i sat down and analyzed the problem. Only investigating the superclass isnt sufficient one has to inspect the whole class hierarchy to get the same behaviour as jackson. I ensured the upstreambehaviour with an test and fixed the missing ascension up the inheritance tree.
I can leave out the upstream behaviuor test, but i think it is saver to get an notice if upstream changes behaviour.

@MikeEdgar
Copy link
Member

@tkalmar , @phillip-kruger - thinking more about this issue, I am wondering if a better approach would be to check whether the properties are ignored in the TypeResolver as the hierarchy is processed there. I'm still thinking through that, I'll reply with some more thoughts on that approach if you agree.

@MikeEdgar
Copy link
Member

This is where I landed: MikeEdgar@8cb217c, which seems to work with @tkalmar's original test case in the TCK project. I haven't incorporated the test changes from this PR, however. I can cherry pick the test changes from this PR if this seems like the correct approach.

@tkalmar
Copy link
Author

tkalmar commented Jul 13, 2020

This is where I landed: MikeEdgar@8cb217c, which seems to work with @tkalmar's original test case in the TCK project. I haven't incorporated the test changes from this PR, however. I can cherry pick the test changes from this PR if this seems like the correct approach.

Checked your branch against my tests, works as expected. So feel free to cherry pick them if needet/wanted

Thomas Kalmár and others added 2 commits July 13, 2020 18:54
Conflicts:
	core/src/main/java/io/smallrye/openapi/runtime/scanner/dataobject/IgnoreResolver.java
@MikeEdgar
Copy link
Member

@tkalmar - I've applied your commit to my branch and made a few updates based on your feedback. If you don't mind, I will go ahead and push it to your jsonignoreproperties branch and we can merge this PR.

@tkalmar
Copy link
Author

tkalmar commented Jul 14, 2020

@MikeEdgar LGTM

Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Approved. Since I added some changes here also, do you want to give it a look @phillip-kruger ?

@MikeEdgar MikeEdgar added this to the 2.0.3 milestone Jul 15, 2020
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

Thanks @MikeEdgar and @tkalmar

@phillip-kruger phillip-kruger merged commit d349a3a into smallrye:master Jul 15, 2020
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.

None yet

3 participants