chore: bump DSL version to 91 and update migration logic #38516
Conversation
This commit updates the LATEST_DSL_VERSION constant to 91 and modifies the migration logic to ensure proper handling of the new version. The change addresses an issue with auto-commit functionality not managing the clientVersion correctly, as detailed in the linked issue. No migrations are introduced with this version bump, ensuring backward compatibility. Related issue: #38511
rahulbarwal
requested review from
sondermanish and
brayn003
and removed request for
a team
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Poem
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
Bug
![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/client/packages/dsl/src/migrate/index.ts (1)
619-626: Well-documented version migration block.The comments effectively explain the context and reason for this version bump. Consider adding a note about the impact (if any) on existing DSLs when they encounter this migration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/dsl/src/migrate/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/packages/dsl/src/migrate/index.ts (2)
96-96: LGTM: Version bump is consistent with PR objectives.
96-96: Verify version consistency across the codebase.Let's ensure there are no other files that need to be updated with this version bump.
Also applies to: 619-626
✅ Verification successful
Version references are consistent
All version references are properly contained within the migration logic file. No other files need updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of version 90 or 91 in the codebase # that might need to be updated rg -g '!*.{log,lock}' -g '!package-lock.json' '(DSL_VERSION.*9[01]|version.*9[01])'Length of output: 92703
This commit introduces a new migration entry for version 90 in the DSLMigration test suite, ensuring that the migration logic accommodates the latest version. The addition includes an empty function lookup array, preparing for future migration functionalities.
github-actions
bot
added
Bug
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (1)
933-936: Verify the migration logic for version 90.While the test verifies that migrations are executed and the count matches
LATEST_DSL_VERSION, it doesn't validate the actual migration logic for version 90 mentioned in the PR objectives (auto-commit functionality).✅ Verification successful
Migration logic for version 90 is correctly implemented
Version 90 is intentionally a version bump without migration logic, specifically designed to fix auto-commit handling of clientVersion (issue #38511). The empty functionLookup in the test is appropriate for this case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the actual migration logic implementation rg -A 5 "case 90:" app/client/packages/dsl/src/migrate/Length of output: 57
Script:
#!/bin/bash # Find migration-related files fd migration -t f app/client/packages/dsl/src/migrate/ # Search for version 90 in migration files rg -i "version.*90|90.*version" app/client/packages/dsl/src/migrate/ # Search for auto-commit related changes rg -i "autocommit|auto.?commit" app/client/packages/dsl/src/migrate/Length of output: 2123
Script:
#!/bin/bash # Get the version 90 related implementation rg -B 5 -A 10 "version = 90" app/client/packages/dsl/src/migrate/index.ts # Get the context around auto-commit comment rg -B 5 -A 10 "auto-commit does not handle" app/client/packages/dsl/src/migrate/index.tsLength of output: 1462
| { | ||
| functionLookup: [], | ||
| version: 90, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Follow established pattern for skipped migration 90
The empty functionLookup array breaks consistency with other skipped migrations in the codebase. Versions 47 and 50 use a clear pattern with skippedMigrationX function names and documentation. Update version 90 to match this pattern:
{
- functionLookup: [],
+ functionLookup: [{
+ moduleObj: "",
+ functionName: "skippedMigration90",
+ }],
version: 90,
},• Add a comment above the migration explaining why version 90 is skipped, similar to the existing comment for version 50.
🔗 Analysis chain
Document the reason for skipping migration 90.
The empty functionLookup array suggests this is a skipped migration. While this pattern is used elsewhere (e.g., skippedMigration47, skippedMigration50), those cases include a placeholder function name. Consider adding a comment or placeholder to maintain consistency with other skipped migrations.
Apply this diff to maintain consistency:
{
- functionLookup: [],
+ functionLookup: [{
+ moduleObj: "",
+ functionName: "skippedMigration90",
+ }],
version: 90,
},Let's verify if there are any other skipped migrations that follow this pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find other migrations with empty functionLookup
ast-grep --pattern 'functionLookup: [], version: $_'
Length of output: 54
Script:
#!/bin/bash
# Find skipped migrations and their patterns
rg "skippedMigration\d+" -A 2 app/client/packages/dsl/src/migrate/tests/
# Find all functionLookup patterns
ast-grep --pattern '{
functionLookup: $_,
version: $_
}'
# Look for migration documentation
rg -i "migration|version" app/client/packages/dsl/src/migrate/**/README.md
Length of output: 65854
Script:
#!/bin/bash
# Find all empty functionLookup arrays
rg "functionLookup: \[\]," app/client/packages/dsl/src/migrate/tests/
# Find all skipped migrations
rg "skippedMigration\d+" -A 3 app/client/packages/dsl/src/migrate/tests/
Length of output: 1253
github-actions
bot
added
Bug
github-actions
bot
added
Bug
sondermanish
requested review from
sharat87,
abhvsn and
AnaghHegde
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (1)
85-94: Simplify client schema migration logicSince there are no specific migration steps between versions, consider simplifying the method by removing the switch case and directly setting the version to the latest.
Apply this diff to simplify the method:
- // Run migration linearly - // Updating the schema version after each migration is not required as we are not exiting by breaking the switch - // cases, but this keeps the version number and the migration in sync - switch (applicationJson.getClientSchemaVersion()) { - case 0: - applicationJson.setClientSchemaVersion(1); - break; - case 1: - applicationJson.setClientSchemaVersion(2); - break; - default: - // No action needed - } + // No migrations needed, set to the latest client schema version + applicationJson.setClientSchemaVersion(jsonSchemaVersions.getClientVersion());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (1)
71-72: Good addition of client schema migration stepIntroducing
migrateClientSchemabeforemigrateServerSchemaensures that the client schema is up-to-date prior to server-side migrations. This sequence is crucial for maintaining data integrity during the migration process.
| switch (applicationJson.getClientSchemaVersion()) { | ||
| case 0: | ||
| applicationJson.setClientSchemaVersion(1); | ||
| case 1: | ||
| applicationJson.setClientSchemaVersion(2); | ||
| default: | ||
| } |
There was a problem hiding this comment.
Add missing 'break' statements in switch cases to prevent fall-through
The switch cases in migrateClientSchema lack break; statements, which causes unintended fall-through to subsequent cases. This can lead to incorrect version increments.
Apply this diff to add break; statements:
switch (applicationJson.getClientSchemaVersion()) {
case 0:
applicationJson.setClientSchemaVersion(1);
+ break;
case 1:
applicationJson.setClientSchemaVersion(2);
+ break;
default:
// No action needed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (applicationJson.getClientSchemaVersion()) { | |
| case 0: | |
| applicationJson.setClientSchemaVersion(1); | |
| case 1: | |
| applicationJson.setClientSchemaVersion(2); | |
| default: | |
| } | |
| switch (applicationJson.getClientSchemaVersion()) { | |
| case 0: | |
| applicationJson.setClientSchemaVersion(1); | |
| break; | |
| case 1: | |
| applicationJson.setClientSchemaVersion(2); | |
| break; | |
| default: | |
| } |
Failed server tests
|
sondermanish
deleted the
rahulbarwal/git-auto-commit-handling-of-clientversion-upgrade-bump
branch
Description
This commit updates the LATEST_DSL_VERSION constant to 91 and modifies the migration logic to ensure proper handling of the new version. The change addresses an issue with auto-commit functionality not managing the clientVersion correctly, as detailed in the linked issue. No migrations are introduced with this version bump, ensuring backward compatibility.
Fixes #
Issue Numberor
Fixes #38511
Warning
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.ImportExport, @tag.Git, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12651263937
Commit: 593cba7
Cypress dashboard.
Tags:
@tag.ImportExport, @tag.Git, @tag.SanitySpec:
Tue, 07 Jan 2025 13:13:15 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit