openapi-tools / jackson-dataformat-hal

Dataformat extension for Jackson to handle HAL JSON
MIT License
24 stars 6 forks source link

Add support for @JsonView #37

Closed ghost closed 3 years ago

ghost commented 3 years ago

This PR addresses Issue #33 by updating the HALBeanSerializer to use the _filteredProps when it is not null and there is an active JSON View. The _filteredProps is further filtered since the default Bean Serializer enforces the JSONView in the serializeAsField, which is not used for the serialization of the Link/Embedded props themselves.

arucard21 commented 3 years ago

I took a quick look and, while I'm not too familiar with the Jackson databind code, it seems pretty straightforward and looks good. I do have a suggestion for some more tests, which should be easy enough to add with the setup that you already have. It might be good to add the following:

codecov[bot] commented 3 years ago

Codecov Report

Merging #37 (627eb0b) into master (c73f726) will increase coverage by 0.17%. The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #37      +/-   ##
============================================
+ Coverage     76.48%   76.66%   +0.17%     
  Complexity       63       63              
============================================
  Files            10       10              
  Lines           353      360       +7     
  Branches         67       69       +2     
============================================
+ Hits            270      276       +6     
  Misses           74       74              
- Partials          9       10       +1     
Impacted Files Coverage Δ Complexity Δ
.../jackson/dataformat/hal/ser/HALBeanSerializer.java 82.48% <87.50%> (+0.17%) 4.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c73f726...627eb0b. Read the comment docs.

ghost commented 3 years ago

I have added the additional tests. The Deserialization tests confirms there are no existing issues wrt JSONViews.

The props in the _filteredProps are wrapped in FilteredBeanPropertyWriter, which makes the decision whether to write the property or not. So just using the _filteredProps was not enough as the HALBeanSerializer doesn't call the FilteredBeanPropertyWriter.serializeAsXXXX method on the prop for Link/Embedded props, but rather call the writeXXXX methods directly.

The additional filtering of _filteredProps is a little over zealous as it handles not just the Link/Embedded props, but all props of the Resource. I don't see this as a big deal and could even improve some performance as all the unnecessary FilteredBeanPropertyWriter.serializeAsXXX calls are skipped.

langecode commented 3 years ago

Thx for the contribution @timothy-volvo and for the review @arucard21 - I have merged it will create a new release soon.