Skip to content

Conversation

nkonev
Copy link
Contributor

@nkonev nkonev commented Dec 27, 2021

As discussed here #636 (comment)

@KENNYSOFT @OlgaMaciaszek Please review.

@nkonev nkonev changed the title fix cascading deserialization Spring Data's Page by setting @JsonIgnoreProperties(ignoreUnknown = true) Fix cascading deserialization Spring Data's Page by setting @JsonIgnoreProperties(ignoreUnknown = true) Dec 27, 2021
@nkonev nkonev changed the base branch from 3.0.x to main December 27, 2021 20:49
Copy link
Contributor

@KENNYSOFT KENNYSOFT left a comment

Choose a reason for hiding this comment

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

Alright, now the deserialization won't fail with unknown properties, so we may rollback SimplePageImpl.getTotalPages() and some methods to @JsonProperty, since v3.0.6 is a patch release and should not have any breaking changes. (The base branch of this PR should be 3.0.x also, to fix 3.0.6 #636 (review))

I've confirmed rollback getTotalPages(), getNumberOfElements(), isFirst(), isLast() from @JsonIgnore to @JsonProperty doesn't break any tests.

@codecov
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

Merging #653 (6f28ff9) into main (10ad8a0) will increase coverage by 0.50%.
The diff coverage is n/a.

❗ Current head 6f28ff9 differs from pull request most recent head b59e597. Consider uploading reports for the commit b59e597 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main     #653      +/-   ##
============================================
+ Coverage     79.83%   80.34%   +0.50%     
- Complexity      545      546       +1     
============================================
  Files            66       66              
  Lines          1984     1984              
  Branches        274      275       +1     
============================================
+ Hits           1584     1594      +10     
+ Misses          250      242       -8     
+ Partials        150      148       -2     
Impacted Files Coverage Δ
...ork/cloud/openfeign/support/PageJacksonModule.java 78.12% <ø> (+18.75%) ⬆️
...mework/cloud/openfeign/FeignAutoConfiguration.java 89.70% <0.00%> (ø)
...enfeign/clientconfig/OkHttpFeignConfiguration.java 93.33% <0.00%> (ø)
...mework/cloud/openfeign/FeignClientFactoryBean.java 77.99% <0.00%> (+0.38%) ⬆️
...ork/cloud/openfeign/support/SpringMvcContract.java 89.02% <0.00%> (+0.60%) ⬆️
...ign/clientconfig/HttpClientFeignConfiguration.java 87.09% <0.00%> (+6.45%) ⬆️

@nkonev
Copy link
Contributor Author

nkonev commented Jan 11, 2022

Hi @OlgaMaciaszek Can you take it to 2021.0.1 and 2020.6 releases ?

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

@nkonev Thanks for submitting the fix. In general looks good. However, I've added a comment about a cosmetic issue - please address it. I don't think we're going to release 3.0.x anymore, but we will be releasing 3.1.x - could you please submit this against 3.1.x instead of main so that we can include it in both release trains.

@OlgaMaciaszek OlgaMaciaszek added the bug Something isn't working label Jan 14, 2022
@OlgaMaciaszek OlgaMaciaszek added this to the 3.1.1 milestone Jan 14, 2022
…reProperties(ignoreUnknown = true), revert ignoring getTotalPages(), getNumberOfElements(), isFirst(), isLast()
@nkonev nkonev changed the base branch from main to 3.1.x January 14, 2022 15:59
@KENNYSOFT
Copy link
Contributor

Thanks for submitting the fix. In general looks good. However, I've added a comment about a cosmetic issue - please address it. I don't think we're going to release 3.0.x anymore, but we will be releasing 3.1.x - could you please submit this against 3.1.x instead of main so that we can include it in both release trains.

Sad to see you're dropping support for 3.0.x release. It's in the Spring Cloud 2020.0.x release train but really late 2020 (December) so it was only 13 months ago.

@nkonev nkonev requested a review from OlgaMaciaszek January 14, 2022 16:01
@OlgaMaciaszek
Copy link
Collaborator

3.1.x will not be introducing breaking changes, so should be seamless to migrate. You can find our current support timelines here: https://spring.io/projects/spring-cloud-openfeign#support

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit 5dff9cf into spring-cloud:3.1.x Jan 14, 2022
@KENNYSOFT
Copy link
Contributor

@OlgaMaciaszek Why was this PR not cherry-picked to v3.0.7? 😭

@OlgaMaciaszek
Copy link
Collaborator

@KENNYSOFT It was my mistake. Was looking for this discussion in issues. Did not realise it was in PRs instead. Will get a 3.0.8 released just for OpenFeign later this week to get this in.

OlgaMaciaszek pushed a commit that referenced this pull request Jul 25, 2022
…reProperties(ignoreUnknown = true), revert ignoring getTotalPages(), getNumberOfElements(), isFirst(), isLast() (#653)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants