added code formatter

[kreloaded]

Summary by CodeRabbit

• Refactor: Improved code readability and maintainability by reformatting and adjusting indentation across multiple classes.
• New Feature: Added CORS configuration for localhost:3000, allowing various HTTP methods from the specified origin.
• Bug Fix: Fixed the Status enum in SalesforceOauthToken.java to use a static map for value lookup, improving performance.
• Refactor: Updated DynamoDBConfiguration.java to use builder classes for creating AmazonDynamoDB and DynamoDBMapperConfig objects.
• New Feature: Introduced SameSiteCookieFilter class to set the SameSite attribute for cookies in HTTP responses.
• Documentation: Updated local setup instructions in LOCAL_SETUP.md, providing updated environment variable values and formatting guidelines.

[coderabbitai]

Walkthrough

The changes primarily focus on code formatting and readability improvements across various classes in the project. Some notable updates include adding a new dependency and plugin to the Maven configuration, CORS configuration for localhost:3000, and modifications to methods in the DatabaseChangelog class. Additionally, there are enhancements in error handling, security headers, and request interceptors.

Changes

File(s)	Summary
pom.xml	Added io.spring.javaformat:spring-javaformat-maven-plugin dependency and plugin.
src/main/java/com/salessparrow/api/config/...	Improved code formatting and indentation across multiple configuration classes.
src/main/java/com/salessparrow/api/controllers/...	Formatting adjustments in various controller classes. Notable change in AccountController to return a ResponseEntity.
src/main/java/com/salessparrow/api/domain/...	Code reformatting in domain classes. Addition of a new method getId(String externalUserId) in SalesforceUser.
src/main/java/com/salessparrow/api/dto/...	Minor formatting changes across DTO classes.
src/main/java/com/salessparrow/api/exception/...	Refactoring in exception handling classes for better readability.
src/main/java/com/salessparrow/api/filter/SameSiteCookieFilter.java	Introduced a new filter to set the SameSite attribute for cookies.
src/main/java/com/salessparrow/api/interceptors/...	Code reformatting and minor changes in interceptor classes.
src/main/java/com/salessparrow/api/lib/...	Various formatting and readability improvements in library classes.
repo-docs/LOCAL_SETUP.md	Updated setup instructions and environment variable values.

"In the land of code, where logic is king,
A rabbit hopped by, making everything sing.
With each line he touched, clarity did bring,
Now the code dances, and developers can swing! 🐇💻🎉"

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance. The bot stays silent unless called upon.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

[coderabbitai]

In the parseResponse method, you are creating a new instance of Util every time the method is called. If Util is stateless, consider making its methods static or autowiring a single instance of Util to improve performance.

- Util util = new Util();
+ @Autowired
+ private Util util;

[coderabbitai]

The getTaskSubjectFromDescription method is correctly implemented. It checks if the description length is less than 60 characters and returns the appropriate string. However, it would be better to handle null descriptions to avoid NullPointerExceptions.

- if (task.getDescription().length() < 60) {
+ if (task.getDescription() != null && task.getDescription().length() < 60) {

[coderabbitai]

The deleteAccountTask method creates a new list of CompositeRequestDto for each call. If this method is called frequently, it could lead to unnecessary memory allocation and garbage collection. Consider reusing the list if possible or using a pool of lists.

- List<CompositeRequestDto> compositeRequests = new ArrayList<CompositeRequestDto>();
+ List<CompositeRequestDto> compositeRequests = getCompositeRequestList();

In the above code, getCompositeRequestList() should be a method that manages a pool of CompositeRequestDto lists.

[coderabbitai]

The parseResponse method creates a new Util object for each call. If this method is called frequently, it could lead to unnecessary memory allocation and garbage collection. Consider making the Util class methods static or creating a single instance of Util and reusing it.

- Util util = new Util();
+ Util util = getUtilInstance();

In the above code, getUtilInstance() should be a method that returns a singleton instance of Util.

[coderabbitai]

The method notesResponse is making a POST request using makeCompositeRequest.makePostRequest. However, the HTTP method specified in noteCompositeRequest is GET. This could lead to unexpected behavior. Please ensure that the correct HTTP method is used for this request.

- HttpClient.HttpResponse response = makeCompositeRequest.makePostRequest(compositeRequests, salesforceUserId);
+ HttpClient.HttpResponse response = makeCompositeRequest.makeGetRequest(compositeRequests, salesforceUserId);

[coderabbitai]

In the parseResponse method, the ObjectMapper instance is created inside the loop which is not efficient. It's better to create it once outside the loop and reuse it. Also, disabling DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES can be done at the time of creation.

- for (JsonNode recordNode : recordsNode) {
-    ObjectMapper mapper = new ObjectMapper();
-    mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
-    SalesforceGetNoteDetailsDto salesforceGetNotesList = mapper.convertValue(recordNode,
-            SalesforceGetNoteDetailsDto.class);
-    noteDetailEntity = salesforceGetNotesList.noteDetailEntity(noteContentResponse);
- }

+ ObjectMapper mapper = new ObjectMapper();
+ mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ for (JsonNode recordNode : recordsNode) {
+    SalesforceGetNoteDetailsDto salesforceGetNotesList = mapper.convertValue(recordNode,
+            SalesforceGetNoteDetailsDto.class);
+    noteDetailEntity = salesforceGetNotesList.noteDetailEntity(noteContentResponse);
+ }

[coderabbitai]

The getAccounts method constructs a Salesforce query, sends a GET request to the Salesforce API, and parses the response. The HTTP method is hardcoded as "GET" in the CompositeRequestDto object creation. Consider making this a constant or configurable if it's likely to change or be reused elsewhere.

[coderabbitai]

In the parseResponse method, an ObjectMapper instance is created for each record in the loop (line 86). This can be inefficient as ObjectMapper is thread-safe and can be reused. Consider moving the ObjectMapper instantiation outside of the loop.

-        for (JsonNode recordNode : recordsNode) {
-            ObjectMapper mapper = new ObjectMapper();
-            mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
-            SalesforceAccountDto salesforceAccount = mapper.convertValue(recordNode, SalesforceAccountDto.class);
-            AccountEntity accountEntity = salesforceAccount.getAccountEntity();
-
-            accountIds.add(accountEntity.getId());
-            accountIdToEntityMap.put(accountEntity.getId(), accountEntity);
-        }
+        ObjectMapper mapper = new ObjectMapper();
+        mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+        for (JsonNode recordNode : recordsNode) {
+            SalesforceAccountDto salesforceAccount = mapper.convertValue(recordNode, SalesforceAccountDto.class);
+            AccountEntity accountEntity = salesforceAccount.getAccountEntity();
+
+            accountIds.add(accountEntity.getId());
+            accountIdToEntityMap.put(accountEntity.getId(), accountEntity);
+        }

[coderabbitai]

The toString() method has been reformatted for better readability. However, the string concatenation operation in this method could be improved by using StringBuilder or String.format(), which are more efficient and readable.

@Override
public String toString() {
    return String.format("ErrorInfo{http_code='%s', code='%s', message='%s'}", httpCode, code, message);
}

[coderabbitai]

In the parseResponse method, an instance of ObjectMapper is created inside the loop that iterates over the recordsNode. Creating a new ObjectMapper for each iteration can be inefficient and may impact performance. Consider moving the creation of the ObjectMapper outside of the loop.

- for (JsonNode recordNode : recordsNode) {
-     ObjectMapper mapper = new ObjectMapper();
-     mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
-     SalesforceGetTasksListDto salesforceGetTasksListDto = mapper.convertValue(recordNode,
-             SalesforceGetTasksListDto.class);
-     TaskEntity taskEntity = salesforceGetTasksListDto.taskEntity();
- 
-     taskIds.add(taskEntity.getId());
-     taskIdToEntityMap.put(taskEntity.getId(), taskEntity);
- }

+ ObjectMapper mapper = new ObjectMapper();
+ mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ for (JsonNode recordNode : recordsNode) {
+     SalesforceGetTasksListDto salesforceGetTasksListDto = mapper.convertValue(recordNode,
+             SalesforceGetTasksListDto.class);
+     TaskEntity taskEntity = salesforceGetTasksListDto.taskEntity();
+ 
+     taskIds.add(taskEntity.getId());
+     taskIdToEntityMap.put(taskEntity.getId(), taskEntity);
+ }

[coderabbitai]

The LATEST_VERSION constant is set to "1". If this value is expected to change over time, consider moving it to a configuration file or environment variable to avoid hardcoding.

[coderabbitai]

The USER_LOGIN_COOKIE_NAME is hardcoded as "ulcn". Consider moving this to a configuration file or environment variable for flexibility and security reasons.

[coderabbitai]

The USER_LOGIN_COOKIE_EXPIRY_IN_SEC is hardcoded as 30 * 60 * 60 * 24 (equivalent to 30 days). If this value is expected to change based on different environments or requirements, consider moving it to a configuration file or environment variable.

[coderabbitai]

The SalesSparrowPrefix constant is public and static but not final, which means it can be modified. It's a good practice to make such constants immutable by declaring them as final.

-	public static String SalesSparrowPrefix = "ss_";
+	public static final String SalesSparrowPrefix = "ss_";

[coderabbitai]

The comments explaining the calculation for cache expiration are split across two lines. This could lead to confusion or misinterpretation. It would be better to keep the comment on the same line as the code it refers to.

-	public static final Integer SS_SALESFORCE_USER_CACHE_EXP = 30 * 24 * 60 * 60; // 30
-																					// days
+	public static final Integer SS_SALESFORCE_USER_CACHE_EXP = 30 * 24 * 60 * 60; // 30 days

-	public static final Integer SS_SALESFORCE_OAUTH_TOKEN_CACHE_EXP = 30 * 24 * 60 * 60; // 30
-																							// days
+	public static final Integer SS_SALESFORCE_OAUTH_TOKEN_CACHE_EXP = 30 * 24 * 60 * 60; // 30 days

[coderabbitai]

This block of code parses the response from Salesforce. It assumes that the response will always have certain fields (compositeResponse, httpStatusCode, body, records). If any of these fields are missing in the response, it could lead to a NullPointerException. Consider adding null checks or try-catch blocks to handle potential exceptions gracefully.

Overall, the changes look good and improve the readability and maintainability of the codebase.

[coderabbitai]

The URL "https://api.openai.com/v1/chat/completions" is hardcoded. It would be better to move this to a configuration file or environment variable, so it can be changed without modifying the code.

-	public String chatCompletionUrl() {
-		return "https://api.openai.com/v1/chat/completions";
-	}
+	public String chatCompletionUrl() {
+		return System.getenv("CHAT_COMPLETION_URL");
+	}

[coderabbitai]

The timeout value of 20000 milliseconds is also hardcoded. This should also be moved to a configuration file or environment variable for flexibility.

-	public Integer timeoutMillis() {
-		return 20000;
-	}
+	public Integer timeoutMillis() {
+		return Integer.parseInt(System.getenv("TIMEOUT_MILLIS"));
+	}

[coderabbitai]

The WebClient instance is being created for each request. This is not efficient as the creation of WebClient is expensive and it's designed to be reused across multiple requests. Consider creating a single WebClient instance and reusing it for all requests.

- WebClient webClient = WebClient.builder().build();
+ private static final WebClient webClient = WebClient.builder().build();

[coderabbitai]

Similar to the above comment, the WebClient instance should be reused here as well.

- WebClient webClient = WebClient.builder().build();
+ // Use the existing WebClient instance

[coderabbitai]

The class name dynamoBeeConfigConstants does not follow Java's naming conventions. Class names should be in PascalCase. Consider renaming it to DynamoBeeConfigConstants.

- public class dynamoBeeConfigConstants {
+ public class DynamoBeeConfigConstants {

[coderabbitai]

These methods are returning hard-coded strings and string concatenations directly. This can make the code harder to maintain if these values need to change in the future. Consider defining these as constants at the top of your class.

+ private static final String CHANGELOG_SCAN_PACKAGE = "com.salessparrow.api.changelogs";
+ private static final String CHANGELOG_TABLE_NAME_SUFFIX = "_changelog";
+ private static final String AUTHOR_NAME_SUFFIX = "_salessparrow_api";

- public String getChangeLogScanPackage() {
-     return "com.salessparrow.api.changelogs";
- }
+ public String getChangeLogScanPackage() {
+     return CHANGELOG_SCAN_PACKAGE;
+ }

- public String getChangelogTableName() {
-     return CoreConstants.environment() + "_changelog";
- }
+ public String getChangelogTableName() {
+     return CoreConstants.environment() + CHANGELOG_TABLE_NAME_SUFFIX;
+ }

- public String authorName() {
-     return CoreConstants.environment() + "_salessparrow_api";
- }
+ public String authorName() {
+     return CoreConstants.environment() + AUTHOR_NAME_SUFFIX;
+ }

[coderabbitai]

The logger is currently using SuggestionsController.class for its name, which seems incorrect as this is the OpenAiRequest class. This could lead to confusion when reading the logs. The logger should be named after the class it's used in.

- private Logger logger = org.slf4j.LoggerFactory.getLogger(SuggestionsController.class);
+ private Logger logger = org.slf4j.LoggerFactory.getLogger(OpenAiRequest.class);

[coderabbitai]

The method makeRequest(Object payload) does not validate the input payload before making the request. It's a good practice to validate inputs before processing them further. Consider adding a validation step for the payload before making the request to the OpenAI API. If the payload is invalid, you can throw an appropriate exception.

[coderabbitai]

The error messages thrown in the exceptions are generic ("something_went_wrong"). It would be more helpful to provide specific error messages that indicate what exactly went wrong. This would make debugging easier and improve the user experience.

[coderabbitai]

The method payloadForCrmActionsSuggestions constructs a JSON string manually, which is error-prone and hard to maintain. Consider using a library like Jackson or Gson to create the JSON object. This will make your code more readable and less prone to errors.

public String payloadForCrmActionsSuggestions(String text) {
    JsonObject payload = new JsonObject();
    payload.addProperty("model", "gpt-3.5-turbo-0613");

    JsonArray messages = new JsonArray();
    JsonObject message = new JsonObject();
    message.addProperty("role", "user");
    message.addProperty("content", "You are an AI assistant which gives suggestion on creating task in crm using the input message.Only use the functions you have been provided with. \\nInput message: \\n" + text + "\\n");
    messages.add(message);
    payload.add("messages", messages);

    // Continue building the rest of the JSON object...
    
    return payload.toString();
}

[coderabbitai]

This method uses SimpleDateFormat which is not thread-safe. If multiple threads access this method simultaneously, it could lead to incorrect results. Consider using DateTimeFormatter from the Java 8 Date/Time API which is thread-safe.

public String getTodaysDate() {
    DateTimeFormatter dtf = DateTimeFormatter.ofPattern("yyyy-MM-dd");
    return dtf.format(LocalDate.now());
}

[coderabbitai]

The method getSalesforceOrganizationId() assumes that the id will always have at least 5 parts when split by "/". If this is not the case, an ArrayIndexOutOfBoundsException will be thrown. Consider adding a check to ensure that the array has enough elements before accessing them.

public String getSalesforceOrganizationId() {
    String[] idParts = id.split("/");
+   if (idParts.length < 5) {
+       return null; // or throw an appropriate exception
+   }
    return idParts[4];
}

[coderabbitai]

The getSecret() method is doing too much work. It's responsible for determining the environment, reading local files, fetching secrets from AWS, parsing JSON, and extracting specific values. Consider breaking this method down into smaller, more focused methods to improve readability and maintainability.

[coderabbitai]

The SecretCache object is created and closed within each call to getSecret(). If getSecret() is called frequently, this could lead to performance issues due to the overhead of creating and closing the SecretCache repeatedly. Consider reusing the SecretCache across multiple calls to getSecret().

[coderabbitai]

The exception handling here is too broad, catching all exceptions and wrapping them in a CustomException. This could make debugging difficult because it hides the original type of the exception. Consider catching more specific exception types (like IOException, JsonProcessingException) and handling them appropriately.

[coderabbitai]

The getLocalEnvVars() method reads the entire file into a string using a FileReader and a loop. This can be simplified and made more efficient by using Files.readString(Path).

- try (FileReader fileReader = new FileReader(filename)) {
-     int ch;
-     StringBuilder secretJsonBuilder = new StringBuilder();
-     while ((ch = fileReader.read()) != -1) {
-         secretJsonBuilder.append((char) ch);
-     }
-     return secretJsonBuilder.toString();
- } catch (FileNotFoundException e) {
-     throw new CustomException(new ErrorObject("l_gc_s_glev_1", "something_went_wrong", e.getMessage()));
- } catch (IOException e) {
-     throw new CustomException(new ErrorObject("l_gc_s_glev_2", "something_went_wrong", e.getMessage()));
- }
+ try {
+     return Files.readString(Paths.get(filename));
+ } catch (IOException e) {
+     throw new CustomException(new ErrorObject("l_gc_s_glev_2", "something_went_wrong", e.getMessage()));
+ }

[coderabbitai]

Consider extracting the magic number 10000 into a constant with a meaningful name. This will make the code more readable and easier to maintain.

- response = HttpClient.makeGetRequest(salesforceIdentityEndpoint, headers, 10000);
+ private static final int TIMEOUT = 10000;
+ response = HttpClient.makeGetRequest(salesforceIdentityEndpoint, headers, TIMEOUT);

[coderabbitai]

The Util object is being created every time the taskEntity() method is called. This could be a performance issue if this method is called frequently. Consider making Util a class-level field and reusing it, or make the getDateFormatFromDatetime method static if it doesn't depend on any instance state.

- public TaskEntity taskEntity() {
-    Util util = new Util();
+ private final Util util = new Util();
+ 
+ public TaskEntity taskEntity() {

    TaskEntity taskEntity = new TaskEntity();
    taskEntity.setId(this.id);
    taskEntity.setCreatorName(this.createdBy.name);
    taskEntity.setDescription(this.description);
    taskEntity.setCrmOrganizationUserName(this.owner.name);
    taskEntity.setLastModifiedTime(this.lastModifiedDate);

    String dueDate = util.getDateFormatFromDatetime(this.activityDate);

    taskEntity.setDueDate(dueDate);
    return taskEntity;
}

[coderabbitai]

There is no error handling in the getNoteContent method. If the makeRequest or makeGetRequest methods throw an exception, it will propagate up the call stack. It would be better to catch any exceptions here and handle them appropriately, possibly by logging an error message and returning a suitable response.

try {
    HttpClient.HttpResponse response = salesforceOauthRequest.makeRequest(salesforceUserId, request);
    return response;
} catch (Exception e) {
    // Log error message and return suitable response
}

[coderabbitai]

Consider using StringBuilder or String.format() for constructing the requestBody. This would improve readability and performance, especially when dealing with large strings.

String requestBody = String.format("grant_type=%s&client_id=%s&client_secret=%s&refresh_token=%s",
    salesforceConstants.refreshTokenGrantType(),
    CoreConstants.salesforceClientId(),
    CoreConstants.salesforceClientSecret(),
    decryptedRefreshToken);

[coderabbitai]

The content type "application/x-www-form-urlencoded" is used frequently in HTTP requests. Consider defining it as a constant to avoid potential typos and enhance maintainability.

public static final String CONTENT_TYPE_FORM_URLENCODED = "application/x-www-form-urlencoded";
...
headers.put("content-type", CONTENT_TYPE_FORM_URLENCODED);

[coderabbitai]

The timeout value 5000 is hardcoded. It's better to define it as a configurable property or at least as a constant at the top of your class. This will make it easier to adjust the timeout value if needed without having to search through the code.

public static final int TIMEOUT = 5000;
...
HttpClient.HttpResponse response = HttpClient.makePostRequest(url, headers, requestBody, TIMEOUT);

[coderabbitai]

The ALLOWED_URIS variable is not final, but it's used like a constant. It would be better to make this variable final to prevent accidental modification.

- private List<String> ALLOWED_URIS = new ArrayList<>(Arrays.asList(CoreConstants.getWhitelistedRedirectUris()));
+ private final List<String> ALLOWED_URIS = new ArrayList<>(Arrays.asList(CoreConstants.getWhitelistedRedirectUris()));

[coderabbitai]

The isValid method checks if the value is in the ALLOWED_URIS list. This operation can be costly if the list is large because it needs to iterate over all elements. Consider using a HashSet instead of a List for ALLOWED_URIS to improve performance as lookup operations in a HashSet are faster (O(1) complexity).

- private final List<String> ALLOWED_URIS = new ArrayList<>(Arrays.asList(CoreConstants.getWhitelistedRedirectUris()));
+ private final Set<String> ALLOWED_URIS = new HashSet<>(Arrays.asList(CoreConstants.getWhitelistedRedirectUris()));

[coderabbitai]

The makeRequest method has been updated to handle a WebClientResponseException and refresh the access token if the status code is 401. However, there's a potential issue here. If the second request (line 43) also fails with a 401 status code, the method will not attempt to refresh the token again and will instead throw an exception. This could lead to unnecessary failures if the first token refresh didn't work for some reason (e.g., temporary network issue). Consider adding a retry mechanism to handle this scenario.

		try {
			return executeRequest(request, sfOAuthToken);
		}
		catch (WebClientResponseException e) {
			if (e.getStatusCode().value() == 401) {
				for (int i = 0; i < MAX_RETRIES; i++) {
					try {
						decryptedAccessToken = getAccessTokenService.updateAndGetRefreshedAccessToken(sfOAuthToken);
						return executeRequest(request, sfOAuthToken);
					}
					catch (WebClientResponseException e1) {
						if (e1.getStatusCode().value() != 401 || i == MAX_RETRIES - 1) {
							throw new CustomException(new ErrorObject("l_s_h_sr_mr_1", "something_went_wrong", e.getMessage()));
						}
					}
				}
			}
			throw new CustomException(new ErrorObject("l_s_h_sr_mr_2", "something_went_wrong", e.getMessage()));
		}
	}

	private <T> T executeRequest(SalesforceRequestInterface<T> request, SalesforceOauthToken sfOAuthToken) {
		return request.execute(decryptedAccessToken, sfOAuthToken.getInstanceUrl());
	}

In this suggested change, I've extracted the request execution into a separate method (executeRequest) for better readability and maintainability. The retry mechanism is implemented in a loop that attempts to refresh the token and execute the request up to MAX_RETRIES times. If all retries fail with a 401 status code, or if any other exception occurs, a CustomException is thrown.

[coderabbitai]

The construction of the request body is done by concatenating strings, which can be inefficient and error-prone. Consider using StringBuilder or String.format() for better performance and readability. Also, it's important to URL encode the parameters to ensure that special characters are correctly interpreted.

- String requestBody = "grant_type=" + salesforceConstants.authorizationCodeGrantType() + "&client_id="
- 				+ CoreConstants.salesforceClientId() + "&client_secret=" + CoreConstants.salesforceClientSecret()
- 				+ "&code=" + code + "&redirect_uri=" + redirectUri;
+ String requestBody = String.format("grant_type=%s&client_id=%s&client_secret=%s&code=%s&redirect_uri=%s",
+               URLEncoder.encode(salesforceConstants.authorizationCodeGrantType(), StandardCharsets.UTF_8),
+               URLEncoder.encode(CoreConstants.salesforceClientId(), StandardCharsets.UTF_8),
+               URLEncoder.encode(CoreConstants.salesforceClientSecret(), StandardCharsets.UTF_8),
+               URLEncoder.encode(code, StandardCharsets.UTF_8),
+               URLEncoder.encode(redirectUri, StandardCharsets.UTF_8));

[coderabbitai]

The timeout value for the HTTP request is hardcoded as 10000. It would be better to move this to a configuration file or constant so that it can be easily adjusted without modifying the code.

- response = HttpClient.makePostRequest(salesforceOAuthEndpoint, headers, requestBody, 10000);
+ response = HttpClient.makePostRequest(salesforceOAuthEndpoint, headers, requestBody, CoreConstants.httpTimeout());

[coderabbitai]

The exception handling here only covers generic exceptions. It would be beneficial to handle specific exceptions (like IOException, HttpException) separately to provide more detailed error messages and potentially recover from certain errors.

try {
    response = HttpClient.makePostRequest(salesforceOAuthEndpoint, headers, requestBody, CoreConstants.httpTimeout());
} catch (IOException e) {
    throw new CustomException(new ParamErrorObject("l_s_w_sgt_gt_io", e.getMessage(), Collections.singletonList("network_error")));
} catch (HttpException e) {
    throw new CustomException(new ParamErrorObject("l_s_w_sgt_gt_http", e.getMessage(), Collections.singletonList("http_error")));
} catch (Exception e) {
    throw new CustomException(new ParamErrorObject("l_s_w_sgt_gt_1", e.getMessage(), Collections.singletonList("invalid_code")));
}

[coderabbitai]

The check for an empty string should use .equals("") instead of ==. The == operator checks if two references point to the same object, while .equals() checks if the contents of the strings are equal.

- if (searchTerm == "") {
+ if (searchTerm.equals("")) {

[coderabbitai]

Same issue as above. Use .equals("") instead of ==.

- if (searchTerm == "") {
+ if (searchTerm.equals("")) {

[coderabbitai]

Consider using Java 8's String.join() method to concatenate the document IDs. This will make the code more readable and maintainable.

- StringBuilder queryBuilder = new StringBuilder(
-    "SELECT Id, Title, TextPreview, CreatedBy.Name, LastModifiedDate FROM ContentNote WHERE Id IN (");
- for (int i = 0; i < documentIds.size(); i++) {
-     if (i > 0) {
-         queryBuilder.append(", ");
-     }
-     String documentId = Util.escapeSpecialChars(documentIds.get(i));
-     queryBuilder.append("'").append(documentId).append("'");
- }
- queryBuilder.append(") ORDER BY LastModifiedDate DESC LIMIT 5");
+ List<String> escapedDocumentIds = documentIds.stream()
+     .map(id -> "'" + Util.escapeSpecialChars(id) + "'")
+     .collect(Collectors.toList());
+ String query = String.format(
+     "SELECT Id, Title, TextPreview, CreatedBy.Name, LastModifiedDate FROM ContentNote WHERE Id IN (%s) ORDER BY LastModifiedDate DESC LIMIT 5",
+     String.join(", ", escapedDocumentIds));

[coderabbitai]

The exception handling in this code is too broad. Catching Exception will catch all exceptions, including those that you might not expect and want to handle differently. It's better to catch specific exceptions that you expect can be thrown in the try block. In this case, it would be ParseException. This way, unexpected exceptions won't be silently ignored but will cause a meaningful crash that you can fix.

-		catch (Exception ex) {
+		catch (ParseException ex) {

[coderabbitai]

There's no need to call toString() on value because it's already a string.

-			return dateString.equals(value.toString());
+			return dateString.equals(value);

[coderabbitai]

The makePostRequest method could be improved by handling potential exceptions that might occur during the execution of the HTTP request. Currently, if an exception occurs (for example, a timeout or a network error), it will propagate up the call stack and potentially crash the application. It would be better to catch these exceptions and handle them gracefully, perhaps by logging an error message and returning a suitable error response.

Here's how you can do this:

  public HttpClient.HttpResponse makePostRequest(List<CompositeRequestDto> compositeRequests,
      String salesforceUserId) {
    Map<String, List<CompositeRequestDto>> compositeRequestsMap = new HashMap<>();
    compositeRequestsMap.put("compositeRequest", compositeRequests);

    Integer timeoutMillis = salesforceConstants.timeoutMillis();

    SalesforceRequestInterface<HttpClient.HttpResponse> request = (token, instanceUrl) -> {
      String httpReqUrl = salesforceConstants.salesforceCompositeUrl(instanceUrl);
      
      Map<String, String> headers = new HashMap<>();
      headers.put("Authorization", "Bearer " + token);

      HttpClient.HttpResponse response = null;
      try {
        response = HttpClient.makePostRequest(
            httpReqUrl,
            headers,
            compositeRequestsMap,
            timeoutMillis);
      } catch (Exception e) {
        logger.error("Error occurred while making composite request to Salesforce", e);
        // return a suitable error response or rethrow the exception
      }
      return response;
    };

    HttpClient.HttpResponse response = null;

    logger.info("making composite request to salesforce");
    
    try {
      response = salesforceOauthRequest.makeRequest(salesforceUserId, request);
    } catch (Exception e) {
      logger.error("Error occurred while making request to Salesforce", e);
      // return a suitable error response or rethrow the exception
    }
    return response;
  }

[coderabbitai]

The class MakeCompositeRequest is not thread-safe because it uses instance variables (salesforceOauthRequest, salesforceConstants) that are shared among multiple threads. If two threads call the makePostRequest method at the same time, they may interfere with each other's operations, leading to inconsistent results or errors. To fix this, consider making the makePostRequest method synchronized or using local variables instead of instance variables where possible.

[coderabbitai]

The method deleteAccountNote takes an accountId parameter but it's not used in the method body. If this parameter is not needed, consider removing it from the method signature. If it's needed, ensure that it's used appropriately within the method.

-	public void deleteAccountNote(HttpServletRequest request, String accountId, String noteId) {
+	public void deleteAccountNote(HttpServletRequest request, String noteId) {

Other than this, the changes look good and improve the readability of the code.

[coderabbitai]

Consider using constructor injection instead of field injection for CreateNoteFactory. This is a recommended practice as it makes your classes easier to test, makes dependencies explicit and reduces the risk of NullPointerExceptions.

- @Autowired
- private CreateNoteFactory createNoteFactory;
+ private final CreateNoteFactory createNoteFactory;

+ @Autowired
+ public CreateAccountNoteService(CreateNoteFactory createNoteFactory) {
+     this.createNoteFactory = createNoteFactory;
+ }

[coderabbitai]

There's no null check before casting the attribute "current_user" to SalesforceUser. If the attribute doesn't exist or isn't of the correct type, this will throw a ClassCastException. Consider adding a null check or try-catch block to handle potential exceptions.

- SalesforceUser currentUser = (SalesforceUser) request.getAttribute("current_user");
+ Object userObject = request.getAttribute("current_user");
+ if(userObject instanceof SalesforceUser){
+     SalesforceUser currentUser = (SalesforceUser) userObject;
+ } else {
+     // Handle error case here
+ }

[coderabbitai]

Update the JavaDoc comment to accurately reflect the method parameters.

-	/**
-	 * Get the details of a note
-	 * @param accountId
-	 * @param noteId
-	 * @return GetNoteDetailsFormatterDto
-	 */
+	/**
+	 * Get the details of a note
+	 * @param request
+	 * @param noteId
+	 * @return GetNoteDetailsFormatterDto
+	 */
	public GetNoteDetailsFormatterDto getNoteDetails(HttpServletRequest request, String noteId) {
		User currentUser = (User) request.getAttribute("current_user");

		return getNoteDetailsFactory.getNoteDetails(currentUser, noteId);
	}

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 42e03d2 and 862debc commits.

Files selected for processing (1)

• repo-docs/LOCAL_SETUP.md (3 hunks)

Files skipped from review due to trivial changes (1)

• repo-docs/LOCAL_SETUP.md