Repository navigation
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughAdds a new schema normalization analyzer with LLM integration, DDL/migration/triggers generators, topological sorting and validators; refactors analyzer base (AbstractRepositoryAnalyzer), improves AI response parsing and error handling, adds OpenRouter AI provider, enhances Liquibase/Indexes parsing, and introduces extensive tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer as SchemaNormalizationAnalyzer
participant Profiler as Entity Profiler
participant LLM as AI Service
participant Validator as DataMigrationPlanValidator
participant DDL as NormalizedTableDDLGenerator
participant Migration as DataMigrationGenerator
participant Triggers as InsteadOfTriggerGenerator
participant Liquibase as LiquibaseGenerator
Analyzer->>Profiler: collect EntityProfile batch
Profiler-->>Analyzer: List<EntityProfile>
Analyzer->>LLM: send batch for normalization analysis
LLM-->>Analyzer: JSON with NormalizationIssues + DataMigrationPlan
Analyzer->>Validator: validate DataMigrationPlan (DAG, refs, coverage)
Validator-->>Analyzer: validation result / throw on error
Analyzer->>DDL: generate CREATE TABLE DDL (topological)
DDL-->>Analyzer: DDL artifacts
Analyzer->>Migration: generate INSERT...SELECT statements
Migration-->>Analyzer: migration SQL
Analyzer->>Triggers: generate INSTEAD OF triggers + rollbacks
Triggers-->>Analyzer: trigger DDL per dialect
Analyzer->>Liquibase: assemble/write changesets (DDL→migration→FK drops→views→triggers)
Liquibase-->>Analyzer: WriteResult (files, master updates)
Analyzer-->>Analyzer: write mapping artifact JSON and generated entities
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
codeant-ai
Bot
added
the
size:XXL
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the schema-normalization/migration tooling (DDL generation, migration SQL, trigger generation, plan validation) and improves AI-provider robustness/configurability (OpenRouter support, better JSON extraction/parsing, analyzer refactor), along with associated unit tests and documentation.
Changes:
- Add schema-normalization utilities: topological sort, normalized-table DDL generation, data-migration SQL generation, migration-plan validation, and INSTEAD OF trigger generation (with tests/docs).
- Extend Liquibase/index tooling: parse FK constraints from Liquibase XML, improve multi-dialect index rollback/drop behavior, and make changes file naming configurable.
- Improve AI layer: extract JSON from model responses, throw explicit parsing exceptions, add OpenRouter provider, and refactor repository analysis into
AbstractRepositoryAnalyzer.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/sa/com/cloudsolutions/liquibase/IndexesTest.java | Adds FK-constraint parsing/lookup tests for Indexes.loadAll() and helpers. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorterTest.java | New unit tests for topological sorting and cycle detection. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java | New tests for normalized table DDL generation (Liquibase + raw SQL). |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java | Extends tests for MySQL drop-index behavior, file prefixing, and new changeset helpers. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGeneratorTest.java | New tests for generated INSTEAD OF triggers and rollback DDL. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java | New tests for validating migration plans (base table, cycles, coverage warnings). |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.java | New tests for migration INSERT ordering and SQL shape. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java | Updates invalid-JSON expectations to align with new exception behavior. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.java | New tests for robust JSON extraction from AI responses. |
| src/main/resources/ai-prompts/normalization-system-prompt.md | Adds schema-normalization system prompt for LLM-guided analysis output format. |
| src/main/java/sa/com/cloudsolutions/liquibase/Indexes.java | Adds FK-constraint extraction, combined parse result (LoadResult), and FK lookup helper. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorter.java | New utility implementing Kahn’s algorithm for FK dependency ordering. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java | New generator for CREATE TABLE output in Liquibase or raw SQL modes. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | Refactors config/result types, adds new changeset helpers, improves multi-dialect SQL/rollback, and file naming prefix. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java | New generator for dialect-specific INSTEAD OF triggers + rollback DDL. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java | New validation for migration plan consistency and dependency DAG. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.java | New generator for INSERT-SELECT migration SQL in FK order. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java | Switches quiet-mode handling to new analyzer base; adds preprocessing of test sources. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java | Refactors to extend AbstractRepositoryAnalyzer and uses Indexes.loadAll(). |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.java | New OpenRouter AI provider implementation extending OpenAI-compatible API. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java | Minor parsing refactor; uses shared parseRecommendations. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java | Expands Mockito stubbing detection to include given. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java | New reusable analyzer base class (filtering, checkpoints, shared state). |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java | Adds robust JSON extraction and changes parse behavior to throw on invalid AI responses. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.java | Adds “openrouter” provider option and updates error message. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AIResponseException.java | New exception type for malformed/unparseable AI responses. |
| docs/schema-normalization-task-list.md | Adds implementation checklist for schema normalization workflow. |
| docs/schema-normalization-improvement-plan.md | Adds detailed multi-phase improvement plan for normalization pipeline. |
| docs/schema-normalization-implementation-plan.md | Adds task-oriented implementation plan for an agent to execute. |
| .gitignore | Ignores depsolver.yml. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+548
to
549
| if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) { | ||
| logger.warn("No valid JSON found in AI response"); |
There was a problem hiding this comment.
The early-return for empty JSON is gated by logger.isWarnEnabled(). If warn logging is disabled, an empty/blank jsonResponse will fall through to objectMapper.readTree(jsonResponse) and throw, instead of returning an empty issues list. The emptiness check and return should be unconditional; only the log line should be conditional.
Suggested change
| if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) { | |
| logger.warn("No valid JSON found in AI response"); | |
| if (jsonResponse == null || jsonResponse.trim().isEmpty()) { | |
| if (logger.isWarnEnabled()) { | |
| logger.warn("No valid JSON found in AI response"); | |
| } |
|
|
||
| if (shouldSkipType(fullyQualifiedName)) continue; | ||
|
|
||
| System.out.println("%n" + "=".repeat(80)); |
There was a problem hiding this comment.
System.out.println("%n" + "=".repeat(80)); prints the literal characters %n rather than a newline. Use System.out.println("\n" + "=".repeat(80)) or System.out.printf("%n%s%n", "=".repeat(80)) to get the intended formatting.
Suggested change
| System.out.println("%n" + "=".repeat(80)); | |
| System.out.println("\n" + "=".repeat(80)); |
|
|
||
| for (String profileCol : profileColumns) { | ||
| if (!mappedViewColumns.contains(profileCol)) { | ||
| logger.error("Column '{}' from source profile is not mapped in columnMappings", profileCol); |
There was a problem hiding this comment.
The Javadoc describes the column-coverage check as a warning, but unmapped profile columns are logged with logger.error(...). This should be warn (or lower) to avoid treating expected/allowed omissions as errors, and to align with the documentation and tests' intent.
Suggested change
| logger.error("Column '{}' from source profile is not mapped in columnMappings", profileCol); | |
| logger.warn("Column '{}' from source profile is not mapped in columnMappings", profileCol); |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+548
to
550
| if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) { | ||
| logger.warn("No valid JSON found in AI response"); | ||
| return issues; |
Contributor
There was a problem hiding this comment.
Suggestion: The empty-response check in the JSON parser is currently gated by the logger's warn level, so when the response is blank and warn logging is disabled, the code still attempts to parse an empty string and will throw a JSON parse exception instead of cleanly returning an empty list; the emptiness check should be independent of logging. [logic error]
Severity Level: Major ⚠️
- ❌ AI query optimization crashes on empty fenced responses.
- ⚠️ QueryOptimizationChecker analyze step may fail unexpectedly.
- ⚠️ Downstream tools relying on recommendations receive exceptions.
Suggested change
| if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) { | |
| logger.warn("No valid JSON found in AI response"); | |
| return issues; | |
| if (jsonResponse.trim().isEmpty()) { | |
| if (logger.isWarnEnabled()) { | |
| logger.warn("No valid JSON found in AI response"); | |
| } | |
| return issues; | |
| } |
Steps of Reproduction ✅
1. A caller triggers AI analysis by invoking `AbstractAIService.analyzeQueryBatch(batch)`
in `src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java:86`,
for example via `QueryOptimizationChecker` which calls
`aiService.analyzeQueryBatch(batch)` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java:174`.
2. Use a concrete AI implementation such as `OpenAIService` (`OpenAIService extends
AbstractAIService` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenAIService.java:20`) or
`GeminiAIService` (`GeminiAIService extends AbstractAIService` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java:22`); each
implementation's `parseResponse(...)` delegates to `parseRecommendations(textResponse,
batch)` as seen at `OpenAIService.java:132` and `GeminiAIService.java:217`.
3. Have the upstream AI API return a textual response that contains an empty JSON code
fence, e.g. the raw body equals `````json\n````, so that `parseRecommendations` at
`AbstractAIService.java:677-687` receives a non-empty `textResponse`.
`extractJson(textResponse)` at `AbstractAIService.java:598-632` will trim the response,
detect the last code fence, strip it, and return an empty string (`""`) as `json`.
4. With logging configured at ERROR level or higher so that `logger.isWarnEnabled()`
returns `false`, `parseRecommendations` calls `parseRecommendationsFromJson(json, batch)`
at `AbstractAIService.java:687`, which reaches the condition at
`AbstractAIService.java:548-550`. Because `jsonResponse.trim().isEmpty()` is true but
`logger.isWarnEnabled()` is false, the `if` condition is false overall, so the method
proceeds to `objectMapper.readTree(jsonResponse)` at `AbstractAIService.java:553` with an
empty string, causing a JSON parse exception instead of cleanly returning an empty
`issues` list.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java
**Line:** 548:550
**Comment:**
*Logic Error: The empty-response check in the JSON parser is currently gated by the logger's warn level, so when the response is blank and warn logging is disabled, the code still attempts to parse an empty string and will throw a JSON parse exception instead of cleanly returning an empty list; the emptiness check should be independent of logging.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+865
to
+885
| // Build JOINs in topological order so each referenced parent table is already in scope. | ||
| // This fixes multi-level FK chains (e.g. customer → address → phone). | ||
| List<String> order = TopologicalSorter.sort(view.tables(), view.foreignKeys()); | ||
| // Index FKs by fromTable for O(1) lookup. One table can have multiple outgoing FKs. | ||
| Map<String, List<InsteadOfTriggerGenerator.ForeignKey>> fksByFromTable = new LinkedHashMap<>(); | ||
| for (InsteadOfTriggerGenerator.ForeignKey fk : view.foreignKeys()) { | ||
| fksByFromTable.computeIfAbsent(fk.fromTable(), k -> new ArrayList<>()).add(fk); | ||
| } | ||
| // Skip the first element (the base table, which is already in the FROM clause) | ||
| for (int i = 1; i < order.size(); i++) { | ||
| String table = order.get(i); | ||
| List<InsteadOfTriggerGenerator.ForeignKey> fks = fksByFromTable.get(table); | ||
| if (fks != null) { | ||
| for (InsteadOfTriggerGenerator.ForeignKey fk : fks) { | ||
| sb.append(" JOIN ").append(fk.toTable()) | ||
| .append(" ON ").append(fk.fromTable()).append(".").append(fk.fromColumn()) | ||
| .append(" = ").append(fk.toTable()).append(".").append(fk.toColumn()); | ||
| } | ||
| } | ||
| } | ||
| return sb.toString(); |
Contributor
There was a problem hiding this comment.
Suggestion: In the compatibility view SQL builder, the JOIN clause mistakenly joins the parent table (fk.toTable) instead of the child table (fk.fromTable), so the generated SELECT references columns from child tables that never appear in the FROM/JOIN list, resulting in invalid SQL at runtime when the view is created. [logic error]
Severity Level: Critical 🚨
- ❌ Liquibase createView changeset fails for multi-table normalization plans.
- ❌ Compatibility view for normalized tables never created in database.
- ⚠️ Downstream INSTEAD OF triggers reference missing or invalid view.
- ⚠️ Automated schema normalization migration pipeline cannot be applied.
Suggested change
| // Build JOINs in topological order so each referenced parent table is already in scope. | |
| // This fixes multi-level FK chains (e.g. customer → address → phone). | |
| List<String> order = TopologicalSorter.sort(view.tables(), view.foreignKeys()); | |
| // Index FKs by fromTable for O(1) lookup. One table can have multiple outgoing FKs. | |
| Map<String, List<InsteadOfTriggerGenerator.ForeignKey>> fksByFromTable = new LinkedHashMap<>(); | |
| for (InsteadOfTriggerGenerator.ForeignKey fk : view.foreignKeys()) { | |
| fksByFromTable.computeIfAbsent(fk.fromTable(), k -> new ArrayList<>()).add(fk); | |
| } | |
| // Skip the first element (the base table, which is already in the FROM clause) | |
| for (int i = 1; i < order.size(); i++) { | |
| String table = order.get(i); | |
| List<InsteadOfTriggerGenerator.ForeignKey> fks = fksByFromTable.get(table); | |
| if (fks != null) { | |
| for (InsteadOfTriggerGenerator.ForeignKey fk : fks) { | |
| sb.append(" JOIN ").append(fk.toTable()) | |
| .append(" ON ").append(fk.fromTable()).append(".").append(fk.fromColumn()) | |
| .append(" = ").append(fk.toTable()).append(".").append(fk.toColumn()); | |
| } | |
| } | |
| } | |
| return sb.toString(); | |
| // Build JOINs in topological order so each referenced parent table is already in scope. | |
| // This fixes multi-level FK chains (e.g. customer → address → phone). | |
| List<String> order = TopologicalSorter.sort(view.tables(), view.foreignKeys()); | |
| // Index FKs by fromTable for O(1) lookup. One table can have multiple outgoing FKs. | |
| Map<String, List<InsteadOfTriggerGenerator.ForeignKey>> fksByFromTable = new LinkedHashMap<>(); | |
| for (InsteadOfTriggerGenerator.ForeignKey fk : view.foreignKeys()) { | |
| fksByFromTable.computeIfAbsent(fk.fromTable(), k -> new ArrayList<>()).add(fk); | |
| } | |
| // Skip the first element (the base table, which is already in the FROM clause) | |
| for (int i = 1; i < order.size(); i++) { | |
| String table = order.get(i); | |
| List<InsteadOfTriggerGenerator.ForeignKey> fks = fksByFromTable.get(table); | |
| if (fks != null) { | |
| for (InsteadOfTriggerGenerator.ForeignKey fk : fks) { | |
| sb.append(" JOIN ").append(fk.fromTable()) | |
| .append(" ON ").append(fk.fromTable()).append(".").append(fk.fromColumn()) | |
| .append(" = ").append(fk.toTable()).append(".").append(fk.toColumn()); | |
| } | |
| } | |
| } | |
| return sb.toString(); |
Steps of Reproduction ✅
1. Run the analyzer entry point `SchemaNormalizationAnalyzer.main` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java:1463-1472`,
with `generator.yml` configured so `schema_normalization` is enabled and the AI service
returns normalization plans that split an entity table into multiple normalized tables
with foreign keys (e.g., a base table `patient` and child table `address` with FK
`address.patient_id → patient.id`, matching the ForeignKey semantics in
`InsteadOfTriggerGenerator.ForeignKey` at
`examples/util/InsteadOfTriggerGenerator.java:29-37`).
2. During analysis, the LLM response is parsed in `parseDataMigrationPlan` at
`SchemaNormalizationAnalyzer.java:678-707`, producing a `DataMigrationPlan` whose
`newTables` contains both parent and child tables (e.g., `["patient", "address"]`) and
whose `foreignKeys` contains at least one entry with `fromTable="address"` and
`toTable="patient"`.
3. In `generateMigrationArtifacts` at `SchemaNormalizationAnalyzer.java:903-990`, the plan
is converted to a `ViewDescriptor` via `toViewDescriptor`
(`SchemaNormalizationAnalyzer.java:717-734`), then passed into `buildChangesetsForPlan`
(`SchemaNormalizationAnalyzer.java:1022-1067`), which calls
`buildCompatibilityViewSql(view)` at `SchemaNormalizationAnalyzer.java:1045-1047` to
generate the SELECT used for the Liquibase `<createView>` changeset.
4. Inside `buildCompatibilityViewSql` at `SchemaNormalizationAnalyzer.java:856-885`, for a
foreign key `address.patient_id → patient.id`, the current code appends `FROM patient JOIN
patient ON address.patient_id = patient.id`, joining `fk.toTable()` (the parent) instead
of `fk.fromTable()` (the child). This produces SQL that references the child table
(`address.*`) in the JOIN condition without ever listing `address` in any `FROM`/`JOIN`
clause, so when Liquibase applies the generated `<createView>` changeset via
`LiquibaseGenerator.createViewChangeset` at
`examples/util/LiquibaseGenerator.java:422-436`, the database rejects the view definition
with an error like "relation/address does not exist in FROM clause," and the migration
fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java
**Line:** 865:885
**Comment:**
*Logic Error: In the compatibility view SQL builder, the JOIN clause mistakenly joins the parent table (`fk.toTable`) instead of the child table (`fk.fromTable`), so the generated SELECT references columns from child tables that never appear in the FROM/JOIN list, resulting in invalid SQL at runtime when the view is created.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.java (1)
23-31:⚠️ Potential issue | 🟡 MinorNormalize
providerbefore the switch.
Map.getOrDefault()only falls back when the key is absent. A config entry likeprovider:or"openrouter "still reaches this path asnullor whitespace and fails at startup instead of using the default provider.🛠️ Proposed fix
public static AbstractAIService create(Map<String, Object> config) throws IOException { - String provider = (String) config.getOrDefault("provider", "gemini"); + Object rawProvider = config == null ? null : config.get("provider"); + String provider = rawProvider instanceof String s ? s.trim() : "gemini"; + if (provider.isEmpty()) { + provider = "gemini"; + } return switch (provider.toLowerCase()) { case "gemini" -> new GeminiAIService(); case "openai" -> new OpenAIService(); case "openrouter" -> new OpenRouterService();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.java` around lines 23 - 31, The create method in AIServiceFactory needs to normalize the provider value before the switch: obtain the raw value from config (currently assigned to provider), convert nulls or blank/whitespace-only strings to the default "gemini", then trim and lowercase the resulting string before entering the switch; update the provider variable used in the switch (in the create method) so values like null, "", " openrouter ", or "OpenAI " are normalized and correctly matched to GeminiAIService, OpenAIService, or OpenRouterService, otherwise throw the existing IllegalArgumentException for unknown providers.src/main/java/sa/com/cloudsolutions/liquibase/Indexes.java (1)
201-224:⚠️ Potential issue | 🟠 MajorDropped FKs never leave
fkConstraintMap.This parser records
addForeignKeyConstraint, but there is no matching removal when a later changeSet drops or replaces that constraint. After an add→drop sequence,lookupFKConstraintName()can return a stale name and generate an invalid follow-up migration.🛠️ Proposed fix
private static void processChange(Change change, Map<String, Set<IndexInfo>> result, Map<String, Set<FKConstraintInfo>> fkResult) { if (change instanceof CreateIndexChange cic) { handleCreateIndex(cic, result); } else if (change instanceof AddPrimaryKeyChange apk) { handleAddPrimaryKey(apk, result); } else if (change instanceof AddUniqueConstraintChange auc) { handleAddUniqueConstraint(auc, result); } else if (change instanceof AddForeignKeyConstraintChange afk) { handleAddForeignKey(afk, fkResult); + } else if (change instanceof DropForeignKeyConstraintChange dfk) { + handleDropForeignKey(dfk, fkResult); } else if (change instanceof CreateTableChange ctc) { handleCreateTable(ctc, result); } else if (change instanceof AddColumnChange acc) { handleAddColumn(acc, result); } else if (change instanceof DropIndexChange dic) { @@ private static void handleAddForeignKey(AddForeignKeyConstraintChange afk, Map<String, Set<FKConstraintInfo>> fkResult) { @@ .add(new FKConstraintInfo( orUnknown(constraintName), baseTable, baseColumn.trim(), refTable, isBlank(refColumn) ? null : refColumn.trim())); } + + private static void handleDropForeignKey(DropForeignKeyConstraintChange dfk, + Map<String, Set<FKConstraintInfo>> fkResult) { + String baseTable = dfk.getBaseTableName(); + String constraintName = dfk.getConstraintName(); + if (isBlank(baseTable) || isBlank(constraintName)) { + return; + } + + Set<FKConstraintInfo> fks = fkResult.get(baseTable); + if (fks == null) { + return; + } + + fks.removeIf(fk -> constraintName.equalsIgnoreCase(fk.constraintName())); + if (fks.isEmpty()) { + fkResult.remove(baseTable); + } + }Also applies to: 227-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/liquibase/Indexes.java` around lines 201 - 224, processChange currently records AddForeignKeyConstraintChange into fkResult/fkConstraintMap but never removes entries, causing stale entries; add handling for DropForeignKeyConstraintChange in processChange (mirror the existing else-if chain) and implement a new handler (e.g., handleDropForeignKey) that accepts the DropForeignKeyConstraintChange and the fkResult/fkConstraintMap and removes the corresponding FK entry by constraintName (and by table/columns fallback if constraintName is null), ensuring lookupFKConstraintName reads the updated map; also apply the same removal logic in any other places that can drop or replace FKs (e.g., CreateTable/DropTable handlers) so dropped/renamed FKs are removed from fkResult/fkConstraintMap.src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java (2)
1205-1210:⚠️ Potential issue | 🟠 MajorCheckpoint cleanup happens before Liquibase generation.
analyze()clears the checkpoint before it returns, so a failure ingenerateLiquibaseChangesFile()here forces a full re-analysis on the next run. Move file generation into the analyzer lifecycle, or only clear the checkpoint after this step succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java` around lines 1205 - 1210, The analyzer currently clears its checkpoint inside QueryOptimizationChecker.analyze() before returning, so if generateLiquibaseChangesFile() fails the checkpoint is lost; modify the flow so checkpoint clearing occurs only after successful generation: either move Liquibase file creation into the analyzer lifecycle (e.g., add a new method on QueryOptimizationChecker that runs analyze() then generateLiquibaseChangesFile() and clears the checkpoint only on success) or change analyze() to not clear the checkpoint and add explicit checkpointClear() that is invoked only after generateLiquibaseChangesFile() completes without error; update calls to use the new lifecycle method or call checkpointClear() after successful generation.
553-577:⚠️ Potential issue | 🟠 MajorQuiet mode drops issues that don't include a rewritten query.
reportOptimizationIssuesQuiet()only prints anything insideif (issue.optimizedQuery() != null), so recommendations that only carry a description or column-order fix disappear entirely in quiet mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java` around lines 553 - 577, reportOptimizationIssuesQuiet currently only prints when issue.optimizedQuery() != null, which hides recommendations that have no rewritten query; change reportOptimizationIssuesQuiet (after obtaining OptimizationIssue issue from reportNumbers(result) and collectIndexSuggestions(result)) to always emit the issue summary even if optimizedQuery() is null: print Repository and Method, then print any human-readable recommendation/description from issue (e.g., issue.getDescription() or similar), always show Parameter Order Change when currentColumnOrder/recommendedColumnOrder differ, and finally print the divider; keep the existing optimized-query block but treat it as additional detail rather than the gate for all output so that non-rewrite suggestions are visible in quiet mode.
🟡 Minor comments (5)
src/main/resources/ai-prompts/normalization-system-prompt.md-105-105 (1)
105-105:⚠️ Potential issue | 🟡 MinorInconsistent foreign key direction between examples.
The migration hint here suggests
customer.address_id -> address.id(customer references address), but the detaileddataMigrationPlanexample at lines 144-158 shows the opposite direction:address.customer_id -> customer.id(address references customer).For the typical "extract address from customer" scenario, the detailed example (lines 144-158) represents the more common pattern where the extracted entity holds the FK back to the parent. Consider aligning line 105 to match that pattern.
📝 Proposed fix to align FK direction
- "liquibaseMigrationHint": "CREATE TABLE address (...); ALTER TABLE customer ADD COLUMN address_id BIGINT REFERENCES address(id); ...", + "liquibaseMigrationHint": "CREATE TABLE address (..., customer_id BIGINT REFERENCES customer(id)); -- populate address from customer columns; UPDATE customer SET street=NULL, city=NULL, zip=NULL; ...",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/ai-prompts/normalization-system-prompt.md` at line 105, Align the foreign-key direction between the liquibaseMigrationHint and the dataMigrationPlan example: update the liquibaseMigrationHint string so it shows the extracted entity (address) holding the FK to the parent (customer), e.g., use "CREATE TABLE address (..., customer_id BIGINT REFERENCES customer(id)); ALTER TABLE customer ...;" to match the dataMigrationPlan example; ensure you modify the value for the key liquibaseMigrationHint and keep the wording consistent with the dataMigrationPlan example that uses address.customer_id -> customer.id.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java-74-77 (1)
74-77:⚠️ Potential issue | 🟡 MinorUse a warning level for the non-fatal coverage check.
This path is documented as warning-only validation. Emitting
errorhere makes incomplete-but-usable plans look like hard failures in monitoring.🔧 Suggested change
- logger.error("Column '{}' from source profile is not mapped in columnMappings", profileCol); + logger.warn("Column '{}' from source profile is not mapped in columnMappings", profileCol);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java` around lines 74 - 77, In DataMigrationPlanValidator (the loop iterating over profileColumns), change the non-fatal validation log from logger.error to a warning-level call (e.g., logger.warn or logger.warning) for the case where profileCol is not in mappedViewColumns so that missing source profile columns emit warnings rather than errors; keep the existing message text that references profileCol and "columnMappings" and update any related tests or assertions expecting an error-level log to expect a warning instead.src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java-237-241 (1)
237-241:⚠️ Potential issue | 🟡 MinorAssert the specific parse-failure type here.
Exception.classwill also pass on unrelated failures, so this no longer verifies the newAIResponseExceptioncontract introduced by the parser changes.🎯 Tighten the assertion
- assertThrows(Exception.class, () -> geminiAIService.parseRecommendations(invalidJson, batch)); + assertThrows(AIResponseException.class, + () -> geminiAIService.parseRecommendations(invalidJson, batch));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java` around lines 237 - 241, The test testParseRecommendations_InvalidJson currently asserts a generic Exception which can hide unrelated failures; update it to assert the specific parse-failure type by replacing Exception.class with AIResponseException.class in the assertThrows call that invokes geminiAIService.parseRecommendations(invalidJson, batch); optionally also assert the exception message or cause to ensure it indicates a JSON parse error from the parser contract.src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.java-68-77 (1)
68-77:⚠️ Potential issue | 🟡 MinorDeclare a logger on
OpenRouterServicebefore logging from it.This class logs at line 76 but uses the inherited logger from
AbstractAIService, causing the entry to be emitted under the base-class logger name instead ofOpenRouterService. Per the coding guidelines for Java files, classes that log must declare their own SLF4J logger field.🪵 Add a class-local logger
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class OpenRouterService extends OpenAIService { + private static final Logger logger = LoggerFactory.getLogger(OpenRouterService.class); private static final String DEFAULT_ENDPOINT = "https://openrouter.ai/api/v1/chat/completions";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.java` around lines 68 - 77, OpenRouterService is using the inherited logger from AbstractAIService when calling logger.info in sendApiRequest, so add a class-local SLF4J logger to OpenRouterService (e.g., declare a private static final Logger logger = LoggerFactory.getLogger(OpenRouterService.class)) and import org.slf4j.Logger and org.slf4j.LoggerFactory; ensure sendApiRequest continues to use this new logger field so log entries are emitted under OpenRouterService rather than the base class.src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java-75-77 (1)
75-77:⚠️ Potential issue | 🟡 MinorQuiet mode still prints a broken banner.
This block is unconditional, and
println("%n"... )emits the literal%n. Wrap it inif (!quietMode)and print the blank line separately.Possible fix
- System.out.println("%n" + "=".repeat(80)); - System.out.printf("Analyzing: %s%n", fullyQualifiedName); - System.out.println("=".repeat(80)); + if (!quietMode) { + System.out.println(); + System.out.println("=".repeat(80)); + System.out.printf("Analyzing: %s%n", fullyQualifiedName); + System.out.println("=".repeat(80)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java` around lines 75 - 77, The banner printing is unconditional and uses a literal "%n", so wrap the three print calls in an if (!quietMode) block; replace System.out.println("%n" + "=".repeat(80)) with System.out.println() to emit a blank line, keep System.out.println("=".repeat(80)) for the separators, and leave System.out.printf("Analyzing: %s%n", fullyQualifiedName) as the middle line; update these calls in AbstractRepositoryAnalyzer where quietMode and fullyQualifiedName are referenced.
🧹 Nitpick comments (5)
src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java (1)
236-239: LGTM! Consider updating the Javadoc to mentiongiven.The addition of
"given".equals(name)correctly extends detection to BDDMockito'sgiven()method, which is the BDD-style equivalent ofwhen().The Javadoc above (lines 231-235) only mentions
verify(field)andwhen(field). For completeness, consider addinggiven(field)to the documentation.,
📝 Optional: Update Javadoc to include `given`
/** * Checks if a method call is a Mockito stubbing or verification call - * (i.e., {`@code` verify(field)} or {`@code` when(field)}). + * (i.e., {`@code` verify(field)}, {`@code` when(field)}, or {`@code` given(field)}). * Used to detect patterns like {`@code` verify(repo).method()} and * {`@code` doReturn(val).when(repo).method()}. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java` around lines 236 - 239, Update the Javadoc above the isMockitoStubbingOrVerify(MethodCallExpr call) method to include BDDMockito's given(field) alongside verify(field) and when(field); locate the Javadoc block that currently lists "verify(field)" and "when(field)" and add "given(field)" so the documentation matches the implementation that checks for "given". Ensure the wording clearly states that the method detects verify, when and given stubbing/verification calls.src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java (1)
169-196:testMissingProfileColumnGeneratesWarningnever asserts the warning.Right now this only proves that validation does not throw. If warning emission regresses or disappears, the test still passes. Capture stderr (or assert a returned warning collection) so the behavior named by the test is actually covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java` around lines 169 - 196, The test testMissingProfileColumnGeneratesWarning only checks that DataMigrationPlanValidator.validate(plan, profile) does not throw but does not assert the expected warning; update the test to capture and assert the warning by either temporarily redirecting stderr and asserting it contains "Column 'email' from source profile is not mapped" or by modifying/using DataMigrationPlanValidator.validate to return or expose a collection of warnings and then assert that the returned warnings include that message; reference the test method name (testMissingProfileColumnGeneratesWarning) and the validator method (DataMigrationPlanValidator.validate) when making the change so the assertion verifies the warning emission rather than only non-throwing behavior.src/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java (1)
74-159: Please assert the generated child FK column shape.The fixture already introduces
address.customer_id, but these checks never inspect the address DDL for that column’s type/constraint. That leaves the current fallback path untested, so the generator can emit the wrong child schema and this suite still stays green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java` around lines 74 - 159, Add assertions that inspect the generated child (address) changeset returned by generator.generate(twoTableView, customerProfile, NormalizedTableDDLGenerator.MODE_LIQUIBASE) — e.g. result.get(1) — and verify the FK column shape: assert it contains a column named "customer_id", that its type maps to the parent ID type (e.g. type="bigint" for Long), and that it does not have autoIncrement; also assert appropriate nullability/constraint markup (nullable="false" or a <constraints .../> entry) so the address FK column shape is actually tested.src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.java (1)
165-240: Please add a duplicate-parent migration case.These assertions only cover ordering and column projection. They would still all pass if
generateMigrationSql()emits duplicate parent inserts, so the current failure mode in 1:n normalization plans is untested here. A regression test for the chosen dedupe strategy would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.java` around lines 165 - 240, Add a test that verifies duplicate parent rows are deduplicated in the generated SQL: create a ViewDescriptor similar to the three-level case but describing a denormalized source that would produce duplicate customer rows (same customer id referenced by multiple address/phone rows), call generator.generateMigrationSql(threeTableView) and assert there is exactly one "INSERT INTO customer" statement and that that customer INSERT's SELECT uses a dedupe strategy (e.g., contains "DISTINCT" or otherwise projects only the parent key/columns once). Reference the existing test names (testThreeLevelDependencyOrderIsCustomerThenAddressThenPhone, testThreeLevelDependencyProducesCorrectSql) and the method generator.generateMigrationSql to locate where to add a new test ensuring duplicate-parent inserts are not emitted.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java (1)
18-18: Add the SLF4J logger field expected for new Java classes.This new utility class is the only changed top-level Java type here without a logger declaration. Add the standard SLF4J field to keep it aligned with the repository convention. As per coding guidelines, "Logger must add or update SLF4J logger fields in Java classes using dependency injection or direct field declarations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java` at line 18, The class InsteadOfTriggerGenerator is missing the standard SLF4J logger field; add a private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InsteadOfTriggerGenerator.class) (or use an injected logger per project conventions) at the top of the class so it follows the repository's logging pattern and can be used by methods in InsteadOfTriggerGenerator for logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/schema-normalization-implementation-plan.md`:
- Around line 82-91: The plan must stop assuming FK names from conventions and
instead resolve them via the existing metadata lookup: when implementing Step 3a
(discover FKs) and Step 3b (generate changesets), call Indexes.loadAll(...) /
lookupFKConstraintName(...) to resolve the actual constraint name for each
(referencingTable, fkColumn) before emitting a drop changeset; use the returned
constraint name (or skip/emit a warning if none found) when creating the
Liquibase <dropForeignKeyConstraint> or raw ALTER TABLE ... DROP CONSTRAINT, and
only then produce the rename_old_table_to changeset after those drops.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 548-550: The blank-response check currently depends on
logger.isWarnEnabled(), causing readTree("") to be called when warnings are
disabled; update the logic in AbstractAIService to first test
jsonResponse.trim().isEmpty() and always return issues for an empty payload, and
only call logger.warn("No valid JSON found in AI response") when
logger.isWarnEnabled() inside that branch; reference the jsonResponse variable,
logger, the readTree call, and the issues return to locate and fix the
guarded-return behavior.
- Around line 650-670: The getJsonStart method currently returns the first '['
or '{' it finds when not on its own line, which can yield invalid JSON slices;
update getJsonStart to locate the earliest candidate brace/bracket (using
arrayStart/objectStart and subsequent occurrences) and validate each candidate
by attempting to parse the substring from that index as JSON (e.g., via your
JSON parser/mapper) until a valid JSON root is found, returning that index; if
no candidate parses successfully, throw the existing AIResponseException with
the full response. Ensure you modify references in getJsonStart only and keep
the existing exception/message behavior.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java`:
- Around line 31-35: The shared static filter fields in
AbstractRepositoryAnalyzer (quietMode, targetClass, targetMethod, skipClass)
cause state leakage between analyzers; change these from protected static to
protected instance fields on AbstractRepositoryAnalyzer and update all usages in
subclasses (e.g., QueryOptimizationChecker, SchemaNormalizationAnalyzer) and any
CLI parsing code to set/get them on the analyzer instance rather than via static
access so each analyzer maintains its own filter state.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java`:
- Around line 1248-1256: The generator currently always emits
`@GeneratedValue`(strategy = GenerationType.IDENTITY) for ID fields (in the block
setting idEmitted and appending using cm.targetColumn(), javaType, fieldName),
which overrides assigned or sequence IDs; change the logic to consult the
FieldProfile auto-generated flag (e.g., FieldProfile.isAutoGenerated() or the
equivalent property available on the profile used to build cm) and only emit a
`@GeneratedValue` annotation when the source ID is actually auto-generated—if the
profile also records sequence vs identity, map that to the appropriate
GenerationType (or omit `@GeneratedValue` entirely for assigned IDs); apply the
same conditional change to the other ID-emitting block around the referenced
lines (1270-1278).
- Around line 329-333: The code currently treats an empty but well-formed AI
response as failure because the condition in SchemaNormalizationAnalyzer that
checks "if (!lastResponseTruncated && !reports.isEmpty())" requires reports to
be non-empty; change it to accept a non-null reports list even if empty. Update
the condition to "if (!lastResponseTruncated && reports != null)" (or equivalent
null-check) so that valid [] responses are added via allReports.addAll(reports)
and return true; ensure you null-check reports before calling addAll to avoid
NPE and leave the existing logger.warn path for truncated/malformed responses.
- Around line 1468-1469: getLiquibasePath() currently terminates the JVM when
base_path or changelog is missing, preventing the advertised "pure normalization
analysis" that should allow passing null to SchemaNormalizationAnalyzer; change
getLiquibasePath() to return null instead of calling System.exit(…) when
metadata is absent (or throw a checked exception) and update the call site where
SchemaNormalizationAnalyzer is instantiated to capture the returned path (e.g.,
String liquibasePath = getLiquibasePath();) and pass it into new
SchemaNormalizationAnalyzer(liquibasePath) so analyzer.analyze() can run in pure
mode when liquibasePath is null.
- Around line 874-882: The loop builds JOINs for each child table but uses
fk.toTable() as the joined table, causing the parent to be joined again; in the
block around order, fksByFromTable and InsteadOfTriggerGenerator.ForeignKey,
change the JOIN target from fk.toTable() to the actual child table (use the
local variable table or fk.fromTable()) so the generated fragment is "JOIN
<child_table> ON <child_table>.<fromColumn> = <parent_table>.<toColumn>"
ensuring each child is joined once and ON uses fk.fromTable()/fk.fromColumn() =
fk.toTable()/fk.toColumn().
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.java`:
- Around line 50-60: The current INSERT built in DataMigrationGenerator using
insertCols/selectCols will duplicate parent rows when the source view repeats
them; modify the SQL generation in the method that constructs the sql string
(referencing insertCols, selectCols and the variable table/view.viewName()) to
dedupe the SELECT—e.g., replace the plain "SELECT " + selectCols + " FROM ..."
with a deduped select such as "SELECT DISTINCT " + selectCols + " FROM ..." or a
"SELECT " + selectCols + " FROM (SELECT " + selectCols + ", ROW_NUMBER() OVER
(PARTITION BY <parent key cols> ORDER BY <stable col>) rn FROM " +
view.viewName() + ") t WHERE rn = 1" approach so only one row per parent is
inserted; ensure you choose the dedupe key appropriate for the target table (use
the parent PK columns if available) and update the sql variable accordingly.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java`:
- Around line 125-126: Replace the silent empty-string returns in
InsteadOfTriggerGenerator (where you call findBasePk(view, groupByTable(view))
and similar checks) with a fail-fast exception: throw a descriptive
IllegalStateException (or custom unchecked exception) that includes the
ViewDescriptor identity (e.g., view.getName() or toString()) and the affected
base table name so malformed ViewDescriptor instances cannot produce empty
UPDATE/DELETE trigger DDL; apply this change to all similar guards (the checks
at the shown occurrences around lines 125-126, 141-142, 173-174, and 187-188) so
the caller receives a clear error instead of a no-op migration.
- Around line 319-320: deriveSetColumns currently drops the first entry by
position which makes child-table updates depend on mapping order; change it to
remove the mapping that represents the base-table/PK mapping instead of blindly
dropping index 0. Locate deriveSetColumns and replace the positional subList
logic with code that finds the ColumnMapping that identifies the base table
(e.g., via a property or method on ColumnMapping or by comparing to the
ViewDescriptor's base table/role) and returns cols without that specific
mapping; if no base mapping is present, return cols or an empty list as
appropriate to preserve semantics for single-column mappings.
- Around line 332-337: The current findImmediateParentFk(String table,
ViewDescriptor view) arbitrarily returns the first FK from view.foreignKeys()
and can pick the wrong outbound FK; change it to select the FK whose fromTable()
equals the given table AND whose target matches the parent view (e.g.,
fk.toTable().equals(view.tableName()) or fk.toView().name()/identifier() equals
view.name()/id()), so the chosen FK actually references the parent view used for
UPDATE/DELETE row targeting; if no such FK exists, throw an
IllegalArgumentException with a clear message listing the candidate FKs for
debugging.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java`:
- Around line 342-387: createDialectSqlChangesetWithRollback currently injects
raw SQL and unescaped attributes into XML; update it to XML-escape attribute
values (changeset id, author, dbms values via config.author(),
generateChangesetId(...) and entry.getKey().getLiquibaseDbms()) and to emit
SQL/rollback bodies inside a CDATA wrapper helper (e.g., toCData(String sql))
instead of appending entry.getValue() directly; apply the same escaping + CDATA
approach for the rollback branch and mirror this fix in the sibling methods
noted in the review (the other SQL/view changelog generators) so all SQL bodies
and attributes are safe for XML.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java`:
- Around line 90-115: The code only seeds column metadata from
sourceProfile.fields(), so synthetic FK-derived child columns (introduced by
normalization) are missing types and nullability and fall back to varchar(255);
fix by walking view.foreignKeys() after building
columnJavaType/columnNullable/columnIsId/columnIsAutoGenerated and for each
ForeignKey where the child column is absent, look up the referenced parent
column's type/nullability (from columnJavaType/columnNullable or sourceProfile)
and populate the child entry accordingly so the subsequent
TopologicalSorter.sort/order loop and buildRawSql/buildLiquibaseChangeset
receive correct metadata; ensure FK-derived columns are marked
nullable/non-id/auto-generated correctly and that FK constraints are emitted by
the existing DDL builders.
- Around line 125-156: buildRawSql currently ignores autoGenMap so
identity/auto-generated PKs become plain types; modify buildRawSql to detect
when a column is both an ID (isIdMap) and auto-generated
(autoGenMap.get(viewColumn) == true) and then either append the dialect-specific
identity/serial syntax to the column definition (via an existing or new helper
like getIdentityFragmentForDialect(sqlType) /
dialect.identityColumnSql(sqlType)) or, if the current dialect cannot emit
identity in raw SQL, fail fast by throwing an IllegalArgumentException with a
clear message; ensure pkCols handling remains correct and update any
callers/tests to expect the new behavior.
---
Outside diff comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.java`:
- Around line 23-31: The create method in AIServiceFactory needs to normalize
the provider value before the switch: obtain the raw value from config
(currently assigned to provider), convert nulls or blank/whitespace-only strings
to the default "gemini", then trim and lowercase the resulting string before
entering the switch; update the provider variable used in the switch (in the
create method) so values like null, "", " openrouter ", or "OpenAI " are
normalized and correctly matched to GeminiAIService, OpenAIService, or
OpenRouterService, otherwise throw the existing IllegalArgumentException for
unknown providers.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 1205-1210: The analyzer currently clears its checkpoint inside
QueryOptimizationChecker.analyze() before returning, so if
generateLiquibaseChangesFile() fails the checkpoint is lost; modify the flow so
checkpoint clearing occurs only after successful generation: either move
Liquibase file creation into the analyzer lifecycle (e.g., add a new method on
QueryOptimizationChecker that runs analyze() then generateLiquibaseChangesFile()
and clears the checkpoint only on success) or change analyze() to not clear the
checkpoint and add explicit checkpointClear() that is invoked only after
generateLiquibaseChangesFile() completes without error; update calls to use the
new lifecycle method or call checkpointClear() after successful generation.
- Around line 553-577: reportOptimizationIssuesQuiet currently only prints when
issue.optimizedQuery() != null, which hides recommendations that have no
rewritten query; change reportOptimizationIssuesQuiet (after obtaining
OptimizationIssue issue from reportNumbers(result) and
collectIndexSuggestions(result)) to always emit the issue summary even if
optimizedQuery() is null: print Repository and Method, then print any
human-readable recommendation/description from issue (e.g.,
issue.getDescription() or similar), always show Parameter Order Change when
currentColumnOrder/recommendedColumnOrder differ, and finally print the divider;
keep the existing optimized-query block but treat it as additional detail rather
than the gate for all output so that non-rewrite suggestions are visible in
quiet mode.
In `@src/main/java/sa/com/cloudsolutions/liquibase/Indexes.java`:
- Around line 201-224: processChange currently records
AddForeignKeyConstraintChange into fkResult/fkConstraintMap but never removes
entries, causing stale entries; add handling for DropForeignKeyConstraintChange
in processChange (mirror the existing else-if chain) and implement a new handler
(e.g., handleDropForeignKey) that accepts the DropForeignKeyConstraintChange and
the fkResult/fkConstraintMap and removes the corresponding FK entry by
constraintName (and by table/columns fallback if constraintName is null),
ensuring lookupFKConstraintName reads the updated map; also apply the same
removal logic in any other places that can drop or replace FKs (e.g.,
CreateTable/DropTable handlers) so dropped/renamed FKs are removed from
fkResult/fkConstraintMap.
---
Minor comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java`:
- Around line 75-77: The banner printing is unconditional and uses a literal
"%n", so wrap the three print calls in an if (!quietMode) block; replace
System.out.println("%n" + "=".repeat(80)) with System.out.println() to emit a
blank line, keep System.out.println("=".repeat(80)) for the separators, and
leave System.out.printf("Analyzing: %s%n", fullyQualifiedName) as the middle
line; update these calls in AbstractRepositoryAnalyzer where quietMode and
fullyQualifiedName are referenced.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.java`:
- Around line 68-77: OpenRouterService is using the inherited logger from
AbstractAIService when calling logger.info in sendApiRequest, so add a
class-local SLF4J logger to OpenRouterService (e.g., declare a private static
final Logger logger = LoggerFactory.getLogger(OpenRouterService.class)) and
import org.slf4j.Logger and org.slf4j.LoggerFactory; ensure sendApiRequest
continues to use this new logger field so log entries are emitted under
OpenRouterService rather than the base class.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java`:
- Around line 74-77: In DataMigrationPlanValidator (the loop iterating over
profileColumns), change the non-fatal validation log from logger.error to a
warning-level call (e.g., logger.warn or logger.warning) for the case where
profileCol is not in mappedViewColumns so that missing source profile columns
emit warnings rather than errors; keep the existing message text that references
profileCol and "columnMappings" and update any related tests or assertions
expecting an error-level log to expect a warning instead.
In `@src/main/resources/ai-prompts/normalization-system-prompt.md`:
- Line 105: Align the foreign-key direction between the liquibaseMigrationHint
and the dataMigrationPlan example: update the liquibaseMigrationHint string so
it shows the extracted entity (address) holding the FK to the parent (customer),
e.g., use "CREATE TABLE address (..., customer_id BIGINT REFERENCES
customer(id)); ALTER TABLE customer ...;" to match the dataMigrationPlan
example; ensure you modify the value for the key liquibaseMigrationHint and keep
the wording consistent with the dataMigrationPlan example that uses
address.customer_id -> customer.id.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java`:
- Around line 237-241: The test testParseRecommendations_InvalidJson currently
asserts a generic Exception which can hide unrelated failures; update it to
assert the specific parse-failure type by replacing Exception.class with
AIResponseException.class in the assertThrows call that invokes
geminiAIService.parseRecommendations(invalidJson, batch); optionally also assert
the exception message or cause to ensure it indicates a JSON parse error from
the parser contract.
---
Nitpick comments:
In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java`:
- Around line 236-239: Update the Javadoc above the
isMockitoStubbingOrVerify(MethodCallExpr call) method to include BDDMockito's
given(field) alongside verify(field) and when(field); locate the Javadoc block
that currently lists "verify(field)" and "when(field)" and add "given(field)" so
the documentation matches the implementation that checks for "given". Ensure the
wording clearly states that the method detects verify, when and given
stubbing/verification calls.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java`:
- Line 18: The class InsteadOfTriggerGenerator is missing the standard SLF4J
logger field; add a private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(InsteadOfTriggerGenerator.class) (or use an
injected logger per project conventions) at the top of the class so it follows
the repository's logging pattern and can be used by methods in
InsteadOfTriggerGenerator for logging.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.java`:
- Around line 165-240: Add a test that verifies duplicate parent rows are
deduplicated in the generated SQL: create a ViewDescriptor similar to the
three-level case but describing a denormalized source that would produce
duplicate customer rows (same customer id referenced by multiple address/phone
rows), call generator.generateMigrationSql(threeTableView) and assert there is
exactly one "INSERT INTO customer" statement and that that customer INSERT's
SELECT uses a dedupe strategy (e.g., contains "DISTINCT" or otherwise projects
only the parent key/columns once). Reference the existing test names
(testThreeLevelDependencyOrderIsCustomerThenAddressThenPhone,
testThreeLevelDependencyProducesCorrectSql) and the method
generator.generateMigrationSql to locate where to add a new test ensuring
duplicate-parent inserts are not emitted.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java`:
- Around line 169-196: The test testMissingProfileColumnGeneratesWarning only
checks that DataMigrationPlanValidator.validate(plan, profile) does not throw
but does not assert the expected warning; update the test to capture and assert
the warning by either temporarily redirecting stderr and asserting it contains
"Column 'email' from source profile is not mapped" or by modifying/using
DataMigrationPlanValidator.validate to return or expose a collection of warnings
and then assert that the returned warnings include that message; reference the
test method name (testMissingProfileColumnGeneratesWarning) and the validator
method (DataMigrationPlanValidator.validate) when making the change so the
assertion verifies the warning emission rather than only non-throwing behavior.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java`:
- Around line 74-159: Add assertions that inspect the generated child (address)
changeset returned by generator.generate(twoTableView, customerProfile,
NormalizedTableDDLGenerator.MODE_LIQUIBASE) — e.g. result.get(1) — and verify
the FK column shape: assert it contains a column named "customer_id", that its
type maps to the parent ID type (e.g. type="bigint" for Long), and that it does
not have autoIncrement; also assert appropriate nullability/constraint markup
(nullable="false" or a <constraints .../> entry) so the address FK column shape
is actually tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4319ba7b-eed2-451b-a36b-eb06e02ee5d6
📒 Files selected for processing (31)
.gitignoredocs/schema-normalization-implementation-plan.mddocs/schema-normalization-improvement-plan.mddocs/schema-normalization-task-list.mdsrc/main/java/sa/com/cloudsolutions/antikythera/examples/AIResponseException.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorter.javasrc/main/java/sa/com/cloudsolutions/liquibase/Indexes.javasrc/main/resources/ai-prompts/normalization-system-prompt.mdsrc/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGeneratorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorterTest.javasrc/test/java/sa/com/cloudsolutions/liquibase/IndexesTest.java
Comment on lines
+82
to
+91
| **Step 3a — Discover FKs that reference the old table** | ||
|
|
||
| - **Input:** `String sourceTable`, `List<EntityProfile> allProfiles` (and optionally entity→table resolution). The entity that maps to `sourceTable` is the “current” entity; we need every *other* entity that has a relationship pointing to it. | ||
| - **Logic:** For each `EntityProfile p` in `allProfiles`, for each `RelationshipProfile r` in `p.relationships()`, resolve `r.targetEntity()` to a table name (e.g. via `EntityMappingResolver.getTableNameForEntity(r.targetEntity())` or by matching entity name to profile and taking `tableName`). If that table equals `sourceTable`, then `p.tableName()` is the **referencing table** and `r.joinColumn()` is the FK column. Constraint name: use convention e.g. `fk_<referencingTable>_<sourceTable>` or derive from JPA if available. | ||
| - **Output:** List of `(referencingTable, constraintName)` or `(referencingTable, fkColumn)`. | ||
|
|
||
| **Step 3b — Generate changesets** | ||
|
|
||
| - **Drop FKs:** For each (referencingTable, constraintName), generate one changeset. Use Liquibase `<dropForeignKeyConstraint baseTableName="referencingTable" constraintName="constraintName"/>` or raw SQL `ALTER TABLE referencingTable DROP CONSTRAINT constraintName`. Use `LiquibaseGenerator.createRawSqlChangeset` with dialect-specific SQL if needed (e.g. `dbms="postgresql"` and `dbms="oracle"`). | ||
| - **Rename old table:** Read `rename_old_table_to` from config; if it contains `{sourceTable}`, substitute the plan’s source table name. Generate one changeset: Liquibase `renameTable` or raw SQL `ALTER TABLE sourceTable RENAME TO backupName`. Add to the same per-plan changeset list **after** the drop-FK changesets. |
Contributor
There was a problem hiding this comment.
Resolve FK names from metadata, not naming conventions.
This phase still suggests deriving constraintName from JPA metadata or fk_<table>_<source> naming. That will generate broken dropForeignKeyConstraint steps for any changelog that used custom names. Since this PR already introduces Indexes.loadAll(...) / lookupFKConstraintName(...), the plan should direct implementers to use that lookup as the source of truth before emitting the drop changesets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/schema-normalization-implementation-plan.md` around lines 82 - 91, The
plan must stop assuming FK names from conventions and instead resolve them via
the existing metadata lookup: when implementing Step 3a (discover FKs) and Step
3b (generate changesets), call Indexes.loadAll(...) /
lookupFKConstraintName(...) to resolve the actual constraint name for each
(referencingTable, fkColumn) before emitting a drop changeset; use the returned
constraint name (or skip/emit a warning if none found) when creating the
Liquibase <dropForeignKeyConstraint> or raw ALTER TABLE ... DROP CONSTRAINT, and
only then produce the rename_old_table_to changeset after those drops.
Comment on lines
+548
to
550
| if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) { | ||
| logger.warn("No valid JSON found in AI response"); | ||
| return issues; |
Contributor
There was a problem hiding this comment.
Keep blank-response handling independent from the warn guard.
With warn disabled, this falls through into readTree("") and throws instead of returning the empty list for a blank payload.
💡 Suggested fix
- if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) {
- logger.warn("No valid JSON found in AI response");
+ if (jsonResponse.trim().isEmpty()) {
+ if (logger.isWarnEnabled()) {
+ logger.warn("No valid JSON found in AI response");
+ }
return issues;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`
around lines 548 - 550, The blank-response check currently depends on
logger.isWarnEnabled(), causing readTree("") to be called when warnings are
disabled; update the logic in AbstractAIService to first test
jsonResponse.trim().isEmpty() and always return issues for an empty payload, and
only call logger.warn("No valid JSON found in AI response") when
logger.isWarnEnabled() inside that branch; reference the jsonResponse variable,
logger, the readTree call, and the issues return to locate and fix the
guarded-return behavior.
Comment on lines
+650
to
+670
| static int getJsonStart(String s) { | ||
| int lastOnLine = Math.max(s.lastIndexOf("\n["), s.lastIndexOf("\n{")); | ||
| if (lastOnLine != -1) { | ||
| return lastOnLine + 1; | ||
| } | ||
|
|
||
| int arrayStart = s.indexOf('['); | ||
| int objectStart = s.indexOf('{'); | ||
|
|
||
| if (arrayStart == -1 && objectStart == -1) { | ||
| throw new AIResponseException("AI response contains no JSON array or object. Full response:\n" + s); | ||
| } | ||
|
|
||
| if (arrayStart == -1) { | ||
| return objectStart; | ||
| } | ||
| if (objectStart == -1) { | ||
| return arrayStart; | ||
| } | ||
|
|
||
| return Math.min(arrayStart, objectStart); |
Contributor
There was a problem hiding this comment.
Validate the fallback JSON root instead of picking the first bracket.
When neither [ nor { starts on its own line, this just returns the first delimiter it sees. Inputs with inline prose or trailing text still yield substrings that are not valid JSON, so the new extractor still fails on the mixed-content cases it is meant to normalize. The new mixed-content tests will need to be tightened along with this fix.
💡 One robust fallback direction
static int getJsonStart(String s) {
int lastOnLine = Math.max(s.lastIndexOf("\n["), s.lastIndexOf("\n{"));
if (lastOnLine != -1) {
return lastOnLine + 1;
}
-
- int arrayStart = s.indexOf('[');
- int objectStart = s.indexOf('{');
-
- if (arrayStart == -1 && objectStart == -1) {
- throw new AIResponseException("AI response contains no JSON array or object. Full response:\n" + s);
- }
-
- if (arrayStart == -1) {
- return objectStart;
- }
- if (objectStart == -1) {
- return arrayStart;
- }
-
- return Math.min(arrayStart, objectStart);
+ for (int i = s.length() - 1; i >= 0; i--) {
+ char ch = s.charAt(i);
+ if (ch != '[' && ch != '{') {
+ continue;
+ }
+
+ String candidate = s.substring(i).trim();
+ try {
+ new ObjectMapper().readTree(candidate);
+ return i;
+ } catch (IOException ignored) {
+ // keep scanning for the last parsable JSON root
+ }
+ }
+
+ throw new AIResponseException("AI response contains no JSON array or object. Full response:\n" + s);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`
around lines 650 - 670, The getJsonStart method currently returns the first '['
or '{' it finds when not on its own line, which can yield invalid JSON slices;
update getJsonStart to locate the earliest candidate brace/bracket (using
arrayStart/objectStart and subsequent occurrences) and validate each candidate
by attempting to parse the substring from that index as JSON (e.g., via your
JSON parser/mapper) until a valid JSON root is found, returning that index; if
no candidate parses successfully, throw the existing AIResponseException with
the full response. Ensure you modify references in getJsonStart only and keep
the existing exception/message behavior.
Comment on lines
+31
to
+35
| // Shared CLI-level filters | ||
| protected static boolean quietMode = false; | ||
| protected static String targetClass = null; | ||
| protected static String targetMethod = null; | ||
| protected static String skipClass = null; |
Contributor
There was a problem hiding this comment.
The shared static filters bleed across analyzers.
quietMode, targetClass, targetMethod, and skipClass live on the abstract base, so QueryOptimizationChecker and SchemaNormalizationAnalyzer share one mutable filter state. In the same JVM/test run, one analyzer can silently influence the next unless every caller clears them first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java`
around lines 31 - 35, The shared static filter fields in
AbstractRepositoryAnalyzer (quietMode, targetClass, targetMethod, skipClass)
cause state leakage between analyzers; change these from protected static to
protected instance fields on AbstractRepositoryAnalyzer and update all usages in
subclasses (e.g., QueryOptimizationChecker, SchemaNormalizationAnalyzer) and any
CLI parsing code to set/get them on the analyzer instance rather than via static
access so each analyzer maintains its own filter state.
Comment on lines
+329
to
+333
| if (!lastResponseTruncated && !reports.isEmpty()) { | ||
| allReports.addAll(reports); | ||
| return true; | ||
| } | ||
| logger.warn("Response empty or malformed on attempt {}; retrying full request", attempt + 1); |
Contributor
There was a problem hiding this comment.
Treat a well-formed empty AI response as success.
Success currently requires !reports.isEmpty(). A valid [] response, or one that legitimately omits clean entities, therefore burns every retry and leaves allReports empty, which also skews the summary/reporting paths that read from allReports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java`
around lines 329 - 333, The code currently treats an empty but well-formed AI
response as failure because the condition in SchemaNormalizationAnalyzer that
checks "if (!lastResponseTruncated && !reports.isEmpty())" requires reports to
be non-empty; change it to accept a non-null reports list even if empty. Update
the condition to "if (!lastResponseTruncated && reports != null)" (or equivalent
null-check) so that valid [] responses are added via allReports.addAll(reports)
and return true; ensure you null-check reports before calling addAll to avoid
NPE and leave the existing logger.warn path for truncated/malformed responses.
Comment on lines
+332
to
+337
| private ForeignKey findImmediateParentFk(String table, ViewDescriptor view) { | ||
| return view.foreignKeys().stream() | ||
| .filter(fk -> fk.fromTable().equals(table)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException( | ||
| "No FK from " + table + " found in foreignKeys")); |
Contributor
There was a problem hiding this comment.
Don't pick an arbitrary FK for UPDATE/DELETE row targeting.
findImmediateParentFk() returns the first FK declared for the table. When a normalized table has multiple outgoing FKs, the generated WHERE clause depends on list order and can update/delete the wrong rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java`
around lines 332 - 337, The current findImmediateParentFk(String table,
ViewDescriptor view) arbitrarily returns the first FK from view.foreignKeys()
and can pick the wrong outbound FK; change it to select the FK whose fromTable()
equals the given table AND whose target matches the parent view (e.g.,
fk.toTable().equals(view.tableName()) or fk.toView().name()/identifier() equals
view.name()/id()), so the chosen FK actually references the parent view used for
UPDATE/DELETE row targeting; if no such FK exists, throw an
IllegalArgumentException with a clear message listing the candidate FKs for
debugging.
Comment on lines
+316
to
+329
| public String createDropForeignKeyChangeset(String id, String baseTableName, String constraintName) { | ||
| String changesetId = generateChangesetId(sanitize(id)); | ||
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
| sb.append("<changeSet id=\"").append(changesetId).append("\" author=\"").append(config.author()).append("\">\n"); | ||
| sb.append(" <dropForeignKeyConstraint baseTableName=\"").append(baseTableName) | ||
| .append("\" constraintName=\"").append(constraintName).append("\"/>\n"); | ||
|
|
||
| if (config.includeRollback()) { | ||
| sb.append(" <rollback><!-- manual rollback required: re-add FK constraint --></rollback>\n"); | ||
| } | ||
|
|
||
| sb.append("</changeSet>"); | ||
| return sb.toString(); |
Contributor
There was a problem hiding this comment.
createDropForeignKeyChangeset cannot support real rollback with this signature.
includeRollback() defaults to true, but this method only receives baseTableName and constraintName, so the rollback block can never recreate the FK. If a later rename/view step fails, liquibase rollback leaves the external constraint permanently missing. Please carry the full FK definition here and emit an executable <addForeignKeyConstraint> rollback.
Comment on lines
+342
to
+387
| public String createDialectSqlChangesetWithRollback(String id, | ||
| Map<DatabaseDialect, String> sqlByDialect, | ||
| Map<DatabaseDialect, String> rollbackByDialect) { | ||
| String changesetId = generateChangesetId(sanitize(id)); | ||
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
| Map<DatabaseDialect, String> filteredSql = sqlByDialect.entrySet().stream() | ||
| .filter(e -> config.supportedDialects().contains(e.getKey())) | ||
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
|
|
||
| if (filteredSql.isEmpty()) { | ||
| throw new IllegalArgumentException("No SQL found for supported dialects in changeset: " + id); | ||
| } | ||
|
|
||
| sb.append("<changeSet id=\"").append(changesetId).append("\" author=\"").append(config.author()).append("\">\n"); | ||
|
|
||
| for (Map.Entry<DatabaseDialect, String> entry : filteredSql.entrySet()) { | ||
| sb.append(" <sql dbms=\"").append(entry.getKey().getLiquibaseDbms()).append("\">\n"); | ||
| sb.append(" ").append(entry.getValue()).append("\n"); | ||
| sb.append(" </sql>\n"); | ||
| } | ||
|
|
||
| if (config.includeRollback()) { | ||
| if (rollbackByDialect != null && !rollbackByDialect.isEmpty()) { | ||
| Map<DatabaseDialect, String> filteredRollback = rollbackByDialect.entrySet().stream() | ||
| .filter(e -> config.supportedDialects().contains(e.getKey())) | ||
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
|
|
||
| if (!filteredRollback.isEmpty()) { | ||
| sb.append(" <rollback>\n"); | ||
| for (Map.Entry<DatabaseDialect, String> entry : filteredRollback.entrySet()) { | ||
| sb.append(" <sql dbms=\"").append(entry.getKey().getLiquibaseDbms()).append("\">\n"); | ||
| sb.append(" ").append(entry.getValue()).append("\n"); | ||
| sb.append(" </sql>\n"); | ||
| } | ||
| sb.append(" </rollback>\n"); | ||
| } else { | ||
| sb.append(" <rollback><!-- manual rollback required --></rollback>\n"); | ||
| } | ||
| } else { | ||
| sb.append(" <rollback><!-- manual rollback required --></rollback>\n"); | ||
| } | ||
| } | ||
|
|
||
| sb.append("</changeSet>"); | ||
| return sb.toString(); |
Contributor
There was a problem hiding this comment.
Escape XML and wrap SQL bodies before emitting changelog XML.
These helpers append raw SQL and view text directly inside XML elements. Any <, &, " or ]]> in generated SQL/trigger/view bodies will produce an invalid changelog before Liquibase ever reaches the database. Please XML-escape attribute values and emit SQL through a shared CDATA helper.
🛠️ Representative fix
+ private static String xmlAttr(String value) {
+ return value
+ .replace("&", "&")
+ .replace("\"", """)
+ .replace("<", "<")
+ .replace(">", ">");
+ }
+
+ private static String cdata(String sql) {
+ return "<![CDATA[" + sql.replace("]]>", "]]]]><![CDATA[>") + "]]>";
+ }
+
public String createRawSqlChangeset(String id, String sql) {
String changesetId = generateChangesetId(sanitize(id));
StringBuilder sb = new StringBuilder();
sb.append("<changeSet id=\"").append(changesetId).append("\" author=\"").append(config.author()).append("\">\n");
- sb.append(" <sql>").append(sql).append("</sql>\n");
+ sb.append(" <sql>").append(cdata(sql)).append("</sql>\n");
@@
public String createViewChangeset(String viewName, String selectSql) {
String changesetId = generateChangesetId("view_" + sanitize(viewName));
StringBuilder sb = new StringBuilder();
sb.append("<changeSet id=\"").append(changesetId).append("\" author=\"").append(config.author()).append("\">\n");
- sb.append(" <createView viewName=\"").append(viewName).append("\" replaceIfExists=\"true\">\n");
- sb.append(" ").append(selectSql).append("\n");
+ sb.append(" <createView viewName=\"").append(xmlAttr(viewName)).append("\" replaceIfExists=\"true\">\n");
+ sb.append(" ").append(cdata(selectSql)).append("\n");
sb.append(" </createView>\n");Also applies to: 398-436, 448-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java`
around lines 342 - 387, createDialectSqlChangesetWithRollback currently injects
raw SQL and unescaped attributes into XML; update it to XML-escape attribute
values (changeset id, author, dbms values via config.author(),
generateChangesetId(...) and entry.getKey().getLiquibaseDbms()) and to emit
SQL/rollback bodies inside a CDATA wrapper helper (e.g., toCData(String sql))
instead of appending entry.getValue() directly; apply the same escaping + CDATA
approach for the rollback branch and mirror this fix in the sibling methods
noted in the review (the other SQL/view changelog generators) so all SQL bodies
and attributes are safe for XML.
Comment on lines
+90
to
+115
| // Build lookup maps from the source profile | ||
| Map<String, String> columnJavaType = new HashMap<>(); | ||
| Map<String, Boolean> columnNullable = new HashMap<>(); | ||
| Map<String, Boolean> columnIsId = new HashMap<>(); | ||
| Map<String, Boolean> columnIsAutoGenerated = new HashMap<>(); | ||
| if (sourceProfile != null) { | ||
| for (var field : sourceProfile.fields()) { | ||
| columnJavaType.put(field.columnName(), field.columnType()); | ||
| columnNullable.put(field.columnName(), field.isNullable()); | ||
| columnIsId.put(field.columnName(), field.isId()); | ||
| columnIsAutoGenerated.put(field.columnName(), field.isAutoGenerated()); | ||
| } | ||
| } | ||
|
|
||
| List<String> order = TopologicalSorter.sort(view.tables(), view.foreignKeys()); | ||
| List<String> result = new ArrayList<>(); | ||
|
|
||
| for (String table : order) { | ||
| List<ColumnMapping> cols = byTable.getOrDefault(table, List.of()); | ||
| if (cols.isEmpty()) continue; | ||
|
|
||
| if (MODE_RAW_SQL.equals(ddlMode)) { | ||
| result.add(buildRawSql(table, cols, columnJavaType, columnNullable, columnIsId, columnIsAutoGenerated)); | ||
| } else { | ||
| result.add(buildLiquibaseChangeset(table, cols, columnJavaType, columnNullable, columnIsId, columnIsAutoGenerated)); | ||
| } |
Contributor
There was a problem hiding this comment.
FK-derived columns are generated with fallback types right now.
Column metadata is only loaded from sourceProfile.fields(), and view.foreignKeys() is never used to backfill synthetic child columns. In the common split case where something like address.customer_id is introduced during normalization, this code falls back to varchar(255)/nullable and never emits an FK at all, so the generated DDL is wrong for multi-table plans.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java`
around lines 90 - 115, The code only seeds column metadata from
sourceProfile.fields(), so synthetic FK-derived child columns (introduced by
normalization) are missing types and nullability and fall back to varchar(255);
fix by walking view.foreignKeys() after building
columnJavaType/columnNullable/columnIsId/columnIsAutoGenerated and for each
ForeignKey where the child column is absent, look up the referenced parent
column's type/nullability (from columnJavaType/columnNullable or sourceProfile)
and populate the child entry accordingly so the subsequent
TopologicalSorter.sort/order loop and buildRawSql/buildLiquibaseChangeset
receive correct metadata; ensure FK-derived columns are marked
nullable/non-id/auto-generated correctly and that FK constraints are emitted by
the existing DDL builders.
Comment on lines
+125
to
+156
| private String buildRawSql(String table, List<ColumnMapping> cols, | ||
| Map<String, String> typeMap, | ||
| Map<String, Boolean> nullableMap, | ||
| Map<String, Boolean> isIdMap, | ||
| Map<String, Boolean> autoGenMap) { | ||
| List<String> colDefs = new ArrayList<>(); | ||
| List<String> pkCols = new ArrayList<>(); | ||
|
|
||
| for (ColumnMapping cm : cols) { | ||
| String sqlType = toSqlType(typeMap.get(cm.viewColumn())); | ||
| boolean isId = Boolean.TRUE.equals(isIdMap.get(cm.viewColumn())); | ||
| boolean nullable = !Boolean.FALSE.equals(nullableMap.getOrDefault(cm.viewColumn(), true)); | ||
|
|
||
| StringBuilder def = new StringBuilder(" ").append(cm.sourceColumn()).append(" ").append(sqlType); | ||
| if (isId || !nullable) { | ||
| def.append(" NOT NULL"); | ||
| } | ||
| colDefs.add(def.toString()); | ||
|
|
||
| if (isId) { | ||
| pkCols.add(cm.sourceColumn()); | ||
| } | ||
| } | ||
|
|
||
| StringBuilder sb = new StringBuilder("CREATE TABLE ").append(table).append(" (\n"); | ||
| sb.append(String.join(",\n", colDefs)); | ||
| if (!pkCols.isEmpty()) { | ||
| sb.append(",\n PRIMARY KEY (").append(String.join(", ", pkCols)).append(")"); | ||
| } | ||
| sb.append("\n)"); | ||
| return sb.toString(); | ||
| } |
Contributor
There was a problem hiding this comment.
raw_sql mode silently drops identity generation.
autoGenMap is threaded into buildRawSql(...) but never read, so a numeric auto-generated ID becomes a plain BIGINT/INTEGER PRIMARY KEY in raw SQL mode. That changes insert semantics compared with Liquibase mode and will break plans that rely on database-generated keys. Either emit dialect-specific identity syntax here or fail fast when raw_sql is requested for auto-generated IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java`
around lines 125 - 156, buildRawSql currently ignores autoGenMap so
identity/auto-generated PKs become plain types; modify buildRawSql to detect
when a column is both an ID (isIdMap) and auto-generated
(autoGenMap.get(viewColumn) == true) and then either append the dialect-specific
identity/serial syntax to the column definition (via an existing or new helper
like getIdentityFragmentForDialect(sqlType) /
dialect.identityColumnSql(sqlType)) or, if the current dialect cannot emit
identity in raw SQL, fail fast by throwing an IllegalArgumentException with a
clear message; ensure pkCols handling remains correct and update any
callers/tests to expect the new behavior.
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
codeant-ai
Bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java (1)
917-919: Minor: Missing blank line between test methods.There's a formatting inconsistency—other test methods in this file are separated by blank lines, but
testWriteChangesetToFileWithCustomPrefixandtestCreateDialectSqlChangesetWithFilteringare not.🔧 Suggested fix
assertTrue(updatedMaster.contains("antikythera-normalize-")); } + `@Test` void testCreateDialectSqlChangesetWithFiltering() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java` around lines 917 - 919, Add a blank line separating the two test methods to match project formatting: insert an empty line between the closing brace of testWriteChangesetToFileWithCustomPrefix and the `@Test` declaration for testCreateDialectSqlChangesetWithFiltering so the methods testWriteChangesetToFileWithCustomPrefix and testCreateDialectSqlChangesetWithFiltering are visually separated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java`:
- Around line 917-919: Add a blank line separating the two test methods to match
project formatting: insert an empty line between the closing brace of
testWriteChangesetToFileWithCustomPrefix and the `@Test` declaration for
testCreateDialectSqlChangesetWithFiltering so the methods
testWriteChangesetToFileWithCustomPrefix and
testCreateDialectSqlChangesetWithFiltering are visually separated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57861290-10b8-474d-bb1a-ad51a0392cb6
📒 Files selected for processing (1)
src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor
CodeAnt-AI Description
Add an AI-driven schema normalization workflow, robust LLM handling, and multi-dialect migration tooling
What Changed
Impact
✅ Fewer malformed LLM responses✅ Safer schema migrations (correct create → migrate → drop-FK → rename → view → trigger order)✅ Generated Liquibase changesets and mapping files for normalization plans💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.