Introduce config override for includeAsProperty #1566
Conversation
|
Apologies for this taking long time: it's high on my todo list but fixing some urgent 2.8 issues (to get 2.8.8 out), and remaining 2.9 oddities has taken longer than expected. But I wanted to add a note here since I think this is a valuable addition I hope also makes it in 2.9 (which is not going to happen for all issues marker with |
respect member's type in findPropertyInclusion() if available
| { | ||
| JsonInclude.Value v0 = getConfigOverride(propertyType).getIncludeAsProperty(); | ||
| if (v0 != null) { | ||
| return v0; |
There was a problem hiding this comment.
May not be a big issue, but usually it'd make sense to merge with defaults;
JsonInclude.Value.merge(defaultIncl, v0);
method does handle null without problems (in case defaultIncl passed as null).
I think this mostly matters for things like global defaults for just one aspects (like don't include nulls for values); not sure if this is commonly used (probably isn't), but easy to add.
There was a problem hiding this comment.
This would be inconsistent with the logic in getDefaultPropertyInclusion(Class, Value).
Should I add it anyway? And if yes, should this merging also cover the potential other override value?
JsonInclude.Value result = defaultIncl;
JsonInclude.Value v1 = getConfigOverride(baseType).getInclude();
if (v1 != null) {
result = JsonInclude.Value.merge(result, v1);
)
JsonInclude.Value v0 = getConfigOverride(propertyType).getIncludeAsProperty();
if (v0 != null) {
result = JsonInclude.Value.merge(result, v0);
)
return result;There was a problem hiding this comment.
Yes, I think this is correct. I do not recall why merge wasn't done in the other case; may have been a simple oversight on my part.
| * | ||
| * @since 2.9 | ||
| */ | ||
| public abstract JsonInclude.Value getDefaultInclusion(Class<?> baseType, |
There was a problem hiding this comment.
Would be good to add brief explanation regarding difference between baseType, propertyType, in context of properties of given POJO.
| @@ -41,6 +41,11 @@ public MutableConfigOverride setInclude(JsonInclude.Value v) { | |||
| return this; | |||
| } | |||
|
|
|||
| public MutableConfigOverride setIncludeAsProperty(JsonInclude.Value v) { | |||
There was a problem hiding this comment.
Would be nice to add javadocs (even if other methods don't necessarily have them either :) ) for brief explanation.
| // latter having precedence) | ||
| // Cc) per-property override | ||
| // config override having precedence) | ||
| // (c) per-property override (from annotation or config overrides; |
There was a problem hiding this comment.
This is sort of correct but maybe misleading, as config override is for type-of-property, but not for specific property. Not sure how to properly explain the distinction.
| * {@link JsonSerialize#include} annotation property work | ||
| * as expected. | ||
| */ | ||
| public class JsonIncludeOverrideTest |
There was a problem hiding this comment.
Perhaps it would make sense to create a separate test class to contain new tests? Not sure which makes most sense: these are of course related, just trying to find good balance between huge test classes and big number of classes.
|
Ok so I think this makes sense, and I hope to get this integrated for 2.9. Added some minor comments for consideration. |
|
Updated according to review comments. Let me know if I should move the tests around or fix-up something else. |
|
I think we'll call it good enough! |
Re-created pull request #1564 based on current
masterbranch (version 2.9). Implements #1522To avoid collisions with the existing (overloaded)
getDefaultPropertyInclusion()methods, I added the new ones (with the additionalClass<?>parameter) asgetDefaultInclusion(), since they are explicitly looking up theJsonInclude.Valuefor a single property.Please have another look, @cowtowncoder. The added unit tests should demonstrate why this second
Class<?>parameter is required.