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

chore: Include version in all requests #37551

Merged
merged 36 commits into from
Nov 27, 2024
Merged

Conversation

sharat87
Copy link
Member

@sharat87 sharat87 commented Nov 19, 2024

We're adding x-appsmith-version to every request from the client. If server sees a request with a x-appsmith-version that doesn't match its own, it rejects. If the server sees a request without any x-appsmith-version header, we don't reject it... for now.

On the client, when server responds with the "version mismatch" error, we trigger the "There's a new version, please refresh" toast UI flow.

This is a step towards removing our dependency on websockets and client—RTS connection.

Automation

/test sanity

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12029721517
Commit: c4321f7
Cypress dashboard.
Tags: @tag.Sanity
Spec:


Tue, 26 Nov 2024 12:28:32 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a custom header for API requests to include application version.
    • Added specific error handling for 400 Bad Request errors, enhancing user feedback on version mismatches.
  • Improvements

    • Enhanced error handling for version mismatches, providing clearer error messages.
    • Simplified version update handling, focusing on logging and user prompts without state management.
    • Improved default handling for application version configuration to prevent undefined values.
  • Chores

    • Removed outdated version state management functions to streamline code.

Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

Walkthrough

The pull request introduces several changes primarily focused on enhancing configuration parsing and error handling across various files. Key modifications include updating the handling of the APPSMITH_VERSION_ID to provide a default value, adding a new function to include version information in request headers, and introducing error handling for 400 Bad Request responses. Additionally, the management of version updates has been simplified by removing state tracking, and new error codes have been added to improve error reporting.

Changes

File Change Summary
app/client/public/index.html Updated APPSMITH_VERSION_ID handling to default to "UNKNOWN" if not set.
app/client/src/api/interceptors/request/apiRequestInterceptor.ts Added addVersionHeader function to include X-Appsmith-Version in request headers.
app/client/src/api/interceptors/response/apiFailureResponseInterceptor.ts Introduced handleBadRequestError for specific handling of 400 Bad Request errors.
app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts New file for handleBadRequestError function to process 400 errors and handle version mismatches.
app/client/src/api/interceptors/response/failureHandlers/index.ts Exported handleBadRequestError from the index file for error handling functions.
app/client/src/sagas/WebsocketSagas/handleAppLevelSocketEvents.tsx Removed handling for APP_LEVEL_SOCKET_EVENTS.RELEASE_VERSION_NOTIFICATION, simplifying control flow.
app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts Removed version state management; simplified version update handling and logging.
app/client/src/utils/storage.ts Removed functions related to version update state management from local storage.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java Enhanced error handling by adding version checks against request headers and modifying error response logic.
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java Added new error code VERSION_MISMATCH for version mismatch scenarios.
app/client/src/ce/constants/ApiConstants.tsx Updated SERVER_ERROR_CODES to include VERSION_MISMATCH key with associated error codes.

Possibly related PRs

  • chore: Distributed tracing for client #37101: The changes in this PR enhance the telemetry capabilities of the application, which is related to the handling of configuration values, similar to the updates made in the main PR regarding the APPSMITH_VERSION_ID.
  • chore: Fix env varibles in 404.html #37672: This PR modifies how environment variables are parsed in the 404.html file, which aligns with the main PR's focus on improving the handling of configuration values in index.html.

Suggested labels

Bug

Suggested reviewers

  • sharat87
  • trishaanand
  • nidhi-nair
  • pratapaprasanna

🎉 In code we trust, with changes so bright,
Handling versions, we set things right.
From headers to errors, we’ve made it clear,
Appsmith’s now stronger, let’s all give a cheer!
🚀


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Nov 19, 2024
@sharat87 sharat87 added the ok-to-test Required label for CI label Nov 19, 2024
@sharat87 sharat87 changed the title chore: Include version in server responses chore: Include version in all requests and responses Nov 20, 2024
@sharat87
Copy link
Member Author

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11932349930.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37551.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-37551.dp.appsmith.com

@sharat87 sharat87 changed the title chore: Include version in all requests and responses chore: Include version in all requests Nov 23, 2024
@sharat87 sharat87 marked this pull request as ready for review November 25, 2024 08:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (9)
app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts (2)

17-20: Remove unnecessary async modifier

The function handleVersionMismatch does not use await. Consider removing async to simplify the code.

Apply this diff to remove the async keyword:

-export async function handleVersionMismatch(
+export function handleVersionMismatch(
  currentVersion: string,
  serverVersion: string,
) {

34-34: Use localized string for action text

Replace the hard-coded "refresh" text with a localized message for internationalization support.

Apply this diff:

     action: {
-      text: "refresh",
+      text: createMessage(REFRESH_BUTTON_LABEL),
       effect: () => handleUpdateRequested(currentVersion, serverVersion),
     },
app/client/src/api/interceptors/response/apiFailureResponseInterceptor.ts (1)

18-18: Consider handler order in the array.

The position of handleBadRequestError at the end of the array means version mismatch errors will only be handled after all other error types are checked. Consider if this is the desired behavior or if version mismatch should be checked earlier.

app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts (2)

6-8: Consider adding type guards for error response

The optional chaining is good, but consider adding type guards to ensure type safety of the error response structure.

+interface VersionMismatchError {
+  responseMeta: {
+    error: {
+      code: string;
+      message: string;
+    };
+  };
+}

-export const handleBadRequestError = async (error: AxiosError<ApiResponse>) => {
+export const handleBadRequestError = async (error: AxiosError<ApiResponse & VersionMismatchError>) => {

20-26: Type the custom error properties

Consider defining an interface for the custom error properties to maintain type safety.

+interface CustomError extends AxiosError<ApiResponse> {
+  clientDefinedError: boolean;
+  statusCode: string;
+  message: string;
+}

     return Promise.reject({
       ...error,
       clientDefinedError: true,
       statusCode: errorCode,
       message,
-    });
+    } as CustomError);
app/client/src/api/interceptors/request/apiRequestInterceptor.ts (1)

52-57: Add type safety for version handling

Consider adding type safety and error handling for the version configuration.

 const addVersionHeader = (config: InternalAxiosRequestConfig) => {
   config.headers = config.headers || {};
-  config.headers["X-Appsmith-Version"] = getAppsmithConfigs().appVersion.id;
+  const appVersion = getAppsmithConfigs()?.appVersion?.id ?? "UNKNOWN";
+  config.headers["X-Appsmith-Version"] = appVersion;
   return config;
 };
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (3)

120-122: Consider using constructor injection instead of field injection

Constructor injection is preferred as it makes dependencies explicit and improves testability.

-    @Autowired
-    private ProjectProperties projectProperties;

+    private final ProjectProperties projectProperties;
+
+    public SecurityConfig(
+            UserService userService,
+            AnalyticsService analyticsService,
+            /* ... other dependencies ... */
+            ProjectProperties projectProperties) {
+        this.userService = userService;
+        this.analyticsService = analyticsService;
+        /* ... other assignments ... */
+        this.projectProperties = projectProperties;
+    }

311-320: Extract header name as a constant

The header name should be defined as a constant to maintain consistency and ease future updates.

+    private static final String APPSMITH_VERSION_HEADER = "X-Appsmith-Version";

     // In sanityCheckFilter method:
-    final String versionHeaderValue = headers.getFirst("X-Appsmith-Version");
+    final String versionHeaderValue = headers.getFirst(APPSMITH_VERSION_HEADER);

338-349: Consolidate error response handling methods

There's code duplication between the two writeErrorResponse methods. Consider consolidating them.

-    private Mono<Void> writeErrorResponse(ServerWebExchange exchange, WebFilterChain chain, String message) {
-        final ServerHttpResponse response = exchange.getResponse();
-        response.setStatusCode(HttpStatus.BAD_REQUEST);
-        response.getHeaders().setContentType(MediaType.APPLICATION_JSON);
-        try {
-            return response.writeWith(Mono.just(response.bufferFactory()
-                    .wrap(objectMapper.writeValueAsBytes(
-                            new ResponseDTO<>(response.getStatusCode().value(), null, message, false)))));
-        } catch (JsonProcessingException ex) {
-            return chain.filter(exchange);
-        }
-    }

     private Mono<Void> writeErrorResponse(ServerWebExchange exchange, WebFilterChain chain, ErrorDTO error) {
         final ServerHttpResponse response = exchange.getResponse();
         final HttpStatus status = HttpStatus.BAD_REQUEST;
         response.setStatusCode(status);
         response.getHeaders().setContentType(MediaType.APPLICATION_JSON);
         try {
             return response.writeWith(Mono.just(response.bufferFactory()
                     .wrap(objectMapper.writeValueAsBytes(new ResponseDTO<>(status.value(), error)))));
         } catch (JsonProcessingException ex) {
             return chain.filter(exchange);
         }
     }

+    private Mono<Void> writeErrorResponse(ServerWebExchange exchange, WebFilterChain chain, String message) {
+        return writeErrorResponse(
+                exchange,
+                chain,
+                new ErrorDTO(null, message));
+    }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0685d33 and 35d9ba6.

📒 Files selected for processing (10)
  • app/client/public/index.html (1 hunks)
  • app/client/src/api/interceptors/request/apiRequestInterceptor.ts (1 hunks)
  • app/client/src/api/interceptors/response/apiFailureResponseInterceptor.ts (1 hunks)
  • app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts (1 hunks)
  • app/client/src/api/interceptors/response/failureHandlers/index.ts (1 hunks)
  • app/client/src/sagas/WebsocketSagas/handleAppLevelSocketEvents.tsx (0 hunks)
  • app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts (1 hunks)
  • app/client/src/utils/storage.ts (0 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (7 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java (1 hunks)
💤 Files with no reviewable changes (2)
  • app/client/src/sagas/WebsocketSagas/handleAppLevelSocketEvents.tsx
  • app/client/src/utils/storage.ts
🔇 Additional comments (10)
app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts (1)

8-14: Function handleUpdateRequested implementation is sound

app/client/src/api/interceptors/response/failureHandlers/index.ts (2)

2-2: Verify version mismatch handling in handleBadRequestError.ts

Since this PR implements version mismatch handling, ensure the implementation in handleBadRequestError.ts properly handles the "AE-BAD-4002" error code and triggers the refresh notification.

✅ Verification successful

Let me verify the implementation of version mismatch handling further by checking the handleVersionMismatch function.


Let me check the implementation of the version mismatch handling in the versionUpdatePrompt saga.


Version mismatch handling implementation verified successfully

The implementation correctly:

  • Detects "AE-BAD-4002" error code and extracts server version
  • Shows a non-auto-closing toast notification with refresh action
  • Logs analytics events for version mismatch detection and update requests
  • Handles the refresh action to reload the application
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check implementation of handleBadRequestError
rg -A 10 "AE-BAD-4002" ./app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts

Length of output: 510


Script:

#!/bin/bash
# Check implementation of handleVersionMismatch
rg -A 5 "handleVersionMismatch" ./app/client/src

# Check if there are any tests for this functionality
rg -A 5 "AE-BAD-4002|handleVersionMismatch" ./app/client/src/**/*test*

Length of output: 4233


Script:

#!/bin/bash
# Check the implementation of version mismatch handling in the saga
cat ./app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts

Length of output: 1023


2-2: LGTM! Verify integration with error handling system.

The export follows the established pattern and maintains good code organization.

✅ Verification successful

Implementation verified and properly integrated ✅

The handleBadRequestError is correctly:

  • Implemented as an error handler for 400 responses with specific version mismatch code (AE-BAD-4002)
  • Integrated into the API failure response interceptor chain
  • Exported alongside other error handlers in the index file
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the handleBadRequestError implementation and its usage
ast-grep --pattern 'handleBadRequestError($$$)' 

Length of output: 131422


Script:

#!/bin/bash
# Let's verify the implementation and usage of handleBadRequestError
# First, find the implementation file
fd handleBadRequestError.ts

# Then check for its implementation and usage
rg -A 10 "handleBadRequestError" --type ts

Length of output: 3702

app/client/src/api/interceptors/response/apiFailureResponseInterceptor.ts (1)

18-18: LGTM! Verify handler implementation.

The addition of handleBadRequestError aligns with the PR objective of handling version mismatch errors.

Let's verify the handler implementation:

✅ Verification successful

Let me generate another script to check the implementation since the previous one had some issues:

The previous script had issues with ast-grep pattern and the ripgrep command was incomplete. Let's try with these more focused searches to verify the handler implementation and version mismatch handling.


Implementation verified and properly handles version mismatch

The handler is well-implemented with proper error handling and version mismatch detection:

  • Correctly checks for 400 status and specific error code "AE-BAD-4002"
  • Extracts server version from error message
  • Integrates with handleVersionMismatch utility
  • Returns appropriate error response
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of handleBadRequestError and its exports

# Check if the handler is properly implemented and exported
ast-grep --pattern 'export const handleBadRequestError = $_'

# Check if the handler handles version mismatch
rg -A 5 "handleVersionMismatch|x-appsmith-version" 

Length of output: 144802


Script:

# Check the implementation of handleBadRequestError and handleVersionMismatch
rg -A 10 "handleBadRequestError|handleVersionMismatch" --type ts

# Check version header related code
rg -A 5 "x-appsmith-version" --type ts

Length of output: 6113

app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts (1)

28-29: LGTM!

The default case handling is clean and straightforward.

app/client/src/api/interceptors/request/apiRequestInterceptor.ts (2)

63-63: LGTM! Proper placement in the interceptor pipeline

The version header interceptor is correctly positioned in the pipeline, after basic headers but before specific feature headers.


52-57: Verify server-side header name compatibility

The implementation adds X-Appsmith-Version header to all requests. Let's verify this matches the server-side expectations.

Also applies to: 63-63

✅ Verification successful

Header implementation verified and matches server expectations

The X-Appsmith-Version header name is correctly implemented and matches the server-side usage in SecurityConfig.java. The server explicitly checks for this exact header name.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the version header name usage in server-side code
rg -i "x-appsmith-version" --type java --type typescript

Length of output: 91


Script:

#!/bin/bash
# Search for version header usage in server code with correct file types
rg -i "x-appsmith-version" --type-add 'ts:*.{ts,tsx}' --type ts --type java

# Search for version header constant definitions
rg -i 'const.*version.*header' --type-add 'ts:*.{ts,tsx}' --type ts --type java

# Search for version-related headers in server request handling
ast-grep --pattern 'request.getHeader($version)'

Length of output: 1264

app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java (1)

70-70: LGTM! Verify error code usage.

The new VERSION_MISMATCH error code follows the established pattern and aligns with the PR objective of implementing version checks.

Let's verify the error code usage:

✅ Verification successful

Error code implementation is properly integrated

The VERSION_MISMATCH error code is correctly implemented and used across the stack:

  • Server throws it in SecurityConfig when versions don't match
  • Client handles it in the error interceptor to trigger version mismatch UI
  • Proper error message parsing and version comparison logic is in place
  • Triggers a user-friendly refresh prompt when versions mismatch
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check where and how the VERSION_MISMATCH error code is used

# Search for direct references to the error code
rg -l "VERSION_MISMATCH" 

# Search for the error code string
rg "AE-BAD-4002"

Length of output: 646


Script:

#!/bin/bash
# Check the implementation details of version mismatch handling
rg -A 10 "VERSION_MISMATCH" app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts
rg -A 10 "VERSION_MISMATCH" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
rg -A 10 "AE-BAD-4002" app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts

Length of output: 1873

app/client/public/index.html (2)

265-265: ⚠️ Potential issue

Reconsider using "UNKNOWN" as the default version value

While providing a fallback value is good practice, using "UNKNOWN" as the default version might bypass server version checks and hide configuration issues.

Let's verify how the server handles the "UNKNOWN" version:

✅ Verification successful

Using "UNKNOWN" as default version is acceptable

The server has proper version mismatch handling that only triggers when a non-null version is provided. The "UNKNOWN" fallback:

  • Won't trigger version mismatch errors as the check is skipped for null values
  • Is already an expected fallback in ProjectProperties.java
  • Provides better observability than a null value for debugging
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how version headers are handled in the server code

# Search for version header validation logic
rg -A 5 "X-Appsmith-Version"

# Look for version comparison or validation code
ast-grep --pattern 'if ($version == "UNKNOWN") { $$$ }'

Length of output: 1549


Script:

#!/bin/bash
# Let's check how version mismatches are handled in the server code
rg -A 10 "projectProperties.getVersion()" app/server/appsmith-server/src/main/java/

# Also check if there's any special handling for "UNKNOWN" version
rg -i "unknown.*version" app/server/appsmith-server/src/main/java/

# Check version-related error handling
rg -A 5 "ErrorDTO.*version" app/server/appsmith-server/src/main/java/

Length of output: 14226


264-268: Verify version configuration across environments

The appVersion configuration block looks correct, but we should ensure consistent version handling across different deployment scenarios.

Let's check version-related configurations:

✅ Verification successful

Version configuration is consistently handled across environments

The version configuration is properly implemented with consistent handling:

  • Docker environment uses the same version variables in caddy-reconfigure.mjs
  • Test environment in jest.config.js maintains identical structure
  • Nginx template correctly substitutes version placeholders
  • Only APPSMITH_VERSION_ID has a fallback to "UNKNOWN", which is appropriate for this identifier
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check version-related configurations across the codebase

# Search for version-related environment variables
rg "APPSMITH_VERSION_ID|APPSMITH_VERSION_SHA|APPSMITH_VERSION_RELEASE_DATE"

# Look for other version fallback handling
ast-grep --pattern 'parseConfig($version) || $fallback'

Length of output: 1265

@trishaanand
Copy link
Contributor

trishaanand commented Nov 25, 2024

@sharat87 : Server side changes LGTM. Can proceed with approval post client side review.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (1)

311-319: Consider adding logging for version mismatches

The version validation logic is well-implemented. However, tracking version mismatches could provide valuable insights for monitoring client-server version distributions.

 if (versionHeaderValue != null && !serverVersion.equals(versionHeaderValue)) {
+    log.debug("Version mismatch detected - Client: {}, Server: {}", versionHeaderValue, serverVersion);
     final ErrorDTO error = new ErrorDTO(
             AppsmithErrorCode.VERSION_MISMATCH.getCode(), AppsmithErrorCode.VERSION_MISMATCH.getDescription());
     return writeErrorResponse(exchange, chain, error, new VersionMismatchData(serverVersion));
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 35d9ba6 and 3311cd2.

📒 Files selected for processing (3)
  • app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts (1 hunks)
  • app/client/src/ce/constants/ApiConstants.tsx (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts
🔇 Additional comments (3)
app/client/src/ce/constants/ApiConstants.tsx (1)

67-67: LGTM! The error code addition aligns with version mismatch handling requirements.

The VERSION_MISMATCH constant with error code "AE-BAD-4002" is correctly implemented for handling version mismatch scenarios.

Let's verify consistent usage of this error code:

✅ Verification successful

Let me search for the actual usage of this error code in error handling code to verify its implementation.


Let me try searching without file type restrictions since the previous attempt failed due to unrecognized file types.


LGTM! The version mismatch error code is properly integrated into the error handling flow.

The error code is:

  • Defined in ApiConstants.tsx as SERVER_ERROR_CODES.VERSION_MISMATCH
  • Handled in handleBadRequestError.ts with proper version comparison logic
  • Matched with server-side AppsmithErrorCode.java definition
  • Triggers appropriate user notification via versionUpdatePrompt.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent usage of VERSION_MISMATCH error code
# Expected: Should find usage in error handling code

# Check for direct usage of the error code
rg -l "AE-BAD-4002"

# Check for usage of VERSION_MISMATCH constant
rg -l "VERSION_MISMATCH.*SERVER_ERROR_CODES"

# Check for related error handling
ast-grep --pattern 'handleBadRequestError($$$)'

Length of output: 130926


Script:

#!/bin/bash
# Search for version mismatch error code usage in error handling code
rg -A 5 "VERSION_MISMATCH" --type ts --type tsx

# Search for error handling code that might use this error code
rg -A 5 "handleBadRequestError" --type ts --type tsx

# Search for any other files that might handle version mismatch errors
rg -A 5 "AE-BAD-4002" --type ts --type tsx

Length of output: 227


Script:

#!/bin/bash
# Search for version mismatch error code usage in error handling code without file type restrictions
rg -A 5 "VERSION_MISMATCH"

# Search for error handling code that might use this error code
rg -A 5 "handleBadRequestError"

# Search for any other files that might handle version mismatch errors
rg -A 5 "AE-BAD-4002"

Length of output: 8431

app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (2)

3-3: LGTM: Dependencies and configurations are properly set up

The new imports and autowired configurations are correctly implemented for version validation.

Also applies to: 120-121


336-354: LGTM: Well-structured error handling implementation

The generic error response method and VersionMismatchData record are well-implemented, following Java best practices.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app/client/src/api/interceptors/request/addVersionHeader.ts (2)

3-6: Add input validation for the version parameter

While the types provide compile-time safety, runtime validation of the version string would make the function more robust.

Consider adding validation:

 export const addVersionHeader = (
   config: InternalAxiosRequestConfig,
   options: { version: string },
 ) => {
+  if (!options?.version) {
+    throw new Error("Version is required for request headers");
+  }

9-10: Consider using immutable pattern for header modification

The function directly modifies the config object. While this is common in axios interceptors, consider creating a new headers object to maintain immutability.

Here's a more immutable approach:

-  config.headers = config.headers || {};
-  config.headers["X-Appsmith-Version"] = version;
+  config.headers = {
+    ...config.headers,
+    "X-Appsmith-Version": version,
+  };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d8244c9 and c4321f7.

📒 Files selected for processing (3)
  • app/client/src/api/interceptors/request/addVersionHeader.ts (1 hunks)
  • app/client/src/api/interceptors/request/apiRequestInterceptor.ts (2 hunks)
  • app/client/src/api/interceptors/response/failureHandlers/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/src/api/interceptors/request/apiRequestInterceptor.ts
  • app/client/src/api/interceptors/response/failureHandlers/index.ts
🔇 Additional comments (2)
app/client/src/api/interceptors/request/addVersionHeader.ts (2)

1-1: LGTM: Proper type import from axios

The import statement correctly uses type import for InternalAxiosRequestConfig, ensuring type safety without runtime overhead.


1-13: Verify version header handling across the application

The PR objectives mention that the server will reject requests with mismatched versions. Let's verify the error handling implementation.

✅ Verification successful

Version mismatch handling is properly implemented

The codebase has comprehensive version mismatch handling:

  • Server version check in handleBadRequestError.ts
  • User-friendly version mismatch notification with refresh option
  • Analytics tracking for version updates
  • Non-blocking UX with user-controlled refresh action
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for version mismatch error handling in the client codebase
echo "Searching for version mismatch error handling..."
rg -l "VERSION_MISMATCH" --type ts

# Search for the header usage in interceptors
echo "Checking header usage in interceptors..."
rg -l "X-Appsmith-Version" --type ts

# Look for version-related error handling
echo "Checking version-related error handling..."
ast-grep --pattern 'catch ($err) { $$$ }' --type ts

Length of output: 884


Script:

#!/bin/bash
# Let's check the version mismatch error handling implementation
echo "Checking version mismatch handler implementation..."
rg -A 10 "VERSION_MISMATCH" app/client/src/api/interceptors/response/failureHandlers/handleBadRequestError.ts

# Check version update prompt implementation
echo "Checking version update prompt implementation..."
rg -A 10 "VERSION_MISMATCH" app/client/src/sagas/WebsocketSagas/versionUpdatePrompt.ts

# Check version-related error messages
echo "Checking version-related error messages..."
rg -A 2 "VERSION_MISMATCH" app/client/src/ce/constants/messages.ts

# Look for error handling in interceptors
echo "Checking error handling in interceptors..."
rg -A 10 "catch" app/client/src/api/interceptors

Length of output: 1950

@sharat87 sharat87 merged commit 381dcaa into release Nov 27, 2024
42 checks passed
@sharat87 sharat87 deleted the chore/versioned-responses branch November 27, 2024 07:28
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Dec 2, 2024
We're adding `x-appsmith-version` to every request from the client. If
server sees a request with a `x-appsmith-version` that doesn't match its
own, it rejects. If the server sees a request _without_ any
`x-appsmith-version` header, we don't reject it... for now.

On the client, when server responds with the "version mismatch" error,
we trigger the "There's a new version, please refresh" toast UI flow.

This is a step towards removing our dependency on websockets and
client&mdash;RTS connection.

## 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/12029721517>
> Commit: c4321f7
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12029721517&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Sanity`
> Spec:
> <hr>Tue, 26 Nov 2024 12:28:32 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Introduced a custom header for API requests to include application
version.
- Added specific error handling for 400 Bad Request errors, enhancing
user feedback on version mismatches.

- **Improvements**
- Enhanced error handling for version mismatches, providing clearer
error messages.
- Simplified version update handling, focusing on logging and user
prompts without state management.
- Improved default handling for application version configuration to
prevent undefined values.

- **Chores**
- Removed outdated version state management functions to streamline
code.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Pawan Kumar <[email protected]>
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Dec 2, 2024
We're adding `x-appsmith-version` to every request from the client. If
server sees a request with a `x-appsmith-version` that doesn't match its
own, it rejects. If the server sees a request _without_ any
`x-appsmith-version` header, we don't reject it... for now.

On the client, when server responds with the "version mismatch" error,
we trigger the "There's a new version, please refresh" toast UI flow.

This is a step towards removing our dependency on websockets and
client&mdash;RTS connection.

## 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/12029721517>
> Commit: c4321f7
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12029721517&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Sanity`
> Spec:
> <hr>Tue, 26 Nov 2024 12:28:32 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Introduced a custom header for API requests to include application
version.
- Added specific error handling for 400 Bad Request errors, enhancing
user feedback on version mismatches.

- **Improvements**
- Enhanced error handling for version mismatches, providing clearer
error messages.
- Simplified version update handling, focusing on logging and user
prompts without state management.
- Improved default handling for application version configuration to
prevent undefined values.

- **Chores**
- Removed outdated version state management functions to streamline
code.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Pawan Kumar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants