Support size = 0 in PageJacksonModule with test

[nkonev]

From #592

[codecov]

Codecov Report

Merging #636 (608e088) into main (f7e4281) will decrease coverage by 0.04%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##               main     #636      +/-   ##
============================================
- Coverage     79.48%   79.43%   -0.05%     
  Complexity      537      537              
============================================
  Files            65       65              
  Lines          1955     1960       +5     
  Branches        271      272       +1     
============================================
+ Hits           1554     1557       +3     
- Misses          252      254       +2     
  Partials        149      149              

Impacted Files	Coverage Δ
...ork/cloud/openfeign/support/PageJacksonModule.java	59.37% <87.50%> (+0.11%)	⬆️

[OlgaMaciaszek]

@nkonev Thanks for fixing this up. Looks good. However, I'm going to cherry-pick this commit directly to 3.0.x, so that this is also included in the 2020.0.x release train.

[OlgaMaciaszek]

Merged with ad892a2.

[KENNYSOFT]

Was there any reason to add @JsonIgnore annotation to such properties? I'm facing the property missing problem, since we just get Page<T> instance from the FeignClient and then return it to the response.

[nkonev]

I ignored them because ignored properties didn't deserialized correctly, try un-ignore them and run PageJacksonModuleTests.

[KENNYSOFT]

Thanks for the fast reply. I've confirmed that it says:

com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "last" (class org.springframework.cloud.openfeign.support.PageJacksonModule$SimplePageImpl), not marked as ignorable (5 known properties: "content", "totalElements", "sort", "number", "size"])
at [Source: (String)"{"content":[],"pageable":"INSTANCE","totalElements":0,"last":true,"totalPages":1,"size":0,"number":0,"sort":[],"numberOfElements":0,"first":true,"empty":true}"; line: 1, column: 132] (through reference chain: org.springframework.cloud.openfeign.support.PageJacksonModule$SimplePageImpl["last"])

But I don't think changing @JsonProperty to @JsonIgnore is a good choice. Instead, we'd better disable FAIL_ON_UNKNOWN_PROPERTIES from the objectMapper, since the PageJacksonModule should be able to deserialize from any other form of Page<T> JSON.

We can see the test passes with un-ignore and disable the feature as mentioned:

objectMapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

@OlgaMaciaszek Can I get your opinion?

[nkonev]

@KENNYSOFT
I agree, the using @IgnoreProperty could be not very good idea.

About disabling feature: personally I would prefer to use @JsonIgnoreProperties(ignoreUnknown = true)
Like here https://github.com/spring-projects/spring-security/blob/ad907457eeeee46da101215f5408d3cd98de4881/web/src/main/java/org/springframework/security/web/jackson2/DefaultCsrfTokenMixin.java#L39 Modification of "global" objectMapper is not good choice because many other parts use it.

Can you provide reproducer? In order to include it to test.

I tried "cascase" serialization-deserialization-serializaton and it failed.

[KENNYSOFT]

Actually, I've modified a local objectMapper, especially https://github.com/spring-cloud/spring-cloud-openfeign/pull/636/files#diff-6338018e154894301cfd5d80b35fd208bf2ce3a6387c3d829d5e2cbc60edcbccR48 here.

[nkonev]

#653

[OlgaMaciaszek]

Hi, @KENNYSOFT Thanks for raising it. I agree with @nkonev that @JsonIgnoreProperties(ignoreUnknown = true) is a better solution.