PR for using @JsonApplyView (#5745)#5754
Conversation
… and objectIdGenerators
| * @return view (represented by class) that the property will be processed with; | ||
| * if null, processing will use the current view if any | ||
| */ | ||
| public Class<?> findApplyView(MapperConfig<?> config, Annotated a) { return null; } |
There was a problem hiding this comment.
Should the translation of "no view" be handled here?
(if so, marker effectively means "no override", same as no annotation -- except that allows for mix-ins and sub-classes to override and disable @JsonApplyView of target/base class).
|
@cowtowncoder thanks for supporting and helping me. You suggested the implementation shall go another direction with a caching of BeanWriterProperty and not cloning SerializationContext/SerializationConfig although it always check first if active view has changed or not and avoid unnecessary cloning (minimal performance impact?). Shall I attempt to find out where else we can do that, or do you have something specific in mind? |
…PropertyWriter)
|
@cowtowncoder in the meanwhile I reworked the handling in BeanWriterProperty to read the annotation just once. Maybe it's enough so, as the SerializationContext creation happens only when needed and is ultimately just a copy by reference of the properties. If you still wish to make the activeView accessible, save it before and restore it after the property it should be quite easy as well. But then you possibly have an activeView not matching its config... |
|
@cowtowncoder would you prefer another cleaner push request to be able to merge it easily? |
|
@cowtowncoder is this implementation fine enough or would you prefer to mutate the context instead. |
|
Ok, sorry, so no, not good as-is -- I don't think new Instead, active view in needs to be changed mutable. Method could then be added, something like which would switch view around call. As to this or new PR, no Strong opinion, either way fine with me. During PR could also temporarily add new annotation in databind package to make code compile & CI run. |
|
@cowtowncoder I managed to revert the cloning of Context and Config and just modify the activeView in BeanPropertyWriter. Can you review my change? And possibly move forward with this PR? Thanks |
| protected final Class<?>[] _includeInViews; | ||
|
|
||
| /** | ||
| * Alternate set of property writers used when applyView is |
There was a problem hiding this comment.
Yes, I suggest the following:
/**
* View to apply for this property when applyView is available for the Bean.
*
* @SInCE 3.2
*/
|
|
||
| Class<?> currentActiveView = ctxt.getActiveView(); | ||
| try { | ||
| ctxt.setActiveView(getAppliedView(ctxt)); |
There was a problem hiding this comment.
This now adds overhead for every write call on hot path.
Not good. Should check if _applyView is null, if so, fast patch with no changes; if not null, then handle active view.
There was a problem hiding this comment.
I did it as asked. Please review again
|
@cowtowncoder here the corrections. |
|
@cowtowncoder did you get the cla? And does the change suits you? Thks |
|
@cowtowncoder I added some more test (or more precisely more complete tests) combining applying another view, suppressing view, and nesting beans. Any other idea of what we shall test? Once again thanks for your involvement and looking forward to 3.2.0 |
| public Bean beanWithApplyViewB = new Bean(); | ||
|
|
||
| @JsonView(ViewA.class) | ||
| @JsonApplyView |
There was a problem hiding this comment.
Hmmmh. Should we actually allow this at all -- or require explicit use of NONE.class?
Looking at this case it is not obvious to me that plain @JsonApplyView means "disable View altogether". I would prefer that it'd need to be explicit like:
@JsonApplyView(NONE.class)
or -- perhaps -- either:
- Keep one constant but rename as
NO_VIEW - Keep
NONEas default but mean "@JsonApplyView that does nothing", addNO_VIEWorDISABLE_VIEWto mean setting Active View tonull(so no View processing).
| Bean2 bean2 = new Bean2(); | ||
|
|
||
| StringWriter sw = new StringWriter(); | ||
| MAPPER.writerWithView(ViewA.class).writeValue(sw, bean2); |
There was a problem hiding this comment.
nit: could just use writeValueAsString() instead of StringWriter.
Good stuff! I think test coverage may be ok now. But one thing I realized -- and commented about -- was whether use of default value for
alternatively if we wanted to allow Aside from this one last concern I think we are done. |
|
I also see three options
As for me I could live with any of the three. The NoOp does not seem necessary to me because semantically you want to apply a view or suppress a view for a given property and just documenting the special value should be enough. So the current implementation. But if we go with 2) then for the sake of completeness we should add the active special value anyway. The last option would obviously be more explicit but what if both annotation are used on the same property. Not to mention it add some more work. |
I don't think what you list cover same cases as mine do. Now: for me separate annotation seems like unnecessary, so no for option 3. But wrt "no-op", to me the only use would be as mix-in override to "disable" What I mean by |
|
Ok, so I do want to prevent use of plain without argument. And question then is whether constant |
|
Made change via FasterXML/jackson-annotations#347 (with minor Javadoc changes). |
|
@cowtowncoder I think it makes perfectly sense to just drop the default forcing usage of the special value class to force the view to NULL. I wanted to thank you for actively contributing and supporting this feature and PR. I'm still amazed by your work and contribution in this great library. Looking forward to using the release candidate 3.2.0 ;) |
@JsonApplyView (#5745)
|
@f-aubert Finally found time to finish up merging. Phew! Thank you for all your work here: should be good for 3.2.0. And maybe add deserialization support, probably for 3.3 (I am planning to finalize 3.2.0 this or next week). |
|
@cowtowncoder : how does it look like with first release candidate of Jackson 3.2? If you want to start discussing the deserialization now or later, I'm ready to help or discuss |
|
@f-aubert Bit slower than planned, but planning notes are here: FasterXML/jackson-future-ideas#106 so probably skipping RCs altogether. But will also need Jackson 2.22(.0) released first. But if you could file an issue for "Deserialization support for |
As discussed, here a Draft PR for #5745.