Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
| -- Keep future tables independent of the MySQL server default. Existing legacy Flowable tables are | ||
| -- converted by FlowableCharsetMigration while their foreign-key checks are safely suspended. | ||
| ALTER DATABASE CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; |
There was a problem hiding this comment.
This appends the database-default change to the already-applied 2.1.0 migration, violating the repository requirement that migrations are append-only and new work must use a new version. Deployments that already recorded 2.1.0 in SERVER_CHANGE_LOG will not execute this statement, leaving the legacy database default unchanged. The new v210 data-migration hook in openmetadata-service/src/main/java/org/openmetadata/service/migration/mysql/v210/Migration.java:42 has the same versioning problem and should move with it.
Context Used: CLAUDE.md (source)
| AND LEFT(UPPER(flowable_table.TABLE_NAME), 4) = 'ACT_' | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM information_schema.TABLES AS candidate_table |
There was a problem hiding this comment.
The name-prefix test treats every base table beginning with ACT_ as a Flowable table. If the OpenMetadata database contains a customer or extension table such as ACT_AUDIT, drift in any matching table causes it to be forcibly converted to utf8mb4_0900_ai_ci, even though non-Flowable tables are intended to remain untouched. Use the canonical Flowable table set or otherwise verify ownership instead of relying only on the prefix.
Code Review ✅ Approved 1 closed / 1 findings🔴 High risk Pins MySQL charset to ✅ 1 closed✅ Quality: DB default collation diverges from Flowable table collation
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
||
| final int converted = FlowableCharsetMigration.alignFlowableTableCharsets(handle); | ||
|
|
||
| assertEquals(2, converted); | ||
| final InOrder statements = inOrder(handle); | ||
| statements.verify(handle).execute("SET SESSION FOREIGN_KEY_CHECKS = 0"); | ||
| statements.verify(handle).execute(conversionStatement("ACT_GE_BYTEARRAY")); | ||
| statements.verify(handle).execute(conversionStatement("ACT_RE_DEPLOYMENT")); | ||
| statements.verify(handle).execute("SET SESSION FOREIGN_KEY_CHECKS = 1"); | ||
| } | ||
|
|
||
| @Test | ||
| void restoresTheOriginalForeignKeySettingWhenConversionFails() { | ||
| final Handle handle = handleWithTables(List.of("ACT_GE_BYTEARRAY"), 1); | ||
| when(handle.execute(conversionStatement("ACT_GE_BYTEARRAY"))) | ||
| .thenThrow(new IllegalStateException("conversion failed")); | ||
|
|
||
| assertThrows( | ||
| IllegalStateException.class, | ||
| () -> FlowableCharsetMigration.alignFlowableTableCharsets(handle)); | ||
|
|
||
| verify(handle).execute("SET SESSION FOREIGN_KEY_CHECKS = 1"); | ||
| } | ||
|
|
||
| @Test | ||
| void preservesAnAlreadyDisabledForeignKeySetting() { | ||
| final Handle handle = handleWithTables(List.of("ACT_GE_BYTEARRAY"), 0); | ||
|
|
||
| FlowableCharsetMigration.alignFlowableTableCharsets(handle); | ||
|
|
||
| verify(handle, times(2)).execute("SET SESSION FOREIGN_KEY_CHECKS = 0"); | ||
| } | ||
|
|
||
| @Test | ||
| void skipsSessionChangesWhenEveryFlowableTableIsAlreadyUtf8mb4() { | ||
| final Handle handle = handleWithTables(List.of(), 1); | ||
|
|
||
| assertEquals(0, FlowableCharsetMigration.alignFlowableTableCharsets(handle)); | ||
|
|
||
| verify(handle, never()).execute(anyString()); | ||
| } | ||
|
|
||
| private static Handle handleWithTables( | ||
| final List<String> tableNames, final int foreignKeyChecks) { | ||
| final Handle handle = mock(Handle.class, RETURNS_DEEP_STUBS); |
There was a problem hiding this comment.
These tests assert Mockito interaction order, exact SQL calls, and invocation counts instead of observable database outcomes. This violates the repository directive to test real behavior and avoid internal verify() assertions. The requirement must be satisfied before merging because these checks can pass even when the migration leaves the schema, constraints, or session setting in the wrong state.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Describe your changes:
Fixes #28255
Pins the OpenMetadata MySQL database default to
utf8mb4for fresh installs and upgrades. The upgrade also aligns all FlowableACT_*tables as one foreign-key-connected set while preserving the session'sFOREIGN_KEY_CHECKS, preventing legacyutf8mb3schemas from failing during restore.Type of change:
High-level design:
utf8mb4_unicode_ciexplicitly in the MySQL bootstrap scripts and current native migration.utf8mb4_0900_ai_cicollation.ACT_*family because changing only one side of a string foreign key is rejected by MySQL.Tests:
Use cases covered
utf8mb4default.utf8mb3Flowable tables with foreign keys migrate successfully without weakening the constraints.Unit tests
FlowableCharsetMigrationTest.java,FlowableCharsetMigrationMySqlTest.javaBackend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
mvn -pl openmetadata-service -Dtest=IngestionPipelineMigrationEntryPointTest,FlowableCharsetMigrationTest,FlowableCharsetMigrationMySqlTest testagainst MySQL 8; all 9 tests passed.mvn -pl openmetadata-service spotless:check,git diff --check, andmake harness-checksuccessfully.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Open workspace in Conductor
The PR is not yet safe to merge because it still modifies an already-applied migration, so existing installations that recorded v210 will not execute the new database-default or Flowable alignment work.
Findings
Summary
This PR pins new MySQL databases to
utf8mb4_unicode_ciand adds a migration utility that converts the foreign-key-connected Flowable table family while preserving the session’s foreign-key-check setting.ACT_prefix selection remains outstanding.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Run v210 schema migration] --> B[Set database default to utf8mb4_unicode_ci] B --> C[Run v210 data migration] C --> D{Any ACT_* table drift?} D -- No --> E[No-op] D -- Yes --> F[Read session FOREIGN_KEY_CHECKS] F --> G[Disable foreign-key checks] G --> H[Convert selected ACT_* tables] H --> I[Restore original session setting]Reviews (4) · Last reviewed commit: "Align Flowable tables with database coll..."