-
Notifications
You must be signed in to change notification settings - Fork 38.3k
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
Fix comparison of title
in equals()
and hashCode()
of ProblemDetail
#30294
Conversation
Lazy computed title property should be taken into account
equals()
and hashCode()
in ProblemDetail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR @quaff but what's the rationale behind this?
using the getTitle()
over this.title
isn't that much of an improvement because if the field is not set, the getter derives the result from the this.status
field (namely, the reason phrase of the enum which is constant). since this.status
already participates in both equals()
and hashcode()
, this change doesn't add new information to these methods.
it even arguably can make them less performant, because now instead of a simple null equality + int comparison, it adds resolution of an enum from the status code + getting the enum's reason phrase to the mix.
so overall I don't see the benefit of this change.
You can review the test I added, currently the ResponseEntity<ProblemDetail> resp = this.testRestTemplate.exchange(
RequestEntity.method(HttpMethod.GET, path).header(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE).build(),
ProblemDetail.class);
ProblemDetail expected = ProblemDetail.forStatusAndDetail(HttpStatus.CONFLICT, "Conflicts arise");
expected.setTitle(HttpStatus.CONFLICT.getReasonPhrase());
expected.setInstance(URI.create(path));
assertThat(resp.getBody()).isEqualTo(expected); Title must be explicit set before comparison, but it is implicit at server side, that's not idiomatic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sense to use getTitle()
to get a consistent value.
equals()
and hashCode()
in ProblemDetail
title
in equals()
and hashCode()
of ProblemDetail
Lazy computed title property should be taken into account See gh-30294
Lazy computed title property should be taken into account