Validation for Whitespace, Restricted and Duplicate

[akshatjain-official]

Describe your changes

This PR provides a solution to handle the different scenarios for

• Whitespace/Null, restricted characters ('/' or ''), and duplicate check in file renaming.
• Fixed the Unit and Integration Test with the increased coverage.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist before requesting a review

• I follow Java Development Guidelines for SAP
• I have tested the functionality on my cloud environment.
• I have provided sufficient automated/ unit tests for the code.
• I have increased or maintained the test coverage.
• I have ran integration tests on my cloud environment.
• I have validated blackduck portal for any vulnerability after my commit.

Upload Screenshots/lists of the scenarios tested

• I have Uploaded Screenshots or added lists of the scenarios tested in description

[akshatjain-official]

gemini review

[github-actions]

Gemini Automated Review
Summary of Changes
The pull request significantly refactors and centralizes filename validation logic into SDMUtils.validateFileNames, leading to reduced code duplication and improved separation of concerns within SDMCreateAttachmentsHandler and SDMUpdateAttachmentsHandler. Error message generation has been standardized and made more granular through new constants and helper methods in SDMConstants. Robustness has been enhanced by explicitly handling null/blank filenames in validation and duplicate checks. Furthermore, method names have been clarified (e.g., isRestrictedCharactersInName to hasRestrictedCharactersInName), and the overall readability of handler logic has been streamlined. The changes also include numerous test improvements, such as explicit mocking for file uploads, improved type safety in mock results, and better test coverage for utility methods.

Best Practices Review 💡

• Test Independence: Relying on @Test @Order for test execution is an anti-pattern. Tests should be independent and atomic to ensure reliability, maintainability, and allow for parallel execution.
• Robust JSON Comparison: Direct equals() comparison of long, complex JSON strings for expected responses is brittle. Minor formatting differences can cause tests to fail. Using ObjectMapper.readTree() for comparison is significantly more robust.
• Deterministic Test Data: Using non-deterministic values like new Date().toString() for fields like createdAt and modifiedBy in test data can lead to flaky tests. Test data should use fixed, known values.
• Type Safety: Raw type usage in constructs like Result.listOf(Map.class) should be addressed with proper generic type declarations (e.g., List<Map<String, Object>>) to improve type safety and eliminate warnings.
• Comprehensive Utility Testing: Utility methods, especially for validation, should explicitly include test cases for null and empty string inputs to clearly define and ensure robust behavior.
• Clarity in Test Mocks: When mocking a specific failure (e.g., filename validation returning false) but asserting a different error (e.g., authorization error), the test setup's intent should be highly clear and isolated. Consider dedicated tests for specific failure conditions.

Potential Bugs 🐛

• Confusing Test Intent: The test testRenameMultipleAttachmentsWithOneUnsupportedCharacter has a confusing intent. It first asserts an expected failure for renaming with unsupported characters but then proceeds to attempt a successful rename and save, which might obscure the original purpose of verifying the failure condition.
• Skipped Assertions: In the new test testRenameToValidateNames, the if (successCount) block, which contains the main assertion for the aggregate error message, will almost certainly be skipped. The names array intentionally includes invalid filenames expected to cause api.renameAttachment calls to return something other than "Renamed", meaning successCount will be false and the critical error validation will never execute.
• Misleading Failure Messages: In testRenameToValidateNames (and potentially other tests), the fail("Could not create entity"); statement at the end of the if (!testStatus) block provides a misleading error message if the initial createEntityDraft call succeeds but any subsequent rename or save operations fail. The failure should reflect the actual reason.

Recommendations & Required Changes 🛠️

1. Eliminate Test Order Dependencies and Verbose Output: Remove @Order annotations from all test methods to ensure test independence. Additionally, remove verbose System.out.println("Test (XX) : ...") statements which clutter test output and are better handled by test reporting tools.

   // Remove @Order annotations from all test methods.
   // Example:
   // @Test
   // // @Order(18) // Remove this line
   // void testRenameToValidateNames() throws IOException {
   //   // System.out.println("Test (18) : Rename attachments to validate names"); // Consider removing or replacing this debug output
   //   ...
   // }

2. Extract Repeated Test Logic into Helper Methods: Consolidate the repeated pattern of checking for an expected error message, then attempting a rename and save operation, into a reusable helper method to improve test clarity and reduce duplication.

   private boolean attemptRecoveryAndSave(String appUrl, String entityName, String facetName, String entityID, String attachmentID, String newName) {
       String renameResponse = api.renameAttachment(appUrl, entityName, facetName, entityID, attachmentID, newName);
       if ("Renamed".equals(renameResponse)) {
           String saveResponse = api.saveEntityDraft(appUrl, entityName, srvpath, entityID);
           return "Saved".equals(saveResponse);
       }
       return false;
   }

   Then, use it like:

   if (response.equals(expected)) {
       testStatus = attemptRecoveryAndSave(appUrl, entityName, facet[2], entityID, ID3[2], "note_valid");
   }

3. Improve JSON Response Comparison in Tests: Replace brittle direct equals() comparisons of complex JSON strings with a more robust method using ObjectMapper.readTree() to handle potential formatting variations (e.g., whitespace, key order) without failing tests.

   // Example for: String expected = "{\"error\":{\"code\":\"400\",\"message\":\"...\"}}";
   // if (response.equals(expected)) {
   ObjectMapper mapper = new ObjectMapper();
   JsonNode actualNode = mapper.readTree(response);
   JsonNode expectedNode = mapper.readTree(expected);
   if (actualNode.equals(expectedNode)) {
   // ...
   }

4. Use Fixed Date Values for Test Data: Replace dynamic new Date().toString() calls when populating test data for createdAt and modifiedBy fields with fixed, deterministic date strings (e.g., ISO 8601 format) to prevent flaky tests.

   // postData.put("createdAt", new Date().toString());
   // postData.put("modifiedBy", new Date().toString());
   postData.put("createdAt", "2023-10-26T10:00:00Z"); // Use a fixed ISO 8601 string or similar
   postData.put("modifiedBy", "2023-10-26T10:00:00Z");

5. Fix Skipped Assertion in testRenameToValidateNames: Modify the testRenameToValidateNames method to ensure the critical assertion block for the aggregated error message is executed by removing the conditional if (successCount). This ensures that the consolidated error response after attempting multiple invalid renames is always verified.

   // Suggested fix: Remove the 'if (successCount)' condition to ensure the save and assertion always run.
   // This assumes the test's intent is to verify the *save* operation's consolidated error response
   // after attempting multiple invalid renames.
   // The successCount variable can still be used for other purposes if needed, but not to gate the main assertion.
   response = api.saveEntityDraft(appUrl, entityName, srvpath, entityID3);
   String expected =
       "{\"error\":{\"code\":\"400\",\"message\":\"The object name cannot be empty or consist entirely of space characters. Enter a value.\",\"details\":[{\"code\":\"<none>\",\"message\":\"\\\"Restricted/Character\\\" contains unsupported characters (‘/’ or ‘\\\\’). Rename and try again.\",\"@Common.numericSeverity\":4},{\"code\":\"<none>\",\"message\":\"An object named \\\"duplicateName.pdf\\\" already exists. Rename the object and try again.\",\"@Common.numericSeverity\":4}]}}";
   if (response.equals(expected)) {
     response = api.deleteEntityDraft(appUrl, entityName, entityID3);
     if (response.equals("Entity Draft Deleted")) testStatus = true;
   }

6. Provide Specific Test Failure Messages: Replace generic failure messages like fail("Could not create entity"); with more specific messages that pinpoint the exact stage or condition that caused the test failure.

   // Instead of: if (!testStatus) fail("Could not create entity");
   // Consider:
   if (!testStatus) {
       fail("Test (14) failed: Expected errors not matched or recovery failed.");
   }
   // And for initial entity creation:
   if (!response.equals("Could not create entity")) {
       entityID3 = response;
   } else {
       fail("Test (14) failed: Initial entity creation failed.");
       return;
   }
   // And for the final `testRenameToValidateNames` check:
   if (!testStatus) fail("Expected validation errors were not returned upon save or cleanup failed.");

7. Address Raw Type Usage in Test Mocks: Refactor test code that uses raw types (e.g., List<Map>) for mock return values to use proper generic type declarations (e.g., List<Map<String, Object>>), utilizing @SuppressWarnings("unchecked") as needed when interacting with existing interfaces.

   // Before (example):
   // List<Map> mockedResultList = new ArrayList<>();
   // when(result.listOf(Map.class)).thenReturn((List) mockedResultList);
   
   // After (example):
   List<Map<String, Object>> mockedResultList = new ArrayList<>();
   // ... populate mockedResultList ...
   @SuppressWarnings("unchecked")
   List<Map> typedList = (List<Map>) (List<?>) mockedResultList;
   when(result.listOf(Map.class)).thenReturn(typedList);

8. Update SDMConstants.nameConstraintMessage Calls: Update all calls to SDMConstants.nameConstraintMessage and SDMConstants.linkNameConstraintMessage to remove the now-obsolete context string argument, aligning with the refactored method signatures.

   // Before (example):
   // SDMConstants.nameConstraintMessage(Collections.singletonList("[email protected]"), "Upload")
   // After:
   SDMConstants.nameConstraintMessage(Collections.singletonList("[email protected]"))

9. Clarify testRenameMultipleAttachmentsWithOneUnsupportedCharacter Intent: Review and refine the testRenameMultipleAttachmentsWithOneUnsupportedCharacter test. Either focus it solely on verifying the expected failure, or clearly separate the failure assertion from any subsequent recovery attempts to avoid ambiguity in its purpose.
10. Clarify Mocking in testCreateNonVersionedDIUnauthorizedI18n: For greater clarity and isolation, consider refining the test setup in testCreateNonVersionedDIUnauthorizedI18n. If the goal is to test authorization despite a validation failure, ensure this interaction is clearly modeled. Alternatively, add a dedicated test to verify the specific error message generated when SDMUtils.validateFileNames returns false (if it doesn't throw an exception).

Quality Rating ⭐
6/10

Overall Assessment
This pull request makes significant and positive strides in improving the core application logic, particularly in filename validation and error message consistency. These refactorings contribute positively to code maintainability and robustness. However, the accompanying test suite has a considerable number of best practice violations and potential bugs that detract from the overall quality and reliability of the changes. The presence of inter-dependent tests, brittle JSON comparisons, misleading failure messages, and unclear test intents indicates that the test code needs further refinement. Addressing these issues is crucial to ensure that the solid improvements in the production code are backed by an equally reliable and maintainable test foundation before merging.

[rishikunnath2747]

Can we move this method to AttachmentHandlerUtils? As it is only being used in the 2 handler classes

[rishikunnath2747]

As this parameter is removed, are we still handling duplicate case in SDM repository (not UI check)

[rishikunnath2747]

You have dismissed a few comments above this by the github code review - but these kind of problems are usually what cause the sonar scan to fail once the PR is merged. So I would suggest we have a discussion to see whether these can be ignored or do they need to be addressed

[rishikunnath2747]

Let's discuss whether fetchAttachments should be in AttachmentsHandlerUtils class, it may be better to place it in SDMUtils itself

[rishikunnath2747]

NIT : can we rename this method to something like fileNameHasWhitespace and isFileNameContainsRestrictedCharaters to something like -> fileNameContainsRestrictedCharaters