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.

Sign up for GitHub

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: bump DSL version to 91 and update migration logic #38516

Merged
merged 3 commits into from
Jan 7, 2025
Merged

chore: bump DSL version to 91 and update migration logic #38516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c553bdd
chore: bump DSL version to 91 and update migration logic
rahulbarwal Jan 7, 2025
b526e61
feat: add migration for version 90 in DSLMigration tests
rahulbarwal Jan 7, 2025
593cba7
added client side migration
sondermanish Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion app/client/packages/dsl/src/migrate/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ import { migrateCustomWidgetDynamicHeight } from "./migrations/088-migrate-custo
import { migrateTableWidgetV2CurrentRowInValidationsBinding } from "./migrations/089-migrage-table-widget-v2-currentRow-binding";
import type { DSLWidget } from "./types";

export const LATEST_DSL_VERSION = 90;
export const LATEST_DSL_VERSION = 91;

export const calculateDynamicHeight = () => {
const DEFAULT_GRID_ROW_HEIGHT = 10;
Expand Down Expand Up @@ -613,6 +613,17 @@ const migrateVersionedDSL = async (currentDSL: DSLWidget, newPage = false) => {

if (currentDSL.version === 89) {
currentDSL = migrateTableWidgetV2CurrentRowInValidationsBinding(currentDSL);
currentDSL.version = 90;
}

if (currentDSL.version === 90) {
/**
* This is just a version bump without any migration
* History: With this PR: https://github.com/appsmithorg/appsmith/pull/38391
* we updated the `clientVersion` to 2.
* What we missed was that, the auto-commit does not handle clientVersion, which lead to this bug: https://github.com/appsmithorg/appsmith/issues/38511
* We are bumping this version to make sure that the auto-commit will handle this version bump.
*/
currentDSL.version = LATEST_DSL_VERSION;
}

Expand Down
4 changes: 4 additions & 0 deletions app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,10 @@ const migrations: Migration[] = [
],
version: 89,
},
{
functionLookup: [],
version: 90,
},
Comment on lines +933 to +936
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

];

const mockFnObj: Record<number, any> = {};
Expand Down
26 changes: 25 additions & 1 deletion ...ver/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,35 @@ public Mono<ApplicationJson> migrateApplicationJsonToLatestSchema(
return Mono.empty();
}

return migrateServerSchema(applicationJson, baseApplicationId, refName);
return migrateClientSchema(appJson, baseApplicationId, refName)
.flatMap(clientJson -> migrateServerSchema(clientJson, baseApplicationId, refName));
})
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.INCOMPATIBLE_IMPORTED_JSON)));
}

private Mono<ApplicationJson> migrateClientSchema(
ApplicationJson applicationJson, String baseApplicationId, String branchName) {

Mono<ApplicationJson> migrateApplicationJsonMono = Mono.just(applicationJson);
if (jsonSchemaVersions.getClientVersion().equals(applicationJson.getClientSchemaVersion())) {
return migrateApplicationJsonMono;
}

// 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);
case 1:
applicationJson.setClientSchemaVersion(2);
default:
}
Comment on lines +88 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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:
}


applicationJson.setClientSchemaVersion(jsonSchemaVersions.getClientVersion());
return migrateApplicationJsonMono;
}

/**
* This method may be moved to the publisher chain itself
*
Expand Down
Loading