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

Introduce config override for includeAsProperty (#1522) #1564

Conversation

CarstenWickner
Copy link
Contributor

@cowtowncoder this is a proof-of-concept how I'd image the include override per property type to work.

Unfortunately, I cannot be sure to have covered all relevant places due to the existing complexity of the databind library. However I hope it is helpful.

Since I couldn't build the current master branch without further setup, I based this on the 2.8 branch.

*
* @since 2.8.8
*/
public abstract JsonInclude.Value getDefaultPropertyInclusion(Class<?> baseType,
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, so could not introduce in a patch for sure, and not really in a patch either. Could of course add a new method, deprecate old one, add bridge method.
But what is the meaning of additional type argument here? Why is baseType not sufficient? I thought it was meant specifically as the type for lookups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, that it shouldn't be merged as-is. As mentioned on the initial PR comment, this is just intended to be a proof-of-concept.

As far I understood the usage of these methods, the baseType refers to the type of bean containing the property and the additional type argument refers to the type of the property itself. The existing methods cannot look up the override configuration for a single property as they are not aware of its type.

@CarstenWickner
Copy link
Contributor Author

Closing this pull requests, as the respective changes are now applied based on the current version 2.9 in a new pull request #1566.

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

2 participants