Add Equals and HashCode methods for better comparison. #16842
Conversation
Closes spring-projectsgh-16394 Signed-off-by: Maximilian Klose <[email protected]>
spring-projects-issues
added
the
status: waiting-for-triage
jzheaux
added
type: enhancement
There was a problem hiding this comment.
Thanks for the PR @GrmpfNarf! I have included some feedback below. Also please note that you will need to add a Signed-off-by trailer in the commit, per contributing guidelines.
| } | ||
| OAuth2AuthorizationRequest that = (OAuth2AuthorizationRequest) obj; | ||
|
|
||
| if (!this.authorizationUri.equals(that.authorizationUri)) { |
There was a problem hiding this comment.
I think it might be nice to just use Objects.equals(a, b) for each of these checks and chain them together with &&. Would you mind doing that to make the entire method more concise while still being null-safe?
return Objects.equals(a, b)
&& Objects.equals(...)
...
| @Override | ||
| public int hashCode() { | ||
| int result = this.authorizationUri.hashCode(); | ||
| result = 31 * result + this.clientId.hashCode(); |
There was a problem hiding this comment.
Similar to equals, it would be nicer to use Objects.hashCode() for each field to make the method more concise. Would you mind updating that?
| @@ -28,8 +29,7 @@ | |||
|
|
|||
| import org.springframework.security.oauth2.core.AuthorizationGrantType; | |||
|
|
|||
| import static org.assertj.core.api.Assertions.assertThat; | |||
| import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; | |||
| import static org.assertj.core.api.Assertions.*; | |||
There was a problem hiding this comment.
I would recommend updating import settings for your IDE to not collapse imports, as this style of import will fail the build.
| @@ -365,4 +365,45 @@ public void buildWhenAdditionalParametersContainsNullThenAuthorizationRequestUri | |||
| + "item1=null&item2=value2"); | |||
| } | |||
|
|
|||
| @Test | |||
| public void equalsTrueTest() { | |||
There was a problem hiding this comment.
Would you please ensure this test populates every available field to ensure they are all accounted for in the equals method? Also, could you change the method name to something like equalsWhenAllFieldsEqualThenTrue?
| } | ||
|
|
||
| @Test | ||
| public void hashCodeTest() { |
There was a problem hiding this comment.
Would you please ensure this test populates every available field to ensure they are all accounted for in the hashCode method? Also, could you change the method name to something like hashCodeWhenAllFieldsEqualThenHashCodesAreEqual?
Closes gh-16394