chore: added compatibility for no artifact json type attribute in metadata.json#41156
Conversation
WalkthroughThis change introduces improved error handling in the process of identifying artifact types from a Git repository. A new method in the GitFSServiceCEImpl class manages specific file system errors, and the CommonGitFileUtilsCE class now returns more precise error codes and messages for missing metadata or attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GitFSServiceCEImpl
participant CommonGitFileUtilsCE
Caller->>GitFSServiceCEImpl: obtainArtifactTypeFromGitRepository()
GitFSServiceCEImpl->>CommonGitFileUtilsCE: getArtifactJsonTypeOfRepository()
CommonGitFileUtilsCE-->>GitFSServiceCEImpl: ArtifactType or error
alt Error occurs
GitFSServiceCEImpl->>GitFSServiceCEImpl: handleErrorWhileArtifactTypeIdentification(error)
alt Error is GIT_FILE_SYSTEM_ERROR
GitFSServiceCEImpl-->>Caller: Mono.just(ArtifactType.APPLICATION)
else Other error
GitFSServiceCEImpl-->>Caller: Propagate original error
end
else Success
GitFSServiceCEImpl-->>Caller: ArtifactType
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
880-881: Consider error type consistency.The descriptive error message is an improvement, but there's a slight inconsistency: missing metadata uses
NO_RESOURCE_FOUND(line 871) while missingartifactJsonTypeattribute usesGIT_FILE_SYSTEM_ERROR. Both represent missing file system resources.Consider using consistent error types:
- return Mono.error(new AppsmithException( - AppsmithError.GIT_FILE_SYSTEM_ERROR, "No artifactJsonType attribute found")); + return Mono.error(new AppsmithException( + AppsmithError.NO_RESOURCE_FOUND, "artifactJsonType attribute"));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: brayn003
PR: appsmithorg/appsmith#40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/index.mdc:0-0
Timestamp: 2025-07-21T07:25:40.986Z
Learning: Pull request titles must follow the Conventional Commits specification (e.g., type(scope): description)
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/README.md:0-0
Timestamp: 2025-07-21T07:25:06.064Z
Learning: Pull request titles must follow semantic conventions as described in 'semantic-pr.md'
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: sharat87
PR: appsmithorg/appsmith#30252
File: deploy/docker/fs/usr/lib/python3/dist-packages/supervisor/appsmith_supervisor_stdout.py:21-29
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The user has confirmed that the suggested changes to handle potential exceptions and improve the robustness of the `main` function in `appsmith_supervisor_stdout.py` are acceptable.
Learnt from: sharat87
PR: appsmithorg/appsmith#39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
Learnt from: brayn003
PR: appsmithorg/appsmith#38681
File: app/client/src/git/requests/fetchRefsRequest.ts:11-21
Timestamp: 2025-01-15T20:46:28.171Z
Learning: Error handling through try-catch blocks is handled at a different layer and should not be added to API request functions in the Git module.
Learnt from: brayn003
PR: appsmithorg/appsmith#37830
File: app/client/packages/git/src/actions/checkoutBranchActions.ts:11-18
Timestamp: 2024-11-29T05:38:54.262Z
Learning: In the Git Redux actions (`app/client/packages/git/src/actions`), the `createSingleArtifactAction` function already handles loading state and error management, so additional checks in action creators are unnecessary.
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/updateLocalProfileSaga.ts:35-40
Timestamp: 2024-12-10T10:52:51.127Z
Learning: In `app/client/src/git/sagas/updateLocalProfileSaga.ts`, it's acceptable to cast `error` to `string` when passing it to `gitArtifactActions.updateLocalProfileError`.
Learnt from: sharat87
PR: appsmithorg/appsmith#37829
File: app/client/packages/rts/src/ctl/backup/links/GitStorageLink.ts:12-20
Timestamp: 2024-11-29T09:32:52.382Z
Learning: In backup link classes like `GitStorageLink`, exceptions in `doBackup` methods are already handled externally, so adding try-catch blocks within these methods is unnecessary.
Learnt from: brayn003
PR: appsmithorg/appsmith#38681
File: app/client/src/git/constants/enums.ts:2-4
Timestamp: 2025-01-16T14:25:46.177Z
Learning: The GitArtifactType enum in TypeScript uses lowercase plural values: "applications", "packages", and "workflows" for Application, Package, and Workflow respectively.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (10)
Learnt from: sharat87
PR: #39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
Learnt from: brayn003
PR: #38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In app/client/src/git/sagas/fetchLocalProfileSaga.ts and similar Git sagas, error handling for baseArtifactId is managed outside the scope, so validation checks for baseArtifactId within the saga functions are unnecessary.
Learnt from: brayn003
PR: #37830
File: app/client/packages/git/src/actions/checkoutBranchActions.ts:11-18
Timestamp: 2024-11-29T05:38:54.262Z
Learning: In the Git Redux actions (app/client/packages/git/src/actions), the createSingleArtifactAction function already handles loading state and error management, so additional checks in action creators are unnecessary.
Learnt from: brayn003
PR: #38060
File: app/client/src/git/sagas/updateLocalProfileSaga.ts:35-40
Timestamp: 2024-12-10T10:52:51.127Z
Learning: In app/client/src/git/sagas/updateLocalProfileSaga.ts, it's acceptable to cast error to string when passing it to gitArtifactActions.updateLocalProfileError.
Learnt from: brayn003
PR: #37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The fetchGitMetadataRequest function in app/client/src/git/requests/fetchGitMetadataRequest.ts does not require additional error handling or timeout configuration.
Learnt from: sharat87
PR: #37672
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:251-258
Timestamp: 2024-11-25T10:19:42.548Z
Learning: In the JavaScript file deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs, explicit error handling for file operations within the finalizeHtmlFiles function is not necessary because failures will show up as exceptions anyway.
Learnt from: sondermanish
PR: #36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the ImportServiceCEImpl class, when throwing new AppsmithException instances, avoid adding the original Throwable as the cause due to messaging constraints.
Learnt from: sondermanish
PR: #36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the ImportServiceCEImpl class, when throwing new AppsmithException instances, avoid adding the original Throwable as the cause due to messaging constraints.
Learnt from: nidhi-nair
PR: #29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The updateJsLibsInContext method in the ApplicationJsLibServiceCEImpl class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.
Learnt from: nidhi-nair
PR: #29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The updateJsLibsInContext method in the ApplicationJsLibServiceCEImpl class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (11)
Learnt from: sharat87
PR: #39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
Learnt from: sondermanish
PR: #36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the ImportServiceCEImpl class, when throwing new AppsmithException instances, avoid adding the original Throwable as the cause due to messaging constraints.
Learnt from: sondermanish
PR: #36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the ImportServiceCEImpl class, when throwing new AppsmithException instances, avoid adding the original Throwable as the cause due to messaging constraints.
Learnt from: brayn003
PR: #38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In app/client/src/git/sagas/fetchLocalProfileSaga.ts and similar Git sagas, error handling for baseArtifactId is managed outside the scope, so validation checks for baseArtifactId within the saga functions are unnecessary.
Learnt from: sharat87
PR: #37829
File: app/client/packages/rts/src/ctl/backup/links/BackupFolderLink.ts:20-25
Timestamp: 2024-11-29T09:32:36.096Z
Learning: In the Appsmith backup process, exceptions are caught outside of the postBackup methods in the backup links, so adding additional error handling within these methods is unnecessary.
Learnt from: brayn003
PR: #37984
File: app/client/src/git/requests/updateGlobalConfigRequest.ts:9-13
Timestamp: 2024-12-05T10:57:15.397Z
Learning: In the TypeScript files within app/client/src/git/requests/, functions should not include internal error handling or request timeouts; they should allow errors to be thrown directly.
Learnt from: brayn003
PR: #38060
File: app/client/src/git/sagas/updateLocalProfileSaga.ts:35-40
Timestamp: 2024-12-10T10:52:51.127Z
Learning: In app/client/src/git/sagas/updateLocalProfileSaga.ts, it's acceptable to cast error to string when passing it to gitArtifactActions.updateLocalProfileError.
Learnt from: brayn003
PR: #37830
File: app/client/packages/git/src/actions/checkoutBranchActions.ts:11-18
Timestamp: 2024-11-29T05:38:54.262Z
Learning: In the Git Redux actions (app/client/packages/git/src/actions), the createSingleArtifactAction function already handles loading state and error management, so additional checks in action creators are unnecessary.
Learnt from: brayn003
PR: #38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: In the TypeScript file app/client/src/git/sagas/deleteBranchSaga.ts, within the deleteBranchSaga function, error handling is managed outside the scope of the catch block. Therefore, casting error to string in this context is acceptable.
Learnt from: sharat87
PR: #37829
File: app/client/packages/rts/src/ctl/backup/links/GitStorageLink.ts:12-20
Timestamp: 2024-11-29T09:32:52.382Z
Learning: In backup link classes like GitStorageLink, exceptions in doBackup methods are already handled externally, so adding try-catch blocks within these methods is unnecessary.
Learnt from: brayn003
PR: #38681
File: app/client/src/git/constants/enums.ts:2-4
Timestamp: 2025-01-16T14:25:46.177Z
Learning: The GitArtifactType enum in TypeScript uses lowercase plural values: "applications", "packages", and "workflows" for Application, Package, and Workflow respectively.
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
870-871: LGTM! Improved error specificity for missing metadata.The change from generic
GIT_FILE_SYSTEM_ERRORtoNO_RESOURCE_FOUNDwith the specific resource name provides better error categorization and clearer identification of the missing resource.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
185-192: LGTM! Well-designed error recovery strategy.The method provides a clean fallback mechanism by defaulting to
APPLICATIONwhen artifact type identification fails due to file system errors. The pattern matching on exception type and error code is appropriate, and proper error propagation is maintained for other error types.
200-204: Ignore error-code mismatch—no update neededI checked
CommonGitFileUtilsCE.getArtifactJsonTypeOfRepositoryin
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
and found no references to eitherNO_RESOURCE_FOUNDorGIT_FILE_SYSTEM_ERROR. The method simply propagates errors viaMono.error(error)—it doesn’t map missing-metadata cases to any specific AppsmithError. As a result, there’s no mismatch to address in your handler.Likely an incorrect or invalid review comment.
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/16641904067
Commit: 83fbbcb
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 31 Jul 2025 07:34:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit