chore: Add simpler constructors for ResponseDTO#39338
Conversation
WalkthroughThis pull request refactors the instantiation of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
# Conflicts: # app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ResponseDTO.java (1)
26-28: LGTM! Good enhancement to the constructor API.The new constructor improves type safety by using
HttpStatusenum instead of raw integer status codes, while maintaining backward compatibility through delegation.Consider marking the integer-based constructor as
@Deprecatedin a future PR, to guide users toward using this new, safer constructor.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/InstanceAdminControllerCE.java (1)
39-48: Consider removing this deprecated endpoint in the future.This endpoint is marked as @deprecated but still maintains the same functionality as its multipart form-data counterpart. As mentioned in the PR objectives about gradual migration, consider creating a follow-up ticket to remove this endpoint after ensuring all clients have migrated to using the multipart form-data endpoint.
Would you like me to help create a tracking issue for the deprecation removal?
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/DatasourceControllerCE.java (1)
70-70: Consider using HttpStatus.CREATED in ResponseDTO constructor.For consistency with the
@ResponseStatusannotation, consider usingHttpStatus.CREATEDin theResponseDTOconstructor instead ofHttpStatus.OK.- return datasourceService.create(resource).map(created -> new ResponseDTO<>(HttpStatus.CREATED, created)); + return datasourceService.create(resource).map(created -> new ResponseDTO<>(HttpStatus.CREATED, created));Also applies to: 71-71, 72-72, 73-73
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (2)
80-80: Fix mismatch between @ResponseStatus and ResponseDTO status.The method is annotated with
@ResponseStatus(HttpStatus.CREATED)but uses the same status inResponseDTOconstructor. This is redundant and could be confusing. Consider removing the@ResponseStatusannotation since the status is already set in theResponseDTO.- @ResponseStatus(HttpStatus.CREATED) public Mono<ResponseDTO<String>> commit(Also applies to: 86-86
91-91: Fix mismatch between @ResponseStatus and ResponseDTO status.Similar to the previous comment, this method has redundant status declaration. Consider removing the
@ResponseStatusannotation.- @ResponseStatus(HttpStatus.CREATED) public Mono<ResponseDTO<? extends Artifact>> createReference(Also applies to: 102-102
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ThemeControllerCE.java (2)
54-54: Consider using HttpStatus.CREATED for resource updatesWhile the change to ResponseDTO construction is good, consider using HttpStatus.CREATED for consistency with REST conventions when creating/updating resources.
- .map(theme -> new ResponseDTO<>(HttpStatus.OK, theme)); + .map(theme -> new ResponseDTO<>(HttpStatus.CREATED, theme));
60-60: Consider breaking the chain for better readabilityWhile the ResponseDTO construction change is good, consider breaking the chain into multiple lines for better readability.
- return service.updateName(themeId, resource).map(theme -> new ResponseDTO<>(HttpStatus.OK, theme)); + return service.updateName(themeId, resource) + .map(theme -> new ResponseDTO<>(HttpStatus.OK, theme));app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (1)
1-113: Consider adding documentation about ResponseDTO usage.Since this is part of a larger effort to simplify ResponseDTO construction, consider adding a comment in the class documentation about the preferred way to construct ResponseDTO objects.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3)
127-140: Consider enhancing error logging.While the error handling is robust, consider adding debug logging in the
getErrorResponseMonomethod to help with troubleshooting.protected <T> Mono<ResponseDTO<T>> getErrorResponseMono(Throwable error) { if (error instanceof AppsmithException appsmithException) { + log.debug("Handling AppsmithException: {}", appsmithException.getMessage()); return Mono.just(new ResponseDTO<>( appsmithException.getHttpStatus(), new ErrorDTO( appsmithException.getAppErrorCode(), appsmithException.getErrorType(), appsmithException.getMessage(), appsmithException.getTitle()))); } + log.debug("Handling generic error: {}", error.getMessage()); return Mono.just(new ResponseDTO<>( INTERNAL_SERVER_ERROR_STATUS, new ErrorDTO(INTERNAL_SERVER_ERROR_CODE, error.getMessage()))); }
191-199: Document cache behavior.Consider adding Javadoc to explain the caching strategy and its implications for the feature flags response.
+ /** + * Caches the feature flags response to avoid repeated database queries. + * The cache is maintained until the Mono is subscribed to again. + */ Mono<ResponseDTO<Map<String, Boolean>>> featureFlagsForCurrentUserResponseDTOMonoCache = userDataService .getFeatureFlagsForCurrentUser() .as(this::toResponseDTO) .doOnError(e -> log.error("Error fetching feature flags", e)) .doOnSuccess(consolidatedAPIResponseDTO::setFeatureFlags)
650-720: Consider adding input validation for ETag computation.The
computeConsolidatedAPIResponseEtagmethod should validate the input DTO before processing to prevent potential NPEs.@NotNull public String computeConsolidatedAPIResponseEtag( ConsolidatedAPIResponseDTO consolidatedAPIResponseDTO, String defaultPageId, String applicationId) { + if (consolidatedAPIResponseDTO == null) { + log.warn("Skipping etag computation: ConsolidatedAPIResponseDTO is null"); + return ""; + } if (isBlank(defaultPageId) && isBlank(applicationId)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/LogoutSuccessHandlerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ActionCollectionControllerCE.java(9 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ActionControllerCE.java(9 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java(19 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationTemplateControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ConfigControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/CustomJSLibControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/DatasourceControllerCE.java(10 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java(17 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/HealthCheckControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/InstanceAdminControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/LayoutControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java(11 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ProductFeatureAlertControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/RestApiImportControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SearchEntityControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ThemeControllerCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UsagePulseControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/WorkspaceControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ResponseDTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java(17 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/LayoutControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ActionCollectionControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationTemplateControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java
🔇 Additional comments (32)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SearchEntityControllerCE.java (1)
36-36: LGTM! Clean and consistent with the PR objectives.The change simplifies ResponseDTO construction by using HttpStatus enum directly instead of its integer value, making the code more maintainable and type-safe.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/HealthCheckControllerCE.java (1)
25-25: LGTM! Clean and consistent with the PR objectives.The simplified
ResponseDTOconstructor usage improves code clarity by removing the unnecessarynullmessage parameter and using theHttpStatusenum directly.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/RestApiImportControllerCE.java (1)
53-53: LGTM! Cleaner ResponseDTO construction.The direct use of
HttpStatus.CREATEDinstead ofHttpStatus.CREATED.value()simplifies the code while maintaining the same functionality. This change aligns well with the@ResponseStatusannotation usage and reduces unnecessary enum-to-int conversion.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UsagePulseControllerCE.java (1)
28-28: LGTM! Nice simplification of ResponseDTO construction.The change improves code clarity by using HttpStatus enum directly and removing the unnecessary null message parameter.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ProductFeatureAlertControllerCE.java (1)
28-28: LGTM! Clean and consistent with the PR objectives.The changes simplify the
ResponseDTOconstruction by usingHttpStatusenum directly, making the code more readable while maintaining the same functionality.Also applies to: 30-30
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/LogoutSuccessHandlerCE.java (1)
48-48: LGTM! Clean and type-safe refactor.The change improves type safety by using HttpStatus enum directly and removes the unnecessary null parameter, aligning with the broader effort to simplify ResponseDTO construction.
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ResponseDTO.java (1)
9-9: LGTM!Clean addition of Spring's HttpStatus enum import.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (2)
55-55: LGTM! Improved ResponseDTO construction.The change enhances type safety by using
HttpStatus.OKdirectly instead of its integer value, while removing the unnecessary null message parameter.
68-68: LGTM! Consistent with the simplified ResponseDTO pattern.The modification maintains consistency with the new pattern of using
HttpStatusenum directly.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/CustomJSLibControllerCE.java (4)
50-50: LGTM! Simplified ResponseDTO construction.The change aligns with the PR objective to simplify ResponseDTO construction by using HttpStatus enum directly.
67-67: LGTM! Consistent with the simplified ResponseDTO pattern.The change maintains consistency with the new ResponseDTO construction pattern.
81-81: LGTM! Clean ResponseDTO instantiation.The change follows the same simplified pattern, improving code clarity.
97-97: LGTM! Maintains consistent ResponseDTO pattern.The change completes the consistent application of the simplified ResponseDTO construction pattern across all methods.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/InstanceAdminControllerCE.java (1)
36-36: LGTM! Consistent improvement in ResponseDTO construction.The changes consistently simplify the ResponseDTO construction across all methods by using HttpStatus enum directly instead of its integer value. This aligns well with the PR objectives.
Also applies to: 47-47, 59-59, 66-66, 73-73
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ConfigControllerCE.java (2)
29-29: LGTM! Simplified ResponseDTO construction.The change improves code clarity by removing the unnecessary null message parameter.
35-35: LGTM! Consistent with getByName changes.The simplified constructor usage maintains consistency across the controller methods.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (2)
73-73: LGTM! Clean and consistent ResponseDTO construction.The change simplifies the ResponseDTO instantiation by using HttpStatus enum directly and removing the unnecessary message parameter.
118-118: LGTM! Consistent ResponseDTO construction across all response scenarios.The changes maintain proper HTTP status code handling while simplifying the ResponseDTO instantiation pattern.
Also applies to: 128-128, 133-133
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/DatasourceControllerCE.java (1)
65-65: LGTM! Consistent simplification of ResponseDTO construction.The changes consistently simplify the
ResponseDTOconstructor calls by directly usingHttpStatusenum instead of.value(). The HTTP status codes are appropriately used across all endpoints:
CREATEDfor resource creationOKfor all other successful operationsAlso applies to: 73-73, 85-85, 100-100, 109-109, 121-121, 133-133, 170-170, 180-180, 192-192, 204-204
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (1)
64-64: LGTM! Consistent simplification of ResponseDTO construction.The changes consistently simplify the
ResponseDTOconstruction by directly usingHttpStatusenum instead ofHttpStatus.value().Also applies to: 75-75, 111-111, 120-120, 129-129, 140-140, 151-151, 165-165, 179-179, 191-191, 200-200, 210-210, 218-218, 226-226, 236-236, 244-244, 257-257
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ThemeControllerCE.java (2)
37-37: LGTM! Simplified ResponseDTO constructionThe change correctly simplifies the ResponseDTO constructor while maintaining the same functionality.
46-46: LGTM! Consistent with the simplified constructor patternThe change maintains consistency with the new ResponseDTO construction pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (4)
54-54: LGTM! Clean refactor of ResponseDTO construction.The change simplifies the code while maintaining the same functionality.
62-62: LGTM! Consistent with the new ResponseDTO pattern.The refactor maintains consistency with other methods in the controller.
72-72: LGTM! Appropriate use of HTTP 202 Accepted status.The refactor maintains the correct semantic use of HTTP status codes while simplifying the code.
80-80: LGTM! Consistent refactoring across all methods.All response constructions have been simplified consistently while maintaining the same HTTP status codes and functionality.
Also applies to: 91-91, 97-97, 105-105, 111-111
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (2)
96-121: Well-structured dependency injection!The constructor implementation using Lombok's
@RequiredArgsConstructorwith final fields follows Spring best practices for dependency injection.
123-125: Good improvement in type safety!Using
HttpStatusenum directly instead of its integer value improves type safety and readability.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java (2)
86-86: LGTM! Status code matches the @ResponseStatus annotation.The use of
HttpStatus.CREATEDaligns with the@ResponseStatus(HttpStatus.CREATED)annotation on line 81.
82-296: LGTM! Consistent refactoring of ResponseDTO construction.The changes consistently simplify the
ResponseDTOconstruction by directly usingHttpStatusenum values instead of their integer representations. This improves code readability and maintainability.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ActionControllerCE.java (1)
61-61:Details
❓ Verification inconclusive
LGTM! Consistent refactoring across all response constructions.
The changes simplify the
ResponseDTOconstruction by directly usingHttpStatusenum instead of its integer value, making the code cleaner and more type-safe.Let's verify that the new
ResponseDTOconstructor is properly defined:Also applies to: 72-72, 85-85, 96-96, 105-105, 115-115, 125-125, 135-135, 155-155
🏁 Script executed:
#!/bin/bash # Search for the new ResponseDTO constructor definition ast-grep --pattern 'class ResponseDTO { $$$ ResponseDTO(HttpStatus $_) { $$$ } $$$ }'Length of output: 99
Refactoring consistency looks solid, but please verify the new constructor
The refactoring across all endpoints in
ActionControllerCE.javais consistently simplified by directly passing theHttpStatusenum to theResponseDTOconstructor. However, our initial search for a matching constructor inResponseDTO(i.e., one accepting aHttpStatusargument) did not yield any output. This might be due to tooling limitations or an issue in locating the exact constructor definition.Could you please double-check (manually or via an additional script) that the new
ResponseDTO(HttpStatus ...)constructor is defined and available? This verification is necessary to ensure that there are no breaking changes due to the constructor update.
- Review changes in endpoints (lines 61, 72, 85, 96, 105, 115, 125, 135, 155) confirming they now use
new ResponseDTO<>(HttpStatus, data).- Action Required: Verify the presence and correct implementation of the
ResponseDTO(HttpStatus ...)constructor.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/WorkspaceControllerCE.java (1)
40-40:Details
❓ Verification inconclusive
LGTM! Consistent simplification of ResponseDTO construction.
The changes consistently simplify the
ResponseDTOconstructor calls by usingHttpStatusenum directly and removing the unusedmessageparameter. This improves code readability without affecting functionality.Since this changes how
ResponseDTOobjects are constructed, let's verify there are no breaking changes for API consumers:Also applies to: 47-47, 53-53, 59-59, 70-70, 78-78, 89-89, 97-97, 103-103, 112-112
🏁 Script executed:
#!/bin/bash # Description: Check if ResponseDTO is used in tests or documentation with the old constructor pattern # Search for ResponseDTO constructor usage in tests echo "Searching for ResponseDTO constructor usage in tests..." rg -A 2 "new ResponseDTO.*\(.*\.value\(\)" --type java # Search for ResponseDTO constructor usage in API documentation echo "Searching for ResponseDTO constructor usage in documentation..." rg -A 2 "new ResponseDTO.*\(.*\.value\(\)" --type md --type yamlLength of output: 2239
Simplification in WorkspaceControllerCE looks good—but verify its broader impact.
The changes in
WorkspaceControllerCE.javaconsistently update theResponseDTOconstruction by directly using theHttpStatusenum and removing the unusedmessageparameter, which improves readability. However, our search shows that other parts of the codebase (e.g., inSecurityConfig.java,GlobalExceptionHandler.java, andCSRFFilter.java) still rely on the older constructor pattern usingHttpStatus.value(). Please confirm that these legacy usages either remain backward compatible or are scheduled for similar updates to avoid potential API consumer issues.
...ith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java
Show resolved
Hide resolved
## Description Refactoring to make calls to construct new `ResponseDTO` objects, be simpler. We aren't using the `ResponseDTO(int status, T data, String message)` signature with non-`null` value for `message`, so this is baggage. I'm not removing that signature in this PR, but that'll come in the next one. After these changes flow into EE and I migrate the usages there as well. No need to hurry, slow but steady. 🙂 ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13425590614> > Commit: 486a4d0 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13425590614&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Thu, 20 Feb 2025 01:37:27 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Standardized backend response handling across numerous endpoints for improved clarity and consistency. - Enhanced the internal response construction process to ensure reliable and maintainable operations without impacting any end-user functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Refactoring to make calls to construct new
ResponseDTOobjects, be simpler. We aren't using theResponseDTO(int status, T data, String message)signature with non-nullvalue formessage, so this is baggage.I'm not removing that signature in this PR, but that'll come in the next one. After these changes flow into EE and I migrate the usages there as well. No need to hurry, slow but steady. 🙂
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13425590614
Commit: 486a4d0
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 20 Feb 2025 01:37:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit