Merged
Conversation
…ng 7 Co-authored-by: jottley <121526+jottley@users.noreply.github.com>
Co-authored-by: jottley <121526+jottley@users.noreply.github.com>
Co-authored-by: jottley <121526+jottley@users.noreply.github.com>
… vulnerability Co-authored-by: jottley <121526+jottley@users.noreply.github.com>
…ion of spring-social dependency) -Update license headers -Remove unneeded resources and classes -Remove uneeded dependencies -Add AccessGrant class
Remove Dependency in Spring Social Clean up code Support both Spring Security and non-Spring Security use cases Update License Headers Start refactoring away from using social in the packaging and naming
Updated project name and links in README.md to reflect the new repository structure and versioning.
There was a problem hiding this comment.
Pull request overview
This pull request migrates the spring-social-salesforce library to Spring 7, removes the deprecated Spring Social dependency, and updates the package namespace from org.springframework.social.salesforce to org.springframework.salesforce. The migration also includes moving to a new Maven repository (Repsy).
Changes:
- Removed Spring Social dependencies and implemented custom OAuth2 client using Spring's RestTemplate
- Updated package namespace throughout the codebase from
org.springframework.social.*toorg.springframework.salesforce.* - Migrated from Spring 6.2.14 to Spring 7.0.0 with updated dependencies (servlet-api, slf4j, logback, commons-lang3)
Reviewed changes
Copilot reviewed 109 out of 109 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated groupId, artifactId, dependencies (Spring 7, Jakarta Servlet 6.0), and Maven repository to Repsy |
| README.md | Updated project name, repository URLs, and Maven dependency instructions |
| src/main/java/org/springframework/salesforce/connect/SalesforceOAuth2Client.java | New OAuth2 client implementation replacing Spring Social OAuth2Template |
| src/main/java/org/springframework/salesforce/connect/SalesforceServiceProvider.java | Refactored to remove Spring Social dependency and use custom OAuth2 client |
| src/main/java/org/springframework/salesforce/connect/SalesforceConnectionFactory.java | New connection factory replacing Spring Social's OAuth2ConnectionFactory |
| src/main/java/org/springframework/salesforce/connect/AccessGrant.java | New data class for OAuth2 access grant response |
| src/main/java/org/springframework/salesforce/connect/SalesforceProfileMapper.java | Renamed from SalesforceAdapter, removed Spring Social ApiAdapter interface |
| src/main/java/org/springframework/salesforce/api/impl/SalesforceTemplate.java | Removed AbstractOAuth2ApiBinding inheritance, implemented custom RestTemplate initialization |
| src/main/java/org/springframework/salesforce/api/impl/SalesforceErrorHandler.java | Updated for Spring 7's new error handler API signature |
| src/main/java/org/springframework/salesforce/api/impl/*Template.java | Updated package imports, replaced URIBuilder with UriComponentsBuilder |
| src/main/java/org/springframework/salesforce/api/*.java | Created new exception classes (ApiException, InvalidAuthorizationException, etc.) replacing Spring Social exceptions |
| src/main/java/org/springframework/salesforce/client/ErrorHandler.java | New error handler with Spring 7 compatibility |
| src/test/java/**/*.java | Updated package imports and test expectations for URL encoding changes |
| Deleted files | Removed Spring Social specific configuration classes (namespace handlers, error handlers, connection classes) |
Comments suppressed due to low confidence (5)
src/main/java/org/springframework/salesforce/api/impl/SObjectsTemplate.java:63
- When dataNode is not null but dataNode.get("sobjects") returns null, this will cause a NullPointerException in api.readList(). Add a null check for dataNode.get("sobjects") before calling api.readList().
src/main/java/org/springframework/salesforce/api/impl/SObjectsTemplate.java:75 - When node is not null but node.get("objectDescribe") returns null, this will cause a NullPointerException in api.readObject(). Add a null check for node.get("objectDescribe") before calling api.readObject().
src/main/java/org/springframework/salesforce/api/impl/AbstractSalesForceOperations.java:40 - The authorization check now only verifies that the API instance is not null, with a TODO comment suggesting token validity should be checked. This is a weaker authorization check than before. Since the Salesforce interface no longer extends ApiBinding (which had an isAuthorized() method), proper authorization validation is missing. Consider implementing a mechanism to verify that the access token is valid and not expired.
src/main/java/org/springframework/salesforce/api/impl/SalesforceTemplate.java:128 - The interceptor list is created locally and set directly, which will overwrite any existing interceptors on the RestTemplate. In the initialize() method (line 270), the ApiRequestInterceptor is added to the existing interceptors list. This creates a race condition where if interceptors are set during initializeRestTemplate(), they will be replaced when initialize() gets the interceptor list and modifies it. Consider consolidating interceptor setup or using addAll() instead of creating a new list.
src/main/java/org/springframework/salesforce/connect/SalesforceServiceProvider.java:78 - The condition checks if instanceUrl is null before setting it, which is backwards logic. If instanceUrl is null, setting it to null has no effect. The condition should be
if (instanceUrl != null)to match the logic in the getApi(String accessToken) method above.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/springframework/salesforce/connect/SalesforceConnectionFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/springframework/salesforce/client/ErrorHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 109 out of 109 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (11)
src/main/java/org/springframework/salesforce/api/impl/SalesforceErrorHandler.java:60
- Missing override annotation. The handleError method overrides DefaultResponseErrorHandler.handleError(ClientHttpResponse) but doesn't have the @OverRide annotation, while the other overload on line 68 does have it. This is inconsistent.
src/main/java/org/springframework/salesforce/api/impl/SalesforceErrorHandler.java:118 - The handleUncategorizedError method is marked as @SuppressWarnings("unused") but is never called in the code. This appears to be dead code that should either be removed or actually used in error handling.
src/main/java/org/springframework/salesforce/api/PickListEntry.java:29 - Inconsistent field modifier usage. While the PickListEntry class changed its fields to be final (which is good), other similar data classes like Photo, ApiVersion, etc. may also benefit from immutability but weren't updated. Consider applying the same pattern consistently across all data transfer objects for better immutability guarantees.
src/main/java/org/springframework/salesforce/api/impl/SalesforceErrorHandler.java:71 - Unused parameter 'response' in handleError overload method. The method signature requires the ClientHttpResponse parameter but it's only used to pass to handleError(response) on line 70. If both overloads always delegate to the same logic, consider whether this complexity is necessary or if the overload with additional parameters (url, method) is actually needed.
src/main/java/org/springframework/salesforce/connect/SalesforceServiceProvider.java:2 - Copyright year includes 2026 which is in the future. Copyright years should reflect actual creation and modification dates, not future dates. The current date context indicates it is January 2026, so using 2026 in the copyright may be premature unless the file was actually modified in 2026.
pom.xml:121 - Missing jspecify dependency in pom.xml. Multiple files throughout the codebase use org.jspecify.annotations.NonNull (CustomApiOperations.java, ApiRequestInterceptor.java, CustomApiTemplate.java, HeaderAddingInterceptor.java, SalesforceErrorHandler.java, and ErrorHandler.java), but the jspecify dependency is not declared in pom.xml. This will cause compilation failures. Add the jspecify dependency or use Spring's @nonnull annotation instead.
<dependencies>
<!-- Core Dependencies -->
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-web</artifactId>
<version>${spring.version}</version>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<version>${spring.version}</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents.client5</groupId>
<artifactId>httpclient5</artifactId>
<version>${httpclient.version}</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
<version>${httpclient.version}</version>
</dependency>
<dependency>
<groupId>jakarta.servlet</groupId>
<artifactId>jakarta.servlet-api</artifactId>
<version>${servlet.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>${jackson.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>${commons-lang3.version}</version>
</dependency>
<dependency>
<groupId>org.jooq</groupId>
<artifactId>joor</artifactId>
<version>${joor.version}</version>
</dependency>
<!-- Logging Dependencies -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>log4j-over-slf4j</artifactId>
<version>${slf4j.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<version>${logback.version}</version>
</dependency>
<!-- Test Dependencies -->
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-library</artifactId>
<version>${hamcrest.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
<version>${spring.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
src/main/java/org/springframework/salesforce/client/BaseSalesforceFactory.java:87
- The comment on line 87 says "Using default request factory - Spring will auto-detect the best available" but this removes the explicit ClientHttpRequestFactorySelector that was used previously. Verify that Spring's auto-detection provides the same behavior and performance characteristics as the previous implementation, especially for connection pooling and timeout configurations.
src/main/java/org/springframework/salesforce/api/impl/SalesforceTemplate.java:112 - The method call to getFormMessageConverter() was removed from the old implementation but there's no FormHttpMessageConverter being created in the initializeRestTemplate method. The FormHttpMessageConverter is now added directly on line 112, which is correct. However, this change removes any custom configuration that may have been in getFormMessageConverter().
src/main/java/org/springframework/salesforce/api/Salesforce.java:2 - The copyright year range 2016-2026 is incorrect. This should be 2017-2026 to match the other files in the codebase, or the actual years when the file was created and last modified.
src/test/java/org/springframework/salesforce/api/impl/QueryTemplateTest.java:40 - URL encoding change from + to %20 for spaces. The test expectations were updated to use %20 instead of + for space encoding, which is correct for query parameters (RFC 3986). However, ensure that the actual UriComponentsBuilder implementation produces %20 encoding and not + encoding, as some URL builders use + for backward compatibility.
src/main/java/org/springframework/salesforce/connect/SalesforceServiceProvider.java:77 - Logic error: The condition checks if instanceUrl is null, then sets it. This should be checking if instanceUrl is NOT null before setting it. Currently, when instanceUrl is null, the template's instanceUrl is set to null, which is redundant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+56
| @JsonProperty("access_token") | ||
| private String access_token; | ||
|
|
||
| @JsonProperty("refresh_token") | ||
| private String refresh_token; | ||
|
|
||
| @JsonProperty("signature") | ||
| private String signature; | ||
|
|
||
| @JsonProperty("scope") | ||
| private String scope; | ||
|
|
||
| @JsonProperty("id_token") | ||
| private String id_token; | ||
|
|
||
| @JsonProperty("instance_url") | ||
| private String instance_url; | ||
|
|
||
| @JsonProperty("id") | ||
| private String id; | ||
|
|
||
| @JsonProperty("token_type") | ||
| private String token_type; | ||
|
|
||
| @JsonProperty("issued_at") | ||
| private String issued_at; |
There was a problem hiding this comment.
Inconsistent naming convention: field names use snake_case (access_token, refresh_token, etc.) while this is a Java class where camelCase is the standard convention. While the @JsonProperty annotations correctly map to the JSON snake_case format, the actual Java field names should follow Java naming conventions (accessToken, refreshToken, etc.). This makes the code less idiomatic.
Comment on lines
+47
to
+76
| public void handleError(@NonNull ClientHttpResponse response) throws IOException { | ||
| if (response.getStatusCode().equals(HttpStatus.BAD_REQUEST)) { | ||
| //TODO move to a switch | ||
| Map<String, String> error = extractErrorDetailsFromResponse(response); | ||
| if ("unsupported_response_type".equals(error.get(ERROR))) { | ||
| throw new OperationNotPermittedException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("invalid_client_id".equals(error.get(ERROR))) { | ||
| throw new InvalidAuthorizationException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("invalid_request".equals(error.get(ERROR))) { | ||
| throw new OperationNotPermittedException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("invalid_client_credentials".equals(error.get(ERROR))) { | ||
| throw new InvalidAuthorizationException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("invalid_grant".equals(error.get(ERROR))) { | ||
| if ("invalid user credentials".equals(error.get(ERROR_DESCRIPTION))) { | ||
| throw new InvalidAuthorizationException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("IP restricted or invalid login hours".equals(error.get(ERROR_DESCRIPTION))) { | ||
| throw new OperationNotPermittedException(error.get(ERROR_DESCRIPTION)); | ||
| } | ||
| throw new InvalidAuthorizationException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("inactive_user".equals(error.get(ERROR))) { | ||
| throw new OperationNotPermittedException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("inactive_org".equals(error.get(ERROR))) { | ||
| throw new OperationNotPermittedException(error.get(ERROR_DESCRIPTION)); | ||
| } else if ("rate_limit_exceeded".equals(error.get(ERROR))) { | ||
| throw new RateLimitExceededException("Rate limit exceeded"); | ||
| } else if ("invalid_scope".equals(error.get(ERROR))) { | ||
| throw new InvalidAuthorizationException(error.get(ERROR_DESCRIPTION)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing override annotation on handleError method which overrides the parent class method from DefaultResponseErrorHandler. The overload method on lines 78-87 has the @OverRide annotation but this one doesn't, creating inconsistency.
Owner
Author
There was a problem hiding this comment.
@copilot This is not a method in the parent class, so an override would not be meaningful.
Comment on lines
+78
to
+87
| @Override | ||
| public void handleError(@NonNull URI url, @NonNull HttpMethod method, @NonNull ClientHttpResponse response) throws IOException { | ||
| if (response.getStatusCode().equals(HttpStatus.BAD_REQUEST)) { | ||
| handleError(response); | ||
| // if not handled, fall through to default handling | ||
| super.handleError(url, method, response); | ||
| return; | ||
| } | ||
| super.handleError(url, method, response); | ||
| } |
There was a problem hiding this comment.
The handleError implementation on lines 78-87 always calls super.handleError after handling BAD_REQUEST. This means BAD_REQUEST errors will be processed twice - once by the custom handler logic and then by the parent class. The logic should return after handling BAD_REQUEST to avoid double processing.
Comment on lines
+82
to
+96
| public String getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public String getTokenType() { | ||
| return token_type; | ||
| } | ||
|
|
||
| public void setTokenType(String tokenType) { | ||
| this.token_type = tokenType; | ||
| } | ||
|
|
||
| public String getIssuedAt() { | ||
| return issued_at; | ||
| } |
There was a problem hiding this comment.
Missing setter methods for id and issued_at fields. The AccessGrant class has getter methods for these fields (getId() on line 82 and getIssuedAt() on line 94) but no corresponding setter methods, while other fields like access_token and token_type have both getters and setters. This inconsistency may prevent proper deserialization of these fields from JSON.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
-Remove dependency on Spring Social
-Update package namespace
-Move to new maven repo