Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpHeaders equals may return false without comparing actual content #25034

Closed
rstoyanchev opened this issue May 8, 2020 · 3 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 8, 2020

In 5.1 HttpHeaders became a MultiValueMap and it became possible to wrap existing HttpHeaders instances rather than copy their contents. As a result it's possible to have multiple HttpHeaders wrappers around the actual MultiValueMap content.

The equals method tries to compare the MultiValueMap content by nesting on the headers field but if the levels of nesting are uneven, it may return false. For example:

HttpHeaders headers1 = new HttpHeaders();
HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1));

assertThat(headers1.equals(headers2)).isTrue(); // Pass
assertThat(headers2.equals(headers1)).isTrue(); // Fail
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels May 8, 2020
@rstoyanchev rstoyanchev added this to the 5.2.7 milestone May 8, 2020
@rstoyanchev rstoyanchev self-assigned this May 8, 2020
@rstoyanchev rstoyanchev changed the title HttpHeaders equals may fail to compare map content HttpHeaders equals may return false without comparing actual content May 8, 2020
@jhoeller jhoeller added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label May 8, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels May 8, 2020
@jhoeller
Copy link
Contributor

jhoeller commented May 8, 2020

Let's backport this to 5.1.x as well then, given that it dates back to a 5.1 change... and that the equals implementation seems to be the same.

@snicoll
Copy link
Member

snicoll commented Jun 6, 2020

This is a binary incompatible change that breaks Spring REST docs, can we please reconsider it?

Tests run: 4, Failures: 0, Errors: 4, Skipped: 0, Time elapsed: 0.435 s <<< FAILURE! - in org.springframework.boot.actuate.autoconfigure.endpoint.web.documentation.CachesEndpointDocumentationTests
evictAllCaches(org.springframework.boot.actuate.autoconfigure.endpoint.web.documentation.CachesEndpointDocumentationTests)  Time elapsed: 0.012 s  <<< ERROR!
java.lang.NoSuchMethodError: org.springframework.http.HttpHeaders.readOnlyHttpHeaders(Lorg/springframework/http/HttpHeaders;)Lorg/springframework/http/HttpHeaders;
	at org.springframework.restdocs.operation.HttpHeadersHelper.getHeaders(HttpHeadersHelper.java:63)
	at org.springframework.restdocs.operation.OperationRequestFactory.augmentHeaders(OperationRequestFactory.java:109)
	at org.springframework.restdocs.operation.OperationRequestFactory.create(OperationRequestFactory.java:48)
	at org.springframework.restdocs.mockmvc.MockMvcRequestConverter.convert(MockMvcRequestConverter.java:76)
	at org.springframework.restdocs.mockmvc.MockMvcRequestConverter.convert(MockMvcRequestConverter.java:55)
	at org.springframework.restdocs.generate.RestDocumentationGenerator.handle(RestDocumentationGenerator.java:186)

@snicoll snicoll reopened this Jun 6, 2020
@jhoeller
Copy link
Contributor

jhoeller commented Jun 6, 2020

It seems that the accepted argument got relaxed from HttpHeaders to MultiValueMap<String, String> which is source compatible but not binary compatible. The relaxed variant only seems to be used in master, so I suppose we'll keep it as an overloaded variant there, even if we still have to reintroduce the old signature there as well.

We need to change the signature back to the original in 5.2.x and 5.1.x. I've got a few other things to backport as well, so I'll take care of it.

FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants