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

Add Equals and HashCode methods for better comparison. #16842

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -188,6 +188,71 @@ public static Builder authorizationCode() {
return new Builder(AuthorizationGrantType.AUTHORIZATION_CODE);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null || this.getClass() != obj.getClass()) {
return false;
}
OAuth2AuthorizationRequest that = (OAuth2AuthorizationRequest) obj;

if (!this.authorizationUri.equals(that.authorizationUri)) {
Copy link
Member

Choose a reason for hiding this comment

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

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(...)
    ...

return false;
}

if (!this.authorizationGrantType.equals(that.authorizationGrantType)) {
return false;
}

if (this.responseType != that.responseType) {
return false;
}

if (!this.clientId.equals(that.clientId)) {
return false;
}

if (!this.redirectUri.equals(that.redirectUri)) {
return false;
}

if (!this.scopes.equals(that.scopes)) {
return false;
}

if (!this.state.equals(that.state)) {
return false;
}

if (!this.additionalParameters.equals(that.additionalParameters)) {
return false;
}

if (!this.authorizationRequestUri.equals(that.authorizationRequestUri)) {
return false;
}

return this.attributes.equals(that.attributes);
}

@Override
public int hashCode() {
int result = this.authorizationUri.hashCode();
result = 31 * result + this.clientId.hashCode();
Copy link
Member

Choose a reason for hiding this comment

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

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?

result = 31 * result + ((this.authorizationGrantType == null) ? 0 : this.authorizationGrantType.hashCode());
result = 31 * result + ((this.responseType == null) ? 0 : this.responseType.hashCode());
result = 31 * result + ((this.redirectUri == null) ? 0 : this.redirectUri.hashCode());
result = 31 * result + ((this.scopes == null) ? 0 : this.scopes.hashCode());
result = 31 * result + ((this.state == null) ? 0 : this.state.hashCode());
result = 31 * result + ((this.additionalParameters == null) ? 0 : this.additionalParameters.hashCode());
result = 31 * result + ((this.authorizationRequestUri == null) ? 0 : this.authorizationRequestUri.hashCode());
result = 31 * result + ((this.attributes == null) ? 0 : this.attributes.hashCode());

return result;
}

/**
* Returns a new {@link Builder}, initialized with the values from the provided
* {@code authorizationRequest}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.net.URI;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
Expand All @@ -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.*;
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend updating import settings for your IDE to not collapse imports, as this style of import will fail the build.


/**
* Tests for {@link OAuth2AuthorizationRequest}.
Expand Down Expand Up @@ -365,4 +365,45 @@ public void buildWhenAdditionalParametersContainsNullThenAuthorizationRequestUri
+ "item1=null&item2=value2");
}

@Test
public void equalsTrueTest() {
Copy link
Member

Choose a reason for hiding this comment

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

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?

OAuth2AuthorizationRequest authorizationRequest1 = TestOAuth2AuthorizationRequests.request()
.authorizationRequestUri("http://example.com")
.additionalParameters(Collections.singletonMap("someAdditionalParameterKey", "someAdditionalParameterValue"))
.parameters(parametersMap -> parametersMap.put("someParameterKey", "someParameterValue"))
.scope("someScope")
.build();

OAuth2AuthorizationRequest authorizationRequest2 = TestOAuth2AuthorizationRequests.request()
.authorizationRequestUri("http://example.com")
.additionalParameters(Collections.singletonMap("someAdditionalParameterKey", "someAdditionalParameterValue"))
.parameters(parametersMap -> parametersMap.put("someParameterKey", "someParameterValue"))
.scope("someScope")
.build();

assertThat(authorizationRequest1).isEqualTo(authorizationRequest2);
}

@Test
public void hashCodeTest() {
Copy link
Member

Choose a reason for hiding this comment

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

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?

OAuth2AuthorizationRequest authorizationRequest1 = TestOAuth2AuthorizationRequests.request()
.authorizationRequestUri("http://example.com")
.additionalParameters(Collections.singletonMap("someAdditionalParameterKey", "someAdditionalParameterValue"))
.parameters(parametersMap -> parametersMap.put("someParameterKey", "someParameterValue"))
.scope("someScope")
.build();

OAuth2AuthorizationRequest authorizationRequest2 = TestOAuth2AuthorizationRequests.request()
.authorizationRequestUri("http://example.com")
.additionalParameters(Collections.singletonMap("someAdditionalParameterKey", "someAdditionalParameterValue"))
.parameters(parametersMap -> parametersMap.put("someParameterKey", "someParameterValue"))
.scope("someScope")
.build();

int authorizationRequest1HashCode = authorizationRequest1.hashCode();
int authorizationRequest2HashCode = authorizationRequest2.hashCode();

assertThat(authorizationRequest1HashCode).isEqualTo(authorizationRequest2HashCode);
}

}