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 schema-normalization feature plus supporting analyzer framework, AI service provider (OpenRouter), validators/generators (DDL, migration, triggers, topological sorter), robust AI response parsing, many tests, and comprehensive documentation/implementation plans. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Main
participant SNA as SchemaNormalization<br/>Analyzer
participant Prof as Entity<br/>Profiler
participant LLM as AI Service
participant Val as DataMigration<br/>PlanValidator
participant Gen as Generators<br/>(DDL/Migration/Trigger)
participant Out as Output<br/>Artifacts
User->>SNA: start analysis (config)
SNA->>Prof: profile entities
Prof-->>SNA: EntityProfile list
SNA->>LLM: send profiles + system prompt
LLM-->>SNA: Normalization issues + DataMigrationPlan
SNA->>Val: validate(plan, profile)
Val-->>SNA: validation result
SNA->>Gen: generate DDL, migration SQL, triggers, mapping JSON
Gen-->>Out: changesets, SQL, mapping files, entities
Out-->>User: write artifacts / report
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 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
Introduces a “Schema Normalizer” workflow that takes LLM-produced normalization plans and generates migration artifacts (DDL, data-migration SQL, compatibility views, and INSTEAD OF triggers), along with supporting utilities, configuration, and tests.
Changes:
- Added core normalization utilities (topological sort, DDL generator, data migration SQL generator, trigger generator, plan validator) and integrated them into
SchemaNormalizationAnalyzer. - Refactored analyzer infrastructure via a new
AbstractRepositoryAnalyzer, and extended AI provider support withOpenRouterServiceplus stricter JSON parsing/error signaling. - Added extensive unit tests and documentation/prompts for the normalization pipeline.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorterTest.java | New unit tests for FK topological sorting scenarios (chains, multiparent, cycles). |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java | New tests for normalized table DDL generation (Liquibase + raw SQL modes, type mapping). |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.java | Updated + expanded Liquibase generator tests for new config fields and new changeset helpers. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGeneratorTest.java | New tests validating generated trigger DDL for insert/update/delete and rollback helpers. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java | New tests for migration plan validation (consistency, cycles, coverage warnings). |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGeneratorTest.java | New tests for migration SQL generation including dependency ordering. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java | Adjusted expectation: invalid JSON now throws instead of returning empty list. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.java | New tests for robust JSON extraction from model responses (code fences, preambles, etc.). |
| src/main/resources/ai-prompts/normalization-system-prompt.md | Added system prompt defining schema normalization input/output contract incl. migration plan schema. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorter.java | New Kahn-based sorter used across generators/validators to enforce FK order and detect cycles. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java | New generator for per-table CREATE TABLE DDL (raw SQL or Liquibase XML) in dependency order. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | Extended generator: file prefix support, view/raw SQL/dialect SQL changesets, rollback support, API tweaks. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java | New trigger DDL generator (PostgreSQL + Oracle) including rollback DDL helpers. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java | New validator to sanity-check LLM migration plans before generating artifacts. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.java | New generator for INSERT…SELECT migration SQL in FK dependency order. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java | Major integration: batching to LLM, parsing/validation, and ordered artifact generation (DDL → migrate → rename → view → triggers → entities). |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java | Updated to use new analyzer base and preprocess test sources. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java | Refactored to extend AbstractRepositoryAnalyzer and adopt updated WriteResult API. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenRouterService.java | New AI provider implementation using OpenAI-compatible endpoint with OpenRouter-specific headers/flags. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java | Expanded Mockito detection to include BDDMockito given. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java | New shared base class providing filtering/checkpointing/iteration and CLI-level flags. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java | Stricter JSON handling, new extractJson, and changed behavior to throw on malformed responses. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AIServiceFactory.java | Added openrouter provider option. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/AIResponseException.java | New exception type for malformed/unexpected AI responses. |
| docs/schema-normalization-task-list.md | New checklist for implementation phases/tasks. |
| docs/schema-normalization-improvement-plan.md | New detailed plan describing intended workflow and future improvements. |
| docs/schema-normalization-implementation-plan.md | New phase-based execution plan for implementing the workflow. |
| .gitignore | Added ignore entry for 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 empty-response guard is now conditional on logger.isWarnEnabled(). If warn logging is disabled and jsonResponse is blank, this method will fall through to objectMapper.readTree("") and throw, which is a behavior change from the prior safe return of an empty issue list. The early-return should depend only on jsonResponse being blank; the logging should be conditional separately.
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" + ...) prints the literal characters %n rather than a newline. Use System.out.println("\n" + ...) or switch to System.out.printf("%n%s%n", "=".repeat(80)) to get the intended formatting.
Suggested change
| System.out.println("%n" + "=".repeat(80)); | |
| System.out.printf("%n%s%n", "=".repeat(80)); |
| .map(ForeignKey::fromColumn) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException( | ||
| "No FK from " + table + " to " + view.baseTable() + " found in foreignKeys")); |
There was a problem hiding this comment.
deriveWhereSourceCol only supports child tables that have a direct FK to view.baseTable() (it filters fk.toTable().equals(view.baseTable())). For multi-level dependency chains (e.g. customer → address → phone), generating UPDATE/DELETE triggers for the grandchild table will throw IllegalArgumentException and abort artifact generation. Either extend this to follow the FK chain back to the base table (e.g. via subqueries/joins), or validate and fail fast with a clearer message that only star schemas are supported.
Suggested change
| "No FK from " + table + " to " + view.baseTable() + " found in foreignKeys")); | |
| "No direct foreign key from child table '" + table + "' to base table '" + view.baseTable() | |
| + "' found in view.foreignKeys(). Only star-schema layouts with a direct FK to the base " | |
| + "table are supported for UPDATE/DELETE trigger generation; multi-level FK chains " | |
| + "(for example, customer → address → phone) are not supported by this generator.")); |
Comment on lines
+70
to
+73
| for (String profileCol : profileColumns) { | ||
| if (!mappedViewColumns.contains(profileCol)) { | ||
| System.err.println("WARNING: Column '" + profileCol + "' from source profile is not mapped in columnMappings"); | ||
| } |
There was a problem hiding this comment.
validate(...) prints warnings directly to System.err. This makes validator output hard to control in CLI/test runs and bypasses the project’s logging configuration. Prefer using SLF4J (or returning warnings to the caller) so the analyzer can decide how/when to surface them (e.g., respect quiet mode).
Comment on lines
107
to
114
| * Creates a Liquibase changeset for a multi-column index using LinkedHashSet. | ||
| * | ||
| * | ||
| * @param tableName the table name | ||
| * @param columns LinkedHashSet of column names in index order | ||
| * @param columns LinkedHashSet of column names in index order | ||
| * @return XML changeset string | ||
| */ | ||
| public String createMultiColumnIndexChangeset(String tableName, LinkedHashSet<String> columns) { | ||
| public String createMultiColumnIndexChangeset(String tableName, Set<String> columns) { | ||
| return createMultiColumnIndexChangeset(tableName, new ArrayList<>(columns)); |
There was a problem hiding this comment.
This overload now accepts a generic Set<String> but the Javadoc still states LinkedHashSet and the implementation relies on the set’s iteration order to preserve column order. Column order is semantically important for multi-column indexes; consider keeping the parameter type as LinkedHashSet (or Collection/List) or explicitly documenting/enforcing that the passed set must be ordered.
Comment on lines
+298
to
+312
| /** | ||
| * Reads {@code schema_normalization.max_continuations} from settings. | ||
| * Defaults to 3 if not configured. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| private int getMaxContinuations() { | ||
| Map<String, Object> normConfig = | ||
| (Map<String, Object>) Settings.getProperty(SCHEMA_NORMALIZATION); | ||
| if (normConfig != null) { | ||
| Object v = normConfig.get("max_continuations"); | ||
| if (v instanceof Integer i) return i; | ||
| if (v instanceof String s) return Integer.parseInt(s); | ||
| } | ||
| return 10; // enough for several binary splits of a large entity set | ||
| } |
There was a problem hiding this comment.
The Javadoc says the default for max_continuations is 3, but the code returns 10 when not configured. Update the comment or the default to avoid misleading configuration expectations.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+598
to
+632
| static String extractJson(String textResponse) { | ||
| String s = textResponse.trim(); | ||
|
|
||
| // Find the last code fence opening (```json or ``` followed by [ or {). | ||
| // When a model outputs a truncated attempt then the real response, the last | ||
| // fence opening is the one that introduces the complete JSON. | ||
| int lastFenceStart = -1; | ||
| lastFenceStart = getLastFenceStart(s, lastFenceStart); | ||
|
|
||
| if (lastFenceStart != -1) { | ||
| int newline = s.indexOf('\n', lastFenceStart); | ||
| if (newline != -1) { | ||
| String content = s.substring(newline + 1).stripLeading(); | ||
| int closingFence = content.lastIndexOf("```"); | ||
| if (closingFence != -1) { | ||
| content = content.substring(0, closingFence).stripTrailing(); | ||
| } | ||
| if (lastFenceStart > 0 && logger.isWarnEnabled()) { | ||
| logger.warn("AI response had {} chars of preamble/garbage before last code fence", | ||
| lastFenceStart); | ||
| } | ||
| return content.replace("`", ""); | ||
| } | ||
| } | ||
|
|
||
| // No code fence — find the last [ or { that starts on its own line, | ||
| // preferring the last occurrence to skip any truncated first attempt | ||
| int jsonStart = getJsonStart(s); | ||
|
|
||
| if (jsonStart > 0 && logger.isWarnEnabled()) { | ||
| logger.warn("AI response contains preamble text before JSON (skipping {} chars): {}", | ||
| jsonStart, s.substring(0, Math.min(jsonStart, 200))); | ||
| } | ||
| return s.substring(jsonStart).replace("`", ""); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The extractJson helper unconditionally removes all backtick characters from the extracted JSON, which will corrupt valid JSON whenever a string value legitimately contains backticks and can lead to hard-to-diagnose parsing or semantic errors. [logic error]
Severity Level: Critical 🚨
- ❌ AI recommendations corrupted when JSON strings contain backticks.
- ❌ Valid AI JSON may fail to parse unexpectedly.
- ⚠️ Debugging difficult since corruption happens silently.
Suggested change
| static String extractJson(String textResponse) { | |
| String s = textResponse.trim(); | |
| // Find the last code fence opening (```json or ``` followed by [ or {). | |
| // When a model outputs a truncated attempt then the real response, the last | |
| // fence opening is the one that introduces the complete JSON. | |
| int lastFenceStart = -1; | |
| lastFenceStart = getLastFenceStart(s, lastFenceStart); | |
| if (lastFenceStart != -1) { | |
| int newline = s.indexOf('\n', lastFenceStart); | |
| if (newline != -1) { | |
| String content = s.substring(newline + 1).stripLeading(); | |
| int closingFence = content.lastIndexOf("```"); | |
| if (closingFence != -1) { | |
| content = content.substring(0, closingFence).stripTrailing(); | |
| } | |
| if (lastFenceStart > 0 && logger.isWarnEnabled()) { | |
| logger.warn("AI response had {} chars of preamble/garbage before last code fence", | |
| lastFenceStart); | |
| } | |
| return content.replace("`", ""); | |
| } | |
| } | |
| // No code fence — find the last [ or { that starts on its own line, | |
| // preferring the last occurrence to skip any truncated first attempt | |
| int jsonStart = getJsonStart(s); | |
| if (jsonStart > 0 && logger.isWarnEnabled()) { | |
| logger.warn("AI response contains preamble text before JSON (skipping {} chars): {}", | |
| jsonStart, s.substring(0, Math.min(jsonStart, 200))); | |
| } | |
| return s.substring(jsonStart).replace("`", ""); | |
| } | |
| static String extractJson(String textResponse) { | |
| String s = textResponse.trim(); | |
| // Find the last code fence opening (```json or ``` followed by [ or {). | |
| // When a model outputs a truncated attempt then the real response, the last | |
| // fence opening is the one that introduces the complete JSON. | |
| int lastFenceStart = -1; | |
| lastFenceStart = getLastFenceStart(s, lastFenceStart); | |
| if (lastFenceStart != -1) { | |
| int newline = s.indexOf('\n', lastFenceStart); | |
| if (newline != -1) { | |
| String content = s.substring(newline + 1).stripLeading(); | |
| int closingFence = content.lastIndexOf("```"); | |
| if (closingFence != -1) { | |
| content = content.substring(0, closingFence).stripTrailing(); | |
| } | |
| if (lastFenceStart > 0 && logger.isWarnEnabled()) { | |
| logger.warn("AI response had {} chars of preamble/garbage before last code fence", | |
| lastFenceStart); | |
| } | |
| return content; | |
| } | |
| } | |
| // No code fence — find the last [ or { that starts on its own line, | |
| // preferring the last occurrence to skip any truncated first attempt | |
| int jsonStart = getJsonStart(s); | |
| if (jsonStart > 0 && logger.isWarnEnabled()) { | |
| logger.warn("AI response contains preamble text before JSON (skipping {} chars): {}", | |
| jsonStart, s.substring(0, Math.min(jsonStart, 200))); | |
| } | |
| return s.substring(jsonStart); | |
| } |
Steps of Reproduction ✅
1. A concrete AI service implementation calls
`AbstractAIService.analyzeQueryBatch(QueryBatch batch)`
(src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java), which
builds a payload, sends it to the AI provider, receives a textual response, and delegates
response parsing to its `parseResponse(...)` implementation.
2. That `parseResponse(...)` implementation uses the shared helper
`parseRecommendations(String textResponse, QueryBatch batch)`, passing the raw AI
response, which includes JSON wrapped in a Markdown fence and containing backticks in a
string value, e.g. `"Column name uses MySQL identifier `user_id`."`.
3. Inside `parseRecommendations(...)`, `extractJson(textResponse)` (lines 598–632) trims
the response, locates the fenced JSON, and returns the JSON content with *all* backtick
characters removed via `.replace("`", "")`, transforming the payload (e.g., ``"Column name
uses MySQL identifier `user_id`."`` becomes `"Column name uses MySQL identifier
user_id."`).
4. `parseRecommendationsFromJson(jsonResponse, batch)` is then called with this modified
JSON; `objectMapper.readTree(jsonResponse)` either fails to parse valid JSON that depended
on backticks or, more subtly, parses successfully but yields corrupted field values (e.g.,
notes text), causing incorrect or misleading `OptimizationIssue` instances to be returned
to callers of `analyzeQueryBatch(...)`.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:** 598:632
**Comment:**
*Logic Error: The extractJson helper unconditionally removes all backtick characters from the extracted JSON, which will corrupt valid JSON whenever a string value legitimately contains backticks and can lead to hard-to-diagnose parsing or semantic errors.
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
+54
to
+107
| public int analyze() throws IOException, ReflectiveOperationException, InterruptedException { | ||
| checkResumptionState(); | ||
|
|
||
| Map<String, TypeWrapper> resolvedTypes = AntikytheraRunTime.getResolvedTypes(); | ||
| int totalTypes = 0; | ||
| int typesProcessed = 0; | ||
| typesResumed = 0; | ||
| typesSkippedByFilter = 0; | ||
|
|
||
| logger.debug("targetClass filter value: {}", targetClass); | ||
| logger.debug("skipClass filter value: {}", skipClass); | ||
|
|
||
| for (Map.Entry<String, TypeWrapper> entry : resolvedTypes.entrySet()) { | ||
| String fullyQualifiedName = entry.getKey(); | ||
| TypeWrapper typeWrapper = entry.getValue(); | ||
|
|
||
| if (shouldProcess(typeWrapper)) { | ||
| totalTypes++; | ||
|
|
||
| if (shouldSkipType(fullyQualifiedName)) continue; | ||
|
|
||
| System.out.println("%n" + "=".repeat(80)); | ||
| System.out.printf("Analyzing: %s%n", fullyQualifiedName); | ||
| System.out.println("=".repeat(80)); | ||
| try { | ||
| analyzeType(typeWrapper); | ||
| typesProcessed++; | ||
|
|
||
| checkpointManager.markProcessed(fullyQualifiedName); | ||
| onBeforeCheckpointSave(); | ||
| checkpointManager.save(); | ||
| } catch (AntikytheraException ae) { | ||
| logger.error("Error analyzing {}: {}", fullyQualifiedName, ae.getMessage()); | ||
| onBeforeCheckpointSave(); | ||
| checkpointManager.save(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| afterAnalysisLoop(); | ||
|
|
||
| // Print summary | ||
| if (typesSkippedByFilter > 0) { | ||
| System.out.printf("%n✅ Analyzed %d types, skipped %d by target_class filter (total: %d)%n", | ||
| typesProcessed, typesSkippedByFilter, totalTypes); | ||
| } else if (typesResumed > 0) { | ||
| System.out.printf("%n✅ Analyzed %d types, skipped %d from checkpoint (total: %d)%n", | ||
| typesProcessed, typesResumed, totalTypes); | ||
| } else { | ||
| System.out.printf("%n✅ Successfully analyzed %d types%n", typesProcessed); | ||
| } | ||
|
|
||
| checkpointManager.clear(); | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: The analysis workflow unconditionally uses the checkpointManager (load, markProcessed, save, clear) but the abstract base class never initializes it, so if a subclass forgets to set it before calling analyze() a NullPointerException will occur instead of a clear error. [null pointer]
Severity Level: Major ⚠️
- ❌ Any analyzer using base class can crash on startup.
- ⚠️ Schema normalization CLI may fail with opaque NPE.
- ⚠️ Debugging misconfigured analyzers becomes harder for users.
Suggested change
| public int analyze() throws IOException, ReflectiveOperationException, InterruptedException { | |
| checkResumptionState(); | |
| Map<String, TypeWrapper> resolvedTypes = AntikytheraRunTime.getResolvedTypes(); | |
| int totalTypes = 0; | |
| int typesProcessed = 0; | |
| typesResumed = 0; | |
| typesSkippedByFilter = 0; | |
| logger.debug("targetClass filter value: {}", targetClass); | |
| logger.debug("skipClass filter value: {}", skipClass); | |
| for (Map.Entry<String, TypeWrapper> entry : resolvedTypes.entrySet()) { | |
| String fullyQualifiedName = entry.getKey(); | |
| TypeWrapper typeWrapper = entry.getValue(); | |
| if (shouldProcess(typeWrapper)) { | |
| totalTypes++; | |
| if (shouldSkipType(fullyQualifiedName)) continue; | |
| System.out.println("%n" + "=".repeat(80)); | |
| System.out.printf("Analyzing: %s%n", fullyQualifiedName); | |
| System.out.println("=".repeat(80)); | |
| try { | |
| analyzeType(typeWrapper); | |
| typesProcessed++; | |
| checkpointManager.markProcessed(fullyQualifiedName); | |
| onBeforeCheckpointSave(); | |
| checkpointManager.save(); | |
| } catch (AntikytheraException ae) { | |
| logger.error("Error analyzing {}: {}", fullyQualifiedName, ae.getMessage()); | |
| onBeforeCheckpointSave(); | |
| checkpointManager.save(); | |
| } | |
| } | |
| } | |
| afterAnalysisLoop(); | |
| // Print summary | |
| if (typesSkippedByFilter > 0) { | |
| System.out.printf("%n✅ Analyzed %d types, skipped %d by target_class filter (total: %d)%n", | |
| typesProcessed, typesSkippedByFilter, totalTypes); | |
| } else if (typesResumed > 0) { | |
| System.out.printf("%n✅ Analyzed %d types, skipped %d from checkpoint (total: %d)%n", | |
| typesProcessed, typesResumed, totalTypes); | |
| } else { | |
| System.out.printf("%n✅ Successfully analyzed %d types%n", typesProcessed); | |
| } | |
| checkpointManager.clear(); | |
| public int analyze() throws IOException, ReflectiveOperationException, InterruptedException { | |
| if (checkpointManager == null) { | |
| throw new IllegalStateException("CheckpointManager must be initialized before calling analyze()"); | |
| } | |
| checkResumptionState(); | |
| Map<String, TypeWrapper> resolvedTypes = AntikytheraRunTime.getResolvedTypes(); | |
| int totalTypes = 0; | |
| int typesProcessed = 0; | |
| typesResumed = 0; | |
| typesSkippedByFilter = 0; | |
| logger.debug("targetClass filter value: {}", targetClass); | |
| logger.debug("skipClass filter value: {}", skipClass); | |
| for (Map.Entry<String, TypeWrapper> entry : resolvedTypes.entrySet()) { | |
| String fullyQualifiedName = entry.getKey(); | |
| TypeWrapper typeWrapper = entry.getValue(); | |
| if (shouldProcess(typeWrapper)) { | |
| totalTypes++; | |
| if (shouldSkipType(fullyQualifiedName)) continue; | |
| System.out.println("%n" + "=".repeat(80)); | |
| System.out.printf("Analyzing: %s%n", fullyQualifiedName); | |
| System.out.println("=".repeat(80)); | |
| try { | |
| analyzeType(typeWrapper); | |
| typesProcessed++; | |
| checkpointManager.markProcessed(fullyQualifiedName); | |
| onBeforeCheckpointSave(); | |
| checkpointManager.save(); | |
| } catch (AntikytheraException ae) { | |
| logger.error("Error analyzing {}: {}", fullyQualifiedName, ae.getMessage()); | |
| onBeforeCheckpointSave(); | |
| checkpointManager.save(); | |
| } | |
| } | |
| } | |
| afterAnalysisLoop(); | |
| // Print summary | |
| if (typesSkippedByFilter > 0) { | |
| System.out.printf("%n✅ Analyzed %d types, skipped %d by target_class filter (total: %d)%n", | |
| typesProcessed, typesSkippedByFilter, totalTypes); | |
| } else if (typesResumed > 0) { | |
| System.out.printf("%n✅ Analyzed %d types, skipped %d from checkpoint (total: %d)%n", | |
| typesProcessed, typesResumed, totalTypes); | |
| } else { | |
| System.out.printf("%n✅ Successfully analyzed %d types%n", typesProcessed); | |
| } | |
| checkpointManager.clear(); | |
| return typesProcessed; | |
| } |
Steps of Reproduction ✅
1. Implement a concrete analyzer class (e.g., `MyAnalyzer`) extending
`AbstractRepositoryAnalyzer` in
`src/main/java/sa/com/cloudsolutions/antikythera/examples/`, overriding
`shouldProcess(..)` and `analyzeType(..)` but not assigning any value to the inherited
`checkpointManager` field.
2. In any entry point (e.g., a main method or test), construct `new MyAnalyzer()` and call
its `analyze()` method; this invokes `AbstractRepositoryAnalyzer.analyze()` at
`AbstractRepositoryAnalyzer.java:54`.
3. Inside `analyze()`, the first call is to `checkResumptionState()` (line 55), which
directly calls `checkpointManager.load()` in `checkResumptionState()` at
`AbstractRepositoryAnalyzer.java:196` without a null check.
4. Because `checkpointManager` was never initialized in `MyAnalyzer`, the dereference in
`checkpointManager.load()` throws a `NullPointerException`, aborting the whole analysis
run before any types are processed or a clear configuration error is reported.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java
**Line:** 54:107
**Comment:**
*Null Pointer: The analysis workflow unconditionally uses the checkpointManager (load, markProcessed, save, clear) but the abstract base class never initializes it, so if a subclass forgets to set it before calling analyze() a NullPointerException will occur instead of a clear error.
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
+549
to
+563
| jsonText = candidates.get(0).path(CONTENT).path(PARTS).get(0).path("text").asText(); | ||
| } else { | ||
| JsonNode choices = root.path("choices"); | ||
| if (!choices.isArray() || choices.isEmpty()) return null; | ||
| jsonText = choices.get(0).path("message").path(CONTENT).asText(); | ||
| } | ||
| jsonText = stripMarkdownFences(jsonText); | ||
| logger.info("=== LLM EXTRACTED JSON ===\n{}", jsonText == null ? "null" : jsonText); | ||
| return jsonText; | ||
| } | ||
|
|
||
| /** | ||
| * Removes markdown code fences that some models wrap around JSON output, | ||
| * e.g. {@code ```json\n[...]\n```} → {@code [...]}. | ||
| * Also trims any leading/trailing whitespace or prose outside the JSON block. |
Contributor
There was a problem hiding this comment.
Suggestion: In the Gemini response parsing branch, the code assumes that the parts array and its first element always exist, so when Gemini returns a candidate without parts or with an empty array, get(0) returns null and the subsequent path("text") call will trigger a NullPointerException; use path() with proper array checks to avoid this and simply treat such responses as empty. [null pointer]
Severity Level: Critical 🚨
- ❌ Schema normalization run crashes on malformed Gemini responses.
- ⚠️ No normalization reports produced when crash occurs.
- ⚠️ Migration artifact generation never executes after such failures.
Suggested change
| jsonText = candidates.get(0).path(CONTENT).path(PARTS).get(0).path("text").asText(); | |
| } else { | |
| JsonNode choices = root.path("choices"); | |
| if (!choices.isArray() || choices.isEmpty()) return null; | |
| jsonText = choices.get(0).path("message").path(CONTENT).asText(); | |
| } | |
| jsonText = stripMarkdownFences(jsonText); | |
| logger.info("=== LLM EXTRACTED JSON ===\n{}", jsonText == null ? "null" : jsonText); | |
| return jsonText; | |
| } | |
| /** | |
| * Removes markdown code fences that some models wrap around JSON output, | |
| * e.g. {@code ```json\n[...]\n```} → {@code [...]}. | |
| * Also trims any leading/trailing whitespace or prose outside the JSON block. | |
| private String extractJsonText(String responseBody) throws IOException { | |
| JsonNode root = objectMapper.readTree(responseBody); | |
| String jsonText; | |
| if (aiService instanceof GeminiAIService) { | |
| JsonNode candidates = root.path("candidates"); | |
| if (!candidates.isArray() || candidates.isEmpty()) { | |
| return null; | |
| } | |
| JsonNode firstCandidate = candidates.path(0); | |
| JsonNode contentNode = firstCandidate.path(CONTENT); | |
| JsonNode partsNode = contentNode.path(PARTS); | |
| if (!partsNode.isArray() || partsNode.isEmpty()) { | |
| return null; | |
| } | |
| jsonText = partsNode.path(0).path("text").asText(); | |
| } else { | |
| JsonNode choices = root.path("choices"); | |
| if (!choices.isArray() || choices.isEmpty()) return null; | |
| jsonText = choices.get(0).path("message").path(CONTENT).asText(); | |
| } | |
| jsonText = stripMarkdownFences(jsonText); | |
| logger.info("=== LLM EXTRACTED JSON ===\n{}", jsonText == null ? "null" : jsonText); | |
| return jsonText; | |
| } |
Steps of Reproduction ✅
1. Configure the AI service so that `AIServiceFactory.create(...)` returns a
`GeminiAIService` instance (used in `SchemaNormalizationAnalyzer` constructor at
`SchemaNormalizationAnalyzer.java:165-176` via `Settings.getProperty("ai_service")`).
2. Run the CLI entry point `SchemaNormalizationAnalyzer.main()` at
`SchemaNormalizationAnalyzer.java:1359-1368`, which calls `analyzer.analyze()` and then
`afterAnalysisLoop()` at `SchemaNormalizationAnalyzer.java:245-296`.
3. Ensure at least one entity profile is collected so `afterAnalysisLoop()` calls
`sendBatchToLLM(allProfiles)` at `SchemaNormalizationAnalyzer.java:265`, which then calls
`aiService.sendRawRequest(...)` and passes the returned body to
`parseBatchResponse(response)` at `SchemaNormalizationAnalyzer.java:426-427`.
4. Have the configured `GeminiAIService` return a JSON body where the first candidate has
no `parts` array (e.g. `{"candidates":[{"content":{}}]}`); when `parseBatchResponse()`
calls `extractJsonText()` at `SchemaNormalizationAnalyzer.java:548`, the Gemini branch at
`SchemaNormalizationAnalyzer.java:552-555` executes, `root.path("candidates")` is a
non-empty array, `candidates.get(0).path(CONTENT).path(PARTS)` yields a `MissingNode`,
`get(0)` on that returns `null`, and `.path("text")` on `null` throws a
`NullPointerException`, terminating the analysis process.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:** 549:563
**Comment:**
*Null Pointer: In the Gemini response parsing branch, the code assumes that the `parts` array and its first element always exist, so when Gemini returns a candidate without `parts` or with an empty array, `get(0)` returns null and the subsequent `path("text")` call will trigger a NullPointerException; use `path()` with proper array checks to avoid this and simply treat such responses as empty.
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
+1056
to
+1065
| generateNewEntity(plan, sourceRoot); | ||
| } | ||
| } | ||
|
|
||
| private void generateNewEntity(DataMigrationPlan plan, Path sourceRoot) throws IOException { | ||
| // Locate the source entity profile by its table name | ||
| EntityProfile sourceProfile = allProfiles.stream() | ||
| .filter(p -> p.tableName().equals(plan.sourceTable())) | ||
| .findFirst() | ||
| .orElse(null); |
Contributor
There was a problem hiding this comment.
Suggestion: The entity generation routine assumes that base_path is always configured and immediately calls toString() on the retrieved setting, so when base_path is missing, a NullPointerException will occur; guard against a null value and skip generation (with a warning) instead. [null pointer]
Severity Level: Critical 🚨
- ❌ Entity source generation crashes when base_path is unset.
- ⚠️ No normalized entity classes generated for migration plans.
- ⚠️ Overall normalization workflow interrupted after changeset generation.
Suggested change
| generateNewEntity(plan, sourceRoot); | |
| } | |
| } | |
| private void generateNewEntity(DataMigrationPlan plan, Path sourceRoot) throws IOException { | |
| // Locate the source entity profile by its table name | |
| EntityProfile sourceProfile = allProfiles.stream() | |
| .filter(p -> p.tableName().equals(plan.sourceTable())) | |
| .findFirst() | |
| .orElse(null); | |
| private void generateNewEntities(List<DataMigrationPlan> plans) throws IOException { | |
| Object basePathProp = Settings.getProperty(BASE_PATH); | |
| if (basePathProp == null) { | |
| logger.warn("base_path not configured — skipping generated entity source files"); | |
| return; | |
| } | |
| Path sourceRoot = Path.of(basePathProp.toString(), "src", "main", "java"); | |
| System.out.printf("%n📝 Generating new entity classes (persistence: %s)…%n", persistencePackage); | |
| for (DataMigrationPlan plan : plans) { | |
| generateNewEntity(plan, sourceRoot); | |
| } | |
| } |
Steps of Reproduction ✅
1. Run the CLI entry point `SchemaNormalizationAnalyzer.main()` at
`SchemaNormalizationAnalyzer.java:1359-1368`, which calls `Settings.loadConfigMap()` and
then `SchemaNormalizationAnalyzer.analyze()` followed by `afterAnalysisLoop()` at
`SchemaNormalizationAnalyzer.java:245-296`.
2. Use a configuration where `Settings.getProperty("schema_normalization")` is present and
the LLM returns at least one `NormalizationIssue` with a non-null `dataMigrationPlan`, so
that `getDataMigrationPlans()` at `SchemaNormalizationAnalyzer.java:894-910` returns a
non-empty list.
3. Ensure that the top-level `base_path` setting is **not** defined (so
`Settings.getProperty(BASE_PATH)` returns `null` when called in `generateNewEntities()` at
`SchemaNormalizationAnalyzer.java:1057`).
4. During `generateMigrationArtifacts()` at `SchemaNormalizationAnalyzer.java:817-890`,
after building and writing Liquibase changesets, the method calls
`generateNewEntities(plans)` at `SchemaNormalizationAnalyzer.java:889`; inside
`generateNewEntities`, `basePathProp` is `null`, `basePathProp.toString()` at
`SchemaNormalizationAnalyzer.java:1059` throws a `NullPointerException`, and the whole
normalization run terminates during entity generation.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:** 1056:1065
**Comment:**
*Null Pointer: The entity generation routine assumes that `base_path` is always configured and immediately calls `toString()` on the retrieved setting, so when `base_path` is missing, a NullPointerException will occur; guard against a null value and skip generation (with a warning) instead.
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
495
to
501
| private String getDropIndexSql(DatabaseDialect dialect, String indexName) { | ||
| return switch (dialect) { | ||
| case POSTGRESQL -> "DROP INDEX CONCURRENTLY IF EXISTS " + indexName + ";"; | ||
| case ORACLE, MYSQL -> "DROP INDEX " + indexName + ";"; | ||
| case H2 -> "DROP INDEX IF EXISTS " + indexName + ";"; | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: For the MySQL dialect, the generated DROP INDEX SQL is syntactically invalid because MySQL requires the table name (DROP INDEX idx_name ON table_name), but this helper only outputs DROP INDEX idx_name;, which will fail at migration time; since the method lacks the table name, a safer fix is to fail fast for MySQL instead of emitting invalid SQL so callers can handle MySQL index drops explicitly. [logic error]
Severity Level: Major ⚠️
- ❌ MySQL migrations fail when dropping indexes via generator.
- ❌ MySQL index-creation rollbacks generate invalid DROP INDEX SQL.
- ⚠️ Schema normalization tooling unreliable on MySQL deployments.
- ⚠️ CI migrations to MySQL break when dialect enabled.
Suggested change
| private String getDropIndexSql(DatabaseDialect dialect, String indexName) { | |
| return switch (dialect) { | |
| case POSTGRESQL -> "DROP INDEX CONCURRENTLY IF EXISTS " + indexName + ";"; | |
| case ORACLE, MYSQL -> "DROP INDEX " + indexName + ";"; | |
| case H2 -> "DROP INDEX IF EXISTS " + indexName + ";"; | |
| }; | |
| } | |
| private String getDropIndexSql(DatabaseDialect dialect, String indexName) { | |
| return switch (dialect) { | |
| case POSTGRESQL -> "DROP INDEX CONCURRENTLY IF EXISTS " + indexName + ";"; | |
| case ORACLE -> "DROP INDEX " + indexName + ";"; | |
| case MYSQL -> throw new UnsupportedOperationException( | |
| "Dropping indexes for MySQL requires the table name; this helper does not support it."); | |
| case H2 -> "DROP INDEX IF EXISTS " + indexName + ";"; | |
| }; | |
| } |
Steps of Reproduction ✅
1. Create a ChangesetConfig including MySQL in supported dialects by constructing it
directly at `LiquibaseGenerator.java:651-704`, e.g. `new ChangesetConfig("antikythera",
Set.of(DatabaseDialect.MYSQL), true, true, "/path/to/master.xml", "changes")`.
2. Instantiate the generator with that config at `LiquibaseGenerator.java:40-47` using
`LiquibaseGenerator generator = new LiquibaseGenerator(config);`.
3. Call `generator.createDropIndexChangeset("idx_test")` defined at
`LiquibaseGenerator.java:118-153`; its loop over `config.supportedDialects()` invokes
`getDropIndexSql(DatabaseDialect.MYSQL, "idx_test")` at `LiquibaseGenerator.java:495-501`.
4. Observe that the generated Liquibase changeset for MySQL contains `DROP INDEX
idx_test;` (no table name); when this changeset is applied by Liquibase against a MySQL
database, MySQL rejects the statement because valid syntax requires `DROP INDEX idx_test
ON some_table`, causing the migration step to fail.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java
**Line:** 495:501
**Comment:**
*Logic Error: For the MySQL dialect, the generated `DROP INDEX` SQL is syntactically invalid because MySQL requires the table name (`DROP INDEX idx_name ON table_name`), but this helper only outputs `DROP INDEX idx_name;`, which will fail at migration time; since the method lacks the table name, a safer fix is to fail fast for MySQL instead of emitting invalid SQL so callers can handle MySQL index drops explicitly.
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
+174
to
+187
| sb.append(" <column name=\"").append(cm.sourceColumn()) | ||
| .append("\" type=\"").append(lbType).append("\""); | ||
|
|
||
| if (isId || !nullable) { | ||
| sb.append(">\n"); | ||
| sb.append(" <constraints"); | ||
| if (isId) sb.append(" primaryKey=\"true\""); | ||
| sb.append(" nullable=\"false\""); | ||
| if (isId) sb.append(" autoIncrement=\"true\""); | ||
| sb.append("/>\n"); | ||
| sb.append(" </column>\n"); | ||
| } else { | ||
| sb.append("/>\n"); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: In the generated Liquibase XML, the autoIncrement attribute is incorrectly placed on the <constraints> tag instead of the <column> tag, which does not match Liquibase's schema and can cause validation or runtime errors when applying the changelog. The fix is to set autoIncrement="true" as an attribute of the <column> element (when the column is an ID) and keep only primary key / nullable flags on <constraints>. [logic error]
Severity Level: Critical 🚨
- ❌ Liquibase rejects generated changeSets for tables with ID columns.
- ❌ Schema normalization Liquibase workflow broken without manual XML editing.
- ⚠️ Example normalization tooling unusable for Liquibase-based migrations.
Suggested change
| sb.append(" <column name=\"").append(cm.sourceColumn()) | |
| .append("\" type=\"").append(lbType).append("\""); | |
| if (isId || !nullable) { | |
| sb.append(">\n"); | |
| sb.append(" <constraints"); | |
| if (isId) sb.append(" primaryKey=\"true\""); | |
| sb.append(" nullable=\"false\""); | |
| if (isId) sb.append(" autoIncrement=\"true\""); | |
| sb.append("/>\n"); | |
| sb.append(" </column>\n"); | |
| } else { | |
| sb.append("/>\n"); | |
| } | |
| sb.append(" <column name=\"").append(cm.sourceColumn()) | |
| .append("\" type=\"").append(lbType).append("\""); | |
| if (isId || !nullable) { | |
| if (isId) { | |
| sb.append(" autoIncrement=\"true\""); | |
| } | |
| sb.append(">\n"); | |
| sb.append(" <constraints"); | |
| if (isId) sb.append(" primaryKey=\"true\""); | |
| sb.append(" nullable=\"false\""); | |
| sb.append("/>\n"); | |
| sb.append(" </column>\n"); | |
| } else { | |
| sb.append("/>\n"); | |
| } |
Steps of Reproduction ✅
1. In `NormalizedTableDDLGenerator.generate(...)` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java:84`,
call the method with `ddlMode` set to `MODE_LIQUIBASE` (constant defined at line 28) and a
`ViewDescriptor` that includes at least one `ColumnMapping` whose `viewColumn()`
corresponds to an ID field in the provided `EntityProfile`.
2. During `generate(...)`, the FK-sorted table order is computed at lines 102–104 and for
each table with column mappings, `buildLiquibaseChangeset(...)` is invoked in the `else`
branch at line 113 because `MODE_RAW_SQL.equals(ddlMode)` is false.
3. Inside `buildLiquibaseChangeset(...)` (method starting at line 161), for each column
mapping the code at lines 169–175 computes `isId` from `isIdMap` and appends the
`<column>` start tag; for an ID column, execution enters the `if (isId || !nullable)`
block at line 177 and appends `<constraints ... autoIncrement="true"/>` at line 182,
placing `autoIncrement` on the `<constraints>` element rather than on `<column>`.
4. Take the generated Liquibase XML string returned from `generate(...)` (for example in a
unit test or a small driver program) and feed it to the Liquibase CLI as a changelog
(e.g., `liquibase updateSql`); Liquibase schema validation fails because `autoIncrement`
is not a valid attribute on `<constraints>`, causing the changelog application to error
out for any table whose ID column is marked as auto-increment.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java
**Line:** 174:187
**Comment:**
*Logic Error: In the generated Liquibase XML, the `autoIncrement` attribute is incorrectly placed on the `<constraints>` tag instead of the `<column>` tag, which does not match Liquibase's schema and can cause validation or runtime errors when applying the changelog. The fix is to set `autoIncrement="true"` as an attribute of the `<column>` element (when the column is an ID) and keep only primary key / nullable flags on `<constraints>`.
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java (3)
1208-1215:⚠️ Potential issue | 🟠 MajorDon't skip Liquibase generation just because no new repositories ran.
restoreCheckpointState()can repopulate pending index suggestions from a previous interrupted run. In that caseanalyze()returns0, this branch skips file generation, and the recovered suggestions are lost.generateLiquibaseChangesFile()already returns early when there is nothing to write, so it is safe to call unconditionally.💡 Suggested fix
- QueryOptimizationChecker checker = new QueryOptimizationChecker(getLiquibasePath()); - int repositoriesAnalyzed = checker.analyze(); - - // Only generate Liquibase file if at least one repository was analyzed - if (repositoriesAnalyzed > 0) { - checker.generateLiquibaseChangesFile(); - } else { - System.out.println("\n⏭️ Skipping Liquibase generation - no new repositories were analyzed"); - } + QueryOptimizationChecker checker = new QueryOptimizationChecker(getLiquibasePath()); + checker.analyze(); + checker.generateLiquibaseChangesFile();🤖 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 1208 - 1215, The current logic skips calling QueryOptimizationChecker.generateLiquibaseChangesFile() when analyze() returns 0, which loses suggestions recovered by restoreCheckpointState(); remove the repositoriesAnalyzed check and always invoke checker.generateLiquibaseChangesFile() after checker.analyze() so recovered pending suggestions are written (generateLiquibaseChangesFile() already no-ops when there is nothing to write). Keep the existing informational print only if you still want a special message, but do not gate the call to generateLiquibaseChangesFile() on repositoriesAnalyzed; use the QueryOptimizationChecker methods (analyze(), generateLiquibaseChangesFile(), restoreCheckpointState()) as the touch points.
1095-1107:⚠️ Potential issue | 🟠 MajorThis drop heuristic is too aggressive for generated migrations.
getDropCandidates()proposes dropping every non-emptyINDEXwhose first column isLOW, but that does not prove the index is redundant. Composite indexes like(status, customer_id)can still be useful, and unique / FK-supporting indexes are especially risky to turn into automatic drop changesets.🤖 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 1095 - 1107, The drop heuristic in getDropCandidates() is too aggressive: only consider an index for dropping if it is a single-column non-unique, non-FK-supporting index whose first column has LOW cardinality. Update the loop that inspects Indexes.IndexInfo (idx) to skip any index where idx.columns() has size > 1, or where idx indicates uniqueness or FK support (e.g., idx.isUnique(), idx.isPrimary(), idx.supportsForeignKey() or equivalent metadata on IndexInfo), and only then run CardinalityAnalyzer.analyzeColumnCardinality(table, first) and add idx.name() to dropCandidates; keep existing checks for idx.type(), non-empty columns and non-empty name. Ensure you reference getDropCandidates, CardinalityAnalyzer, and Indexes.IndexInfo when making the changes.
297-299:⚠️ Potential issue | 🟡 MinorUse a consistent accessor style for
WhereConditionwithin each statement.Both
WhereConditionaccessor styles exist and work correctly: the record-styletableName()and the bean-stylegetTableName()coexist intentionally as backward-compatible alternatives. However, lines 298 and 685 mix them redundantly in the same expression (condition.tableName() == null ? ... : condition.getTableName()), reducing code clarity.Choose one accessor style and use it consistently:
- Option A: All record-style →
condition.tableName() == null ? ... : condition.tableName()- Option B: All bean-style →
condition.getTableName() == null ? ... : condition.getTableName()Same applies to line 685.
🤖 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 297 - 299, In QueryOptimizationChecker, use a single accessor style for WhereCondition: replace the mixed ternary using condition.tableName() and condition.getTableName() with a consistent call (either always record-style tableName() or always bean-style getTableName()) in the for loop and the other occurrence; e.g., change the expression to condition.tableName() == null ? rawQuery.getPrimaryTable() : condition.tableName() (or the equivalent using getTableName()) so both reads use the same accessor consistently.
🧹 Nitpick comments (5)
src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java (1)
169-196: Consider adding assertion for warning verification.The test documents that a warning should be printed to stderr, but only asserts that validation passes. While the current assertion is correct for verifying the non-fatal behavior, you might want to capture and verify the warning output to ensure the warning mechanism works as expected.
💡 Optional: Verify warning is logged
If using a logging framework like SLF4J with a test appender, you could capture and assert on the warning message:
// Example using a custom LogCaptor or similar test utility `@Test` void testMissingProfileColumnGeneratesWarning() { // ... existing setup ... // Capture logs and verify warning contains "email" assertDoesNotThrow(() -> DataMigrationPlanValidator.validate(plan, profile)); // Optionally: verify warning was logged for unmapped column }🤖 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, Add an assertion that verifies the warning for the unmapped "email" column is actually emitted: capture stderr or the logger output around the call to DataMigrationPlanValidator.validate(plan, profile) in testMissingProfileColumnGeneratesWarning and assert the captured output contains "Column 'email' from source profile is not mapped" (or the exact warning text produced by DataMigrationPlanValidator). Use the test harness's stream capture (e.g., System.err capture) or your logging test appender for SLF4J to wrap the validate(...) invocation and then assert the warning is present while still keeping assertDoesNotThrow for validation success.src/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.java (1)
38-41: Test documents potentially problematic behavior.This test case shows that when text contains
[not json]before actual JSON{\"id\": 1}, the extraction returns[not json] then {\"id\": 1}- which is not valid JSON. While this documents current behavior, downstream parsing would fail on this output.This may be acceptable if the calling code handles parse failures gracefully, but worth noting as an edge case where
extractJsonproduces invalid JSON.💡 Consider adding a comment
Arguments.of( // Edge case: first [ is not JSON; downstream parsing will fail but extractJson doesn't validate "Some text [not json] then {\"id\": 1}", "[not json] then {\"id\": 1}" ),🤖 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/AbstractAIServiceTest.java` around lines 38 - 41, Add an inline comment to the Arguments entry in AbstractAIServiceTest that documents this edge case: note that the input "Some text [not json] then {\"id\": 1}" leads extractJson to return "[not json] then {\"id\": 1}" which is not valid JSON and will cause downstream parsing to fail unless callers handle parse errors; place the comment next to the Arguments.of(...) tuple so future readers of the test and maintainers of extractJson (or callers of extractJson) understand this known limitation.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorter.java (2)
63-72: Minor:getOrDefaultfallback is unnecessary.Since all tables are pre-initialized in the
childrenmap (line 40), theList.of()fallback will never be used for tables in the result. This is defensive but could be simplified.♻️ Minor simplification
- for (String child : children.getOrDefault(current, List.of())) { + for (String child : children.get(current)) {🤖 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/TopologicalSorter.java` around lines 63 - 72, The fallback List.of() in the loop is unnecessary since the children map is fully pre-initialized; replace children.getOrDefault(current, List.of()) with children.get(current) in the for-loop inside the TopologicalSorter routine (the block using queue.poll(), result.add(current), for (String child : ...)), removing the defensive fallback and relying on the initialized children entries and existing inDegree handling.
44-51: Minor:computeIfAbsentis redundant here.Since line 40 already initializes all tables in the
childrenmap with empty lists viaputIfAbsent, thecomputeIfAbsenton line 49 will never create a new list for valid tables. The condition on line 48 already skips edges where parent/child aren't ininDegree, sochildren.get(parent)would suffice.♻️ Minor simplification
for (ForeignKey fk : foreignKeys) { String parent = fk.toTable(); String child = fk.fromTable(); // Only consider edges between tables in our set if (!inDegree.containsKey(parent) || !inDegree.containsKey(child)) continue; - children.computeIfAbsent(parent, k -> new ArrayList<>()).add(child); + children.get(parent).add(child); inDegree.merge(child, 1, Integer::sum); }🤖 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/TopologicalSorter.java` around lines 44 - 51, In TopologicalSorter, the loop that processes ForeignKey edges uses children.computeIfAbsent(parent, ...) despite earlier initializing every table in the children map; replace the computeIfAbsent call with children.get(parent).add(child) (after the existing guard that checks inDegree contains parent/child) so you use the pre-initialized list directly and keep the inDegree.merge(child, 1, Integer::sum) logic unchanged.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.java (1)
67-73: Minor naming inconsistency:sourceTable()vs. naming convention.The
ColumnMappingrecord fieldsourceTable()actually refers to the target/destination table (the normalized table receiving data), not the source. The grouping logic is correct, but the field naming inColumnMapping(defined inInsteadOfTriggerGenerator) may cause confusion. Consider adding a brief comment clarifying this.📝 Clarifying comment
private Map<String, List<ColumnMapping>> groupByTable(ViewDescriptor view) { + // Groups mappings by their destination (normalized) table. + // Note: ColumnMapping.sourceTable() refers to the target table in the normalization context. Map<String, List<ColumnMapping>> result = new LinkedHashMap<>(); for (ColumnMapping cm : view.columnMappings()) { result.computeIfAbsent(cm.sourceTable(), k -> new ArrayList<>()).add(cm); } return result; }🤖 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/DataMigrationGenerator.java` around lines 67 - 73, The grouping uses ColumnMapping.sourceTable(), but that field actually represents the destination/normalized table rather than the original source; add a brief clarifying comment either on the ColumnMapping record in InsteadOfTriggerGenerator and/or directly above groupByTable(ViewDescriptor view) explaining that sourceTable() is the target/destination table receiving normalized data to avoid confusion when reading groupByTable and related logic.
🤖 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`:
- Line 34: The plan uses repository-prefixed paths in the "New file" entries
(e.g.,
antikythera-examples/src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java)
which will produce incorrect file locations; update each "New file" entry
(including the one referencing DataMigrationPlanValidator.java and the other at
57-57) to use repo-relative paths starting at src/ (e.g., src/main/java/...),
ensuring all future entries follow that format so agents create files in the
correct repository location.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Line 619: The current return line in AbstractAIService that uses
content.replace("`", "") will remove all backticks and can corrupt JSON string
values; change it to only strip leading/trailing fence/backtick runs and
preserve interior backticks — e.g., trim the content and use a regex like
content = content.strip(); return content.replaceAll("^`+|`+$", "");
additionally consider validating that the trimmed content begins with "{" or "["
(or attempt JSON parse) and skip any stripping if the JSON is already
well-formed to avoid unnecessary modification.
- Around line 548-551: The current condition combines the empty-response check
with logger.isWarnEnabled(), causing a silent fall-through when jsonResponse is
empty but warning logs are disabled; change the logic in AbstractAIService where
jsonResponse is tested so you first check if jsonResponse.trim().isEmpty() and
always return issues in that case, and then, if logger.isWarnEnabled(), call
logger.warn("No valid JSON found in AI response") — keep variable names
jsonResponse, logger and the surrounding method intact so the return happens
regardless of logging level.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java`:
- Around line 75-77: In AbstractRepositoryAnalyzer.java, the banner prints a
literal "%n" because System.out.println("%n" + "=".repeat(80)) doesn't interpret
format specifiers; change the print to use System.out.printf or a real newline
character. Locate the three lines around the banner (the calls to
System.out.println and System.out.printf that reference fullyQualifiedName) and
replace the first println with either System.out.printf("%n%s%n",
"=".repeat(80)) or System.out.println("\n" + "=".repeat(80)) so the newline is
emitted correctly while keeping the existing System.out.printf("Analyzing:
%s%n", fullyQualifiedName) and the trailing "=".repeat(80) line consistent.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java`:
- Around line 771-780: The current join planner overwrites multiple foreign keys
for the same child table because fkByFromTable is a Map<String,
InsteadOfTriggerGenerator.ForeignKey> populated with
fkByFromTable.put(fk.fromTable(), fk); change fkByFromTable to map each
fromTable to a collection (e.g., Map<String,
List<InsteadOfTriggerGenerator.ForeignKey>> or a Multimap), populate lists when
iterating view.foreignKeys(), and in the join-building loop (the for over order
from TopologicalSorter.sort(...)) retrieve the list for the table and apply all
foreign keys (not just one) to build the join predicates so multiple outgoing
FKs are preserved.
- Around line 1192-1206: In buildManyToOne, derive the relation field name from
the FK column rather than the parent table to avoid duplicate member names;
replace the use of toCamelCase(fk.toTable()) with a camel-case name based on the
FK column (e.g., cm.targetColumn() or fk.fromColumn()), stripping common
suffixes like "_id" if present, while keeping parentEntityName =
toPascalCase(fk.toTable()) unchanged and using the new fieldName in the
fields.append formatted block.
- Around line 257-278: The loop in afterAnalysisLoop/sendBatchToLLM incorrectly
treats empty or failed LLM responses as success and lets exceptions escape, so
ensure parseBatchResponse sets lastResponseTruncated=true when provider content
is blank/missing (instead of returning List.of() silently) and that
afterAnalysisLoop treats an empty reports result as a truncated/failed response
(avoid adding to allReports and break only when reports non-empty and
lastResponseTruncated==false). Also wrap the call to sendBatchToLLM/readTree
inside the retry loop with a try/catch that logs the failure, sets
lastResponseTruncated=true (or a similar retry flag), increments attempt, and
continues so transient exceptions are retried; preserve token accounting via
aiService.getLastTokenUsage() (cumulativeTokenUsage) even on retries.
- Around line 692-695: The code is synthesizing FK names
("fk_"+p.tableName()+"_"+sourceTable) which Liquibase cannot use to drop
constraints; update SchemaNormalizationAnalyzer so when discovering foreign keys
(where sourceEntityName.equals(r.targetEntity())) you query the database
metadata (e.g., DatabaseMetaData.getImportedKeys or
information_schema/key_column_usage) to look up the actual constraint name for
p.tableName(), joinCol and sourceTable, then construct ExternalFKReference with
that real constraint name instead of the synthetic one; ensure
buildDropFkChangeset() uses the real name stored in ExternalFKReference so
Liquibase generates valid <dropForeignKeyConstraint> entries.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java`:
- Around line 56-74: Replace the System.err.println warning in
DataMigrationPlanValidator with the project logger: add an SLF4J Logger field
(e.g., private static final Logger logger =
LoggerFactory.getLogger(DataMigrationPlanValidator.class)) and change the
unmapped-column message inside the loop to logger.warn(...) preserving the same
message text (including the profileCol value); ensure you add the necessary
import for org.slf4j.Logger and org.slf4j.LoggerFactory and remove the
System.err.println usage.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java`:
- Around line 227-240: appendUpdateStatements currently uses
deriveWhereSourceCol(table, view, pkMapping) and then compares that source
column to pkMapping.viewColumn(), which only works when the table has a foreign
key directly to view.baseTable(); to fix, change the logic to resolve the FK
chain from the current table back to the view's base table and use the
immediate-parent mapping for the WHERE clause: either update
deriveWhereSourceCol (or add a helper) to return both the source column name on
the child table and the corresponding ancestor view column (or the intermediate
ColumnMapping) along the FK path, then in appendUpdateStatements (and the
similar blocks at lines ~248-260 and ~308-315) use that ancestor/intermediate
viewColumn (e.g., the mapping that points to the child’s parent) instead of
pkMapping.viewColumn() so the WHERE uses NEW.<immediate_parent_column> for
multi-hop relationships; refer to appendUpdateStatements, deriveWhereSourceCol
and pkMapping/viewColumn to locate the code to change.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java`:
- Around line 347-360: The code in LiquibaseGenerator looping over sqlByDialect
and rollbackByDialect must first filter those maps to only include keys whose
DatabaseDialect is in the configured supportedDialects() (or the generator's
supportedDialects()), so change the iteration over sqlByDialect and
rollbackByDialect to iterate over a filtered view (e.g., stream/filter or build
a new map) that contains only dialects present in supportedDialects(); if the
filtered sql map is empty (no entries remain) throw or fail fast with a clear
error rather than emitting an empty changeSet; apply the same filtering and
empty-check for the rollbackByDialect block and the other similar helper
referenced by InsteadOfTriggerGenerator so supported_dialects actually limits
emitted SQL.
- Around line 379-418: The SQL strings are injected into XML unescaped causing
invalid XML for characters like <, >, &, or ]]>; update createRawSqlChangeset,
createViewChangeset, createDialectSqlChangesetWithRollback, and
createDialectSqlChangeset to wrap any SQL content (e.g., the sql and selectSql
parameters) with a CDATA helper before appending into the StringBuilder (for
example call wrapInCData(sql) or wrapInCData(selectSql)); if no helper exists
add a small utility method (e.g., wrapInCData(String s) that returns "<![CDATA["
+ s.replace("]]>", "]]]]><![CDATA[>") + "]]>") and use that helper wherever SQL
is embedded into XML.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java`:
- Around line 177-183: The current DDL generator unconditionally appends
autoIncrement="true" for ID fields; update NormalizedTableDDLGenerator to only
add the autoIncrement attribute when the FieldProfile explicitly indicates an
auto-generated numeric ID (e.g., a new isAutoIncrement/isAutoGenerated flag) and
the field type is numeric (add an isNumericType check for FieldProfile types
like bigint/int/smallint); modify the block that builds constraints (where isId
is used) to append autoIncrement="true" only when isId &&
fieldProfile.isAutoIncrement() && isNumericType(fieldProfile.getType()), and add
the new flag or type-check helper to FieldProfile so UUIDs, composite keys and
externally-assigned IDs are not marked auto-incremented.
- Around line 165-166: The changeset ID currently uses
System.currentTimeMillis() making IDs non-deterministic; in
NormalizedTableDDLGenerator replace the timestamp-based ID construction (the
sb.append call that builds "create_table_"+table+"_"+System.currentTimeMillis())
with a deterministic generation strategy (e.g., use the table name plus a stable
version/sequence parameter, or a hash of the table-DDL/schema) so repeated runs
produce the same ID; update the method that builds the changeSet string to
accept/derive a stable version/sequence parameter or compute a stable hash and
use that instead of System.currentTimeMillis().
---
Outside diff comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 1208-1215: The current logic skips calling
QueryOptimizationChecker.generateLiquibaseChangesFile() when analyze() returns
0, which loses suggestions recovered by restoreCheckpointState(); remove the
repositoriesAnalyzed check and always invoke
checker.generateLiquibaseChangesFile() after checker.analyze() so recovered
pending suggestions are written (generateLiquibaseChangesFile() already no-ops
when there is nothing to write). Keep the existing informational print only if
you still want a special message, but do not gate the call to
generateLiquibaseChangesFile() on repositoriesAnalyzed; use the
QueryOptimizationChecker methods (analyze(), generateLiquibaseChangesFile(),
restoreCheckpointState()) as the touch points.
- Around line 1095-1107: The drop heuristic in getDropCandidates() is too
aggressive: only consider an index for dropping if it is a single-column
non-unique, non-FK-supporting index whose first column has LOW cardinality.
Update the loop that inspects Indexes.IndexInfo (idx) to skip any index where
idx.columns() has size > 1, or where idx indicates uniqueness or FK support
(e.g., idx.isUnique(), idx.isPrimary(), idx.supportsForeignKey() or equivalent
metadata on IndexInfo), and only then run
CardinalityAnalyzer.analyzeColumnCardinality(table, first) and add idx.name() to
dropCandidates; keep existing checks for idx.type(), non-empty columns and
non-empty name. Ensure you reference getDropCandidates, CardinalityAnalyzer, and
Indexes.IndexInfo when making the changes.
- Around line 297-299: In QueryOptimizationChecker, use a single accessor style
for WhereCondition: replace the mixed ternary using condition.tableName() and
condition.getTableName() with a consistent call (either always record-style
tableName() or always bean-style getTableName()) in the for loop and the other
occurrence; e.g., change the expression to condition.tableName() == null ?
rawQuery.getPrimaryTable() : condition.tableName() (or the equivalent using
getTableName()) so both reads use the same accessor consistently.
---
Nitpick comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationGenerator.java`:
- Around line 67-73: The grouping uses ColumnMapping.sourceTable(), but that
field actually represents the destination/normalized table rather than the
original source; add a brief clarifying comment either on the ColumnMapping
record in InsteadOfTriggerGenerator and/or directly above
groupByTable(ViewDescriptor view) explaining that sourceTable() is the
target/destination table receiving normalized data to avoid confusion when
reading groupByTable and related logic.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/TopologicalSorter.java`:
- Around line 63-72: The fallback List.of() in the loop is unnecessary since the
children map is fully pre-initialized; replace children.getOrDefault(current,
List.of()) with children.get(current) in the for-loop inside the
TopologicalSorter routine (the block using queue.poll(), result.add(current),
for (String child : ...)), removing the defensive fallback and relying on the
initialized children entries and existing inDegree handling.
- Around line 44-51: In TopologicalSorter, the loop that processes ForeignKey
edges uses children.computeIfAbsent(parent, ...) despite earlier initializing
every table in the children map; replace the computeIfAbsent call with
children.get(parent).add(child) (after the existing guard that checks inDegree
contains parent/child) so you use the pre-initialized list directly and keep the
inDegree.merge(child, 1, Integer::sum) logic unchanged.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.java`:
- Around line 38-41: Add an inline comment to the Arguments entry in
AbstractAIServiceTest that documents this edge case: note that the input "Some
text [not json] then {\"id\": 1}" leads extractJson to return "[not json] then
{\"id\": 1}" which is not valid JSON and will cause downstream parsing to fail
unless callers handle parse errors; place the comment next to the
Arguments.of(...) tuple so future readers of the test and maintainers of
extractJson (or callers of extractJson) understand this known limitation.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.java`:
- Around line 169-196: Add an assertion that verifies the warning for the
unmapped "email" column is actually emitted: capture stderr or the logger output
around the call to DataMigrationPlanValidator.validate(plan, profile) in
testMissingProfileColumnGeneratesWarning and assert the captured output contains
"Column 'email' from source profile is not mapped" (or the exact warning text
produced by DataMigrationPlanValidator). Use the test harness's stream capture
(e.g., System.err capture) or your logging test appender for SLF4J to wrap the
validate(...) invocation and then assert the warning is present while still
keeping assertDoesNotThrow for validation success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2aec9dd6-30f2-4d09-a495-329cdeafbf7c
📒 Files selected for processing (28)
.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/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/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.java
|
|
||
| **Goal:** Validate each `DataMigrationPlan` before generating artifacts; skip or fail fast on invalid plans. | ||
|
|
||
| **New file:** `antikythera-examples/src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java` |
Contributor
There was a problem hiding this comment.
Use repo-relative paths in the plan.
These “New file” entries include the repository name prefix (antikythera-examples/src/...). From the repo root that path does not exist, so an agent following this literally can create files in the wrong place. src/main/java/... is the correct format here.
Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/schema-normalization-implementation-plan.md` at line 34, The plan uses
repository-prefixed paths in the "New file" entries (e.g.,
antikythera-examples/src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java)
which will produce incorrect file locations; update each "New file" entry
(including the one referencing DataMigrationPlanValidator.java and the other at
57-57) to use repo-relative paths starting at src/ (e.g., src/main/java/...),
ensuring all future entries follow that format so agents create files in the
correct repository location.
Comment on lines
+548
to
551
| 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.
Logic issue: Condition structure may cause silent return on empty response.
The condition jsonResponse.trim().isEmpty() && logger.isWarnEnabled() only enters the block when both conditions are true. If the response is empty but warn logging is disabled, the method will continue past this check with an empty string, potentially causing issues downstream.
🐛 Proposed fix
- if (jsonResponse.trim().isEmpty() && logger.isWarnEnabled()) {
+ if (jsonResponse.trim().isEmpty()) {
+ if (logger.isWarnEnabled()) {
+ logger.warn("No valid JSON found in AI response");
+ }
logger.warn("No valid JSON found in AI response");
return issues;
}Or more concisely:
- 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;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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; | |
| } |
🤖 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 - 551, The current condition combines the empty-response check
with logger.isWarnEnabled(), causing a silent fall-through when jsonResponse is
empty but warning logs are disabled; change the logic in AbstractAIService where
jsonResponse is tested so you first check if jsonResponse.trim().isEmpty() and
always return issues in that case, and then, if logger.isWarnEnabled(), call
logger.warn("No valid JSON found in AI response") — keep variable names
jsonResponse, logger and the surrounding method intact so the return happens
regardless of logging level.
Comment on lines
+75
to
+77
| System.out.println("%n" + "=".repeat(80)); | ||
| System.out.printf("Analyzing: %s%n", fullyQualifiedName); | ||
| System.out.println("=".repeat(80)); |
Contributor
There was a problem hiding this comment.
println prints %n literally here.
System.out.println() does not interpret %n, so the banner starts with the two characters %n. Use printf or prepend "\n" instead.
💡 Suggested fix
- System.out.println("%n" + "=".repeat(80));
+ System.out.println("\n" + "=".repeat(80));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| System.out.println("%n" + "=".repeat(80)); | |
| System.out.printf("Analyzing: %s%n", fullyQualifiedName); | |
| System.out.println("=".repeat(80)); | |
| System.out.println("\n" + "=".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, In AbstractRepositoryAnalyzer.java, the banner prints a
literal "%n" because System.out.println("%n" + "=".repeat(80)) doesn't interpret
format specifiers; change the print to use System.out.printf or a real newline
character. Locate the three lines around the banner (the calls to
System.out.println and System.out.printf that reference fullyQualifiedName) and
replace the first println with either System.out.printf("%n%s%n",
"=".repeat(80)) or System.out.println("\n" + "=".repeat(80)) so the newline is
emitted correctly while keeping the existing System.out.printf("Analyzing:
%s%n", fullyQualifiedName) and the trailing "=".repeat(80) line consistent.
Comment on lines
+379
to
+418
| 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"); | ||
|
|
||
| if (config.includeRollback()) { | ||
| sb.append(" <rollback><!-- manual rollback required --></rollback>\n"); | ||
| } | ||
|
|
||
| sb.append("</changeSet>"); | ||
| return sb.toString(); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a Liquibase {@code <createView>} changeset using the built-in Liquibase tag. | ||
| * Supported by Liquibase on both PostgreSQL and Oracle without dialect-specific SQL. | ||
| * Rollback uses {@code <dropView>}. | ||
| * | ||
| * @param viewName the name of the view to create | ||
| * @param selectSql the SELECT statement that defines the view body | ||
| * @return XML changeset string | ||
| */ | ||
| 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>\n"); | ||
|
|
||
| if (config.includeRollback()) { | ||
| sb.append(" <rollback><dropView viewName=\"").append(viewName).append("\"/></rollback>\n"); | ||
| } | ||
|
|
||
| sb.append("</changeSet>"); | ||
| return sb.toString(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "LiquibaseGenerator.java" -type fRepository: e4c5/antikythera-examples
Length of output: 156
🏁 Script executed:
wc -l ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 160
🏁 Script executed:
sed -n '379,418p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 1819
🏁 Script executed:
sed -n '1,100p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 3755
🏁 Script executed:
grep -n "logger\|Logger\|log\|Log" ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | head -20Repository: e4c5/antikythera-examples
Length of output: 931
🏁 Script executed:
grep -n "private\|protected\|public" ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | grep -i "log\|logger"Repository: e4c5/antikythera-examples
Length of output: 51
🏁 Script executed:
sed -n '30,150p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 4906
🏁 Script executed:
sed -n '200,300p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 4065
🏁 Script executed:
sed -n '300,380p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 3814
🏁 Script executed:
sed -n '420,500p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 3930
🏁 Script executed:
rg -n "cdata|CDATA|XMLEncoder|escapeXml|XmlUtils" ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 51
🏁 Script executed:
grep -n "append(.*Sql)\|append(.*sql)" ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 201
🏁 Script executed:
sed -n '330,370p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 2195
🏁 Script executed:
sed -n '440,450p' ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.javaRepository: e4c5/antikythera-examples
Length of output: 448
🏁 Script executed:
cat -n ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | sed -n '1,50p'Repository: e4c5/antikythera-examples
Length of output: 2038
🏁 Script executed:
rg -n "private|protected|public" ./src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java | head -30Repository: e4c5/antikythera-examples
Length of output: 2353
Escape SQL strings with CDATA before embedding in XML elements.
The methods createRawSqlChangeset, createViewChangeset, createDialectSqlChangesetWithRollback, and createDialectSqlChangeset all append SQL parameters directly into XML without escaping. SQL statements containing <, >, &, or ]]> characters will generate malformed XML and fail before Liquibase processes them. Use a CDATA wrapper helper in all affected locations:
+ private String cdata(String text) {
+ return "<![CDATA[" + text.replace("]]>", "]]]]><![CDATA[>") + "]]>";
+ }Apply this helper to all SQL parameter insertions across these four methods.
🤖 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 379 - 418, The SQL strings are injected into XML unescaped causing
invalid XML for characters like <, >, &, or ]]>; update createRawSqlChangeset,
createViewChangeset, createDialectSqlChangesetWithRollback, and
createDialectSqlChangeset to wrap any SQL content (e.g., the sql and selectSql
parameters) with a CDATA helper before appending into the StringBuilder (for
example call wrapInCData(sql) or wrapInCData(selectSql)); if no helper exists
add a small utility method (e.g., wrapInCData(String s) that returns "<![CDATA["
+ s.replace("]]>", "]]]]><![CDATA[>") + "]]>") and use that helper wherever SQL
is embedded into XML.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java (1)
210-221:⚠️ Potential issue | 🟠 MajorDon't collapse malformed Gemini payloads into “no findings.”
If
candidatesis missing/empty or the first candidate has no text parts, this returns[]and the analyzer treats a provider failure like a clean batch. ThrowAIResponseExceptionhere so the retry/error path can surface the problem instead of silently dropping it.🤖 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/GeminiAIService.java` around lines 210 - 221, In GeminiAIService, don’t return an empty list when the Gemini response is malformed; instead detect the failure paths around candidates, firstCandidate, and parts (and missing text in parts.get(0).path("text")) and throw an AIResponseException with a clear message (include identifying info from the response/payload) so the retry/error handling can run; keep parseRecommendations only for valid textResponse values and replace the final fallback return new ArrayList<>() with raising AIResponseException when the expected fields are absent or empty.
♻️ Duplicate comments (4)
src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java (1)
548-551:⚠️ Potential issue | 🟡 MinorReturn early on empty JSON regardless of logging configuration.
logger.isWarnEnabled()is still part of the guard, so an emptyjsonResponsefalls through toreadTree(...)whenever WARN is disabled. The return should be unconditional, with logging gated inside the block.🤖 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 - 551, The current guard mixes the empty-response check with logger.isWarnEnabled(), so an empty jsonResponse can still reach readTree(...) when WARN is disabled; in the method in AbstractAIService, change the flow to unconditionally return issues when jsonResponse.trim().isEmpty(), and move the logger.isWarnEnabled() check inside that block so the logger.warn("No valid JSON found in AI response") is only conditional on isWarnEnabled(); ensure readTree(...) is never called for an empty jsonResponse.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java (1)
358-375:⚠️ Potential issue | 🟠 MajorWrap embedded SQL in CDATA before emitting XML.
These builders append raw SQL directly into XML elements. Any view/trigger/body containing
<,>,&, or]]>will produce malformed changelogs before Liquibase can parse them.Suggested fix
+ private String cdata(String sql) { + return "<![CDATA[" + sql.replace("]]>", "]]]]><![CDATA[>") + "]]>"; + } ... - sb.append(" <sql>").append(sql).append("</sql>\n"); + sb.append(" <sql>").append(cdata(sql)).append("</sql>\n");Apply the same helper everywhere SQL text is embedded, including the dialect-specific and
<createView>bodies.Also applies to: 402-404, 426-429, 462-465
🤖 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 358 - 375, The SQL strings are being appended raw into XML (for example in the filteredSql loop and the rollbackByDialect handling inside the block guarded by config.includeRollback()), which can produce malformed XML for SQL containing characters like <, >, &, or the sequence ]]>; update the emission to wrap all embedded SQL bodies in a CDATA section via a shared helper (e.g., createCData(String sql) used wherever SQL is appended: the filteredSql loop, the filteredRollback loop, the dialect-specific blocks and createView bodies referenced around lines noted in the review), so replace direct sb.append(entry.getValue()) (and similar occurrences) with sb.append(createCData(entry.getValue())) everywhere SQL text is inserted into XML.src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java (1)
240-242:⚠️ Potential issue | 🟠 MajorUse the child FK mapping for
viewCol, not the parent PK mapping.For non-base tables this resolves
viewColvia(parentFk.toTable(), parentFk.toColumn()). That still misses the immediate child mapping the code relies on elsewhere, so grandchildUPDATE/DELETEbreak whenever the parent PK is not projected or uses a different alias than the child FK mapping. Resolve the view column from(table, parentFk.fromColumn())instead.Also applies to: 274-276
🤖 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 240 - 242, The code currently resolves viewCol using the parent PK mapping via findViewColumnForParent(parentFk.toTable(), parentFk.toColumn(), view) after calling findImmediateParentFk(table, view); change that to resolve the view column from the child FK mapping by calling findViewColumnForParent(table, parentFk.fromColumn(), view) (i.e., use parentFk.fromColumn() with the current table) so the immediate child mapping is used; make the same change for the other occurrence noted (the later block around the repeated findImmediateParentFk usage).src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java (1)
715-718:⚠️ Potential issue | 🟠 MajorDon't turn unknown FK metadata into a guessed drop target.
discoverExternalFKs()can calllookupConstraintName()with a null join column, andlookupConstraintName()then falls back tofk_<referencing>_<source>when metadata lookup misses. That converts “we couldn't resolve this FK” into either a metadata-time failure or a drop changeset for a name that was never actually verified.Also applies to: 734-766
🤖 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 715 - 718, discoverExternalFKs() is currently converting unknown FK metadata into a guessed constraint name by calling lookupConstraintName() even when joinColumn() is null; stop treating unresolved FKs as valid drop targets by skipping creation of ExternalFKReference when r.joinColumn() is null or when lookupConstraintName() returns the unverified default pattern (e.g., the fallback "fk_<referencing>_<source>"). Update the block that constructs new ExternalFKReference(p.tableName(), joinCol, constraintName) to first check r.joinColumn() and ensure lookupConstraintName() produced a verified name (not the fallback guess), and only then add to refs; apply the same guard to the other similar blocks in discoverExternalFKs() (lines handling the mirrored cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 634-643: The logic in getLastFenceStart can mark a closing fence
as an opening one because it only inspects the tail; update getLastFenceStart to
also verify the fence is actually an opening fence by checking the character
before the found index (e.g., treat it as opening only if idx == 0 or
s.charAt(idx - 1) == '\n'), and keep the existing tail checks (json, "\n[",
"\n{", etc.); modify the condition inside getLastFenceStart (the block that
currently sets lastFenceStart = idx) to require this
"preceded-by-start-or-newline" test so closing fences followed by blank lines or
trailer text are not treated as openings (affects extractJson behavior that
depends on getLastFenceStart).
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 89-93: restoreCheckpointState() can refill
suggestedNewIndexes/suggestedMultiColumnIndexes from the checkpoint before any
repository is analyzed, but main() only writes the Liquibase file when analyze()
returns >0, causing resumed runs to never materialize restored suggestions;
update main() in QueryOptimizationChecker to also write the output when either
suggestedNewIndexes or suggestedMultiColumnIndexes is non-empty (or when a
"restored" flag set by restoreCheckpointState() is true) even if analyze()
returns zero, so that restored suggestions are persisted after
restoreCheckpointState() runs.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java`:
- Around line 1219-1227: The current isId branch always emits
`@GeneratedValue`(strategy = GenerationType.IDENTITY) which forces every copied ID
to be DB-generated; change the logic in the isId branch (where idEmitted is set
and fields.append(...) is called for cm.targetColumn(), javaType and fieldName)
to only include the `@GeneratedValue` annotation when the column is actually
auto-generated by using isAutoGeneratedColumn(...) (pass the relevant
column/profile info as used elsewhere), and otherwise emit only the `@Id` and
`@Column` lines so assigned/UUID/natural keys are preserved.
- Around line 792-795: Ensure the plan always moves the existing table out of
the way before adding the compatibility view/trigger changesets: when handling
renameOldTableTo (and the similar block around the other occurrence), only add
the view/trigger changesets for the original sourceTable after you have a
non-blank renameOldTableTo and have added the buildRenameTableChangeset result
to changesets; if renameOldTableTo is blank, skip or defer creating the
replacement view/trigger changesets so they don’t collide with the existing
table. Identify and modify the code paths that build the replacement
view/trigger changesets that follow renameOldTableTo handling to depend on the
successful addition of buildRenameTableChangeset (use the variables
renameOldTableTo, backupName, renameCs, and changesets to gate creation).
- Around line 1382-1388: The summary currently uses allReports (which omits
clean entities) to compute totals; change totalEntities to use
allProfiles.size(), and compute entitiesWithIssues by building the set of entity
IDs from allReports where r.issues() is non-empty and then counting how many
profiles in allProfiles have their identifier present in that set (use the
profile identifier accessor, e.g., profile.id(), and the report identifier
accessor, e.g., report.entityId()); leave the Total violations
(totalRecommendations) as-is. This ensures totalEntities reflects all analyzed
profiles and entitiesWithIssues counts profiles that have corresponding reports
with issues.
- Around line 272-277: The loop in SchemaNormalizationAnalyzer is treating an
empty but well-formed batch (reports == empty list) as a failure and keeps
retrying; update the exit condition to accept any non-truncated response (i.e.,
if (!lastResponseTruncated) { add the reports to allReports and break }) so that
an empty array is treated as "no violations" and stops retries; ensure you still
handle null/malformed responses separately before adding to allReports.
- Around line 845-853: The code currently emits a JOIN per foreign-key using
fk.toTable(), causing wrong SQL and referencing the child table before it's in
FROM; instead, for each entry from order (variable table) fetch fks =
fksByFromTable.get(table), append a single " JOIN " + table, then build one ON
clause that joins that child table to its parents by concatenating each fk
predicate as table + "." + fk.fromColumn() + " = " + fk.toTable() + "." +
fk.toColumn(), joining multiple predicates with " AND ". Update the loop that
uses order, fksByFromTable and InsteadOfTriggerGenerator.ForeignKey to create
one JOIN per table (variable table) with a combined ON clause rather than
joining fk.toTable() for each fk.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidator.java`:
- Around line 74-77: In DataMigrationPlanValidator (validate method) change the
non-fatal logging for unmapped source profile columns from logger.error to
logger.warn: when iterating profileColumns and checking mappedViewColumns (the
block referencing profileCol), replace the logger.error call with logger.warn
(keep the existing message text or slightly rephrase for warning context) so
missing source-column mappings are logged as warnings rather than errors.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java`:
- Around line 727-736: The code silently falls back to the default dialects when
config-provided supported_dialects (dialectNames) contains only unrecognized
values; change the logic in the block that uses DatabaseDialect.fromString to
reject the configuration instead of defaulting: after mapping and collecting
parsedDialects from dialectNames, if dialectNames was non-null/non-empty but
parsedDialects.isEmpty() then throw a clear configuration exception (or return a
validation error) indicating which input values failed to parse (use the
original dialectNames and DatabaseDialect.fromString results to report invalid
entries); only assign dialects = parsedDialects when parsedDialects is non-empty
as currently done.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGenerator.java`:
- Around line 90-115: The code only populates column metadata from
sourceProfile.fields(), so foreign-key columns introduced by relationships get
default VARCHAR(255) and no constraints; update the logic that builds
columnJavaType/columnNullable/columnIsId/columnIsAutoGenerated to also iterate
sourceProfile.relationships() (and/or resolve the referenced target key via view
or the target table's fields) to copy the referenced column's
type/nullability/id info into the maps for the FK column name, and collect FK
constraint descriptors; then modify the per-table generation loop (the code that
calls buildRawSql and buildLiquibaseChangeset) to pass or compute those FK
descriptors so buildRawSql and buildLiquibaseChangeset emit proper FOREIGN KEY /
addForeignKeyConstraint clauses; reference symbols:
sourceProfile.relationships(), view.foreignKeys(), byTable,
TopologicalSorter.sort(...), buildRawSql(...), buildLiquibaseChangeset(...).
---
Outside diff comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java`:
- Around line 210-221: In GeminiAIService, don’t return an empty list when the
Gemini response is malformed; instead detect the failure paths around
candidates, firstCandidate, and parts (and missing text in
parts.get(0).path("text")) and throw an AIResponseException with a clear message
(include identifying info from the response/payload) so the retry/error handling
can run; keep parseRecommendations only for valid textResponse values and
replace the final fallback return new ArrayList<>() with raising
AIResponseException when the expected fields are absent or empty.
---
Duplicate comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 548-551: The current guard mixes the empty-response check with
logger.isWarnEnabled(), so an empty jsonResponse can still reach readTree(...)
when WARN is disabled; in the method in AbstractAIService, change the flow to
unconditionally return issues when jsonResponse.trim().isEmpty(), and move the
logger.isWarnEnabled() check inside that block so the logger.warn("No valid JSON
found in AI response") is only conditional on isWarnEnabled(); ensure
readTree(...) is never called for an empty jsonResponse.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.java`:
- Around line 715-718: discoverExternalFKs() is currently converting unknown FK
metadata into a guessed constraint name by calling lookupConstraintName() even
when joinColumn() is null; stop treating unresolved FKs as valid drop targets by
skipping creation of ExternalFKReference when r.joinColumn() is null or when
lookupConstraintName() returns the unverified default pattern (e.g., the
fallback "fk_<referencing>_<source>"). Update the block that constructs new
ExternalFKReference(p.tableName(), joinCol, constraintName) to first check
r.joinColumn() and ensure lookupConstraintName() produced a verified name (not
the fallback guess), and only then add to refs; apply the same guard to the
other similar blocks in discoverExternalFKs() (lines handling the mirrored
cases).
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/InsteadOfTriggerGenerator.java`:
- Around line 240-242: The code currently resolves viewCol using the parent PK
mapping via findViewColumnForParent(parentFk.toTable(), parentFk.toColumn(),
view) after calling findImmediateParentFk(table, view); change that to resolve
the view column from the child FK mapping by calling
findViewColumnForParent(table, parentFk.fromColumn(), view) (i.e., use
parentFk.fromColumn() with the current table) so the immediate child mapping is
used; make the same change for the other occurrence noted (the later block
around the repeated findImmediateParentFk usage).
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGenerator.java`:
- Around line 358-375: The SQL strings are being appended raw into XML (for
example in the filteredSql loop and the rollbackByDialect handling inside the
block guarded by config.includeRollback()), which can produce malformed XML for
SQL containing characters like <, >, &, or the sequence ]]>; update the emission
to wrap all embedded SQL bodies in a CDATA section via a shared helper (e.g.,
createCData(String sql) used wherever SQL is appended: the filteredSql loop, the
filteredRollback loop, the dialect-specific blocks and createView bodies
referenced around lines noted in the review), so replace direct
sb.append(entry.getValue()) (and similar occurrences) with
sb.append(createCData(entry.getValue())) everywhere SQL text is inserted into
XML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c661771-620c-4a38-b3cf-4490bc0d304a
📒 Files selected for processing (12)
src/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/GeminiAIService.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.javasrc/main/java/sa/com/cloudsolutions/antikythera/examples/SchemaNormalizationAnalyzer.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/test/java/sa/com/cloudsolutions/antikythera/examples/util/DataMigrationPlanValidatorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/LiquibaseGeneratorTest.javasrc/test/java/sa/com/cloudsolutions/antikythera/examples/util/NormalizedTableDDLGeneratorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractRepositoryAnalyzer.java
Comment on lines
+634
to
+643
| private static int getLastFenceStart(String s, int lastFenceStart) { | ||
| int pos = 0; | ||
| while (pos < s.length()) { | ||
| int idx = s.indexOf("```", pos); | ||
| if (idx == -1) break; | ||
| String tail = s.substring(Math.min(idx + 3, s.length())); | ||
| // Treat as an opening fence if followed by "json", a newline with [/{, or whitespace | ||
| if (tail.startsWith("json") || tail.startsWith("\n[") || tail.startsWith("\n{") | ||
| || tail.startsWith("\n ") || tail.startsWith("\n\n")) { | ||
| lastFenceStart = idx; |
Contributor
There was a problem hiding this comment.
Closing fences can still be mistaken for the last opening fence.
The "\n " / "\n\n" branches also match a normal closing ``` followed by blank lines or trailer text. In that case lastFenceStart moves to the closing fence, and `extractJson()` returns the postscript instead of the JSON block.
Suggested fix
- if (tail.startsWith("json") || tail.startsWith("\n[") || tail.startsWith("\n{")
- || tail.startsWith("\n ") || tail.startsWith("\n\n")) {
- lastFenceStart = idx;
- }
+ if (tail.startsWith("json")) {
+ lastFenceStart = idx;
+ } else if (tail.startsWith("\n")) {
+ int i = idx + 3;
+ while (i < s.length() && Character.isWhitespace(s.charAt(i))) {
+ i++;
+ }
+ if (i < s.length() && (s.charAt(i) == '[' || s.charAt(i) == '{')) {
+ lastFenceStart = idx;
+ }
+ }🤖 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 634 - 643, The logic in getLastFenceStart can mark a closing fence
as an opening one because it only inspects the tail; update getLastFenceStart to
also verify the fence is actually an opening fence by checking the character
before the found index (e.g., treat it as opening only if idx == 0 or
s.charAt(idx - 1) == '\n'), and keep the existing tail checks (json, "\n[",
"\n{", etc.); modify the condition inside getLastFenceStart (the block that
currently sets lastFenceStart = idx) to require this
"preceded-by-start-or-newline" test so closing fences followed by blank lines or
trailer text are not treated as openings (affects extractJson behavior that
depends on getLastFenceStart).
Comment on lines
+89
to
+93
| @Override | ||
| protected void restoreCheckpointState() { | ||
| suggestedNewIndexes.addAll(checkpointManager.getSuggestedNewIndexes()); | ||
| suggestedMultiColumnIndexes.addAll(checkpointManager.getSuggestedMultiColumnIndexes()); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't skip file generation when only checkpointed suggestions were restored.
restoreCheckpointState() can repopulate both suggestion sets before any repository is analyzed, but main() only writes the Liquibase file when analyze() returns a positive count. A resumed run can therefore recover the suggestions and still exit without ever materializing them.
💡 Suggested fix
- if (repositoriesAnalyzed > 0) {
+ if (repositoriesAnalyzed > 0
+ || !checker.getSuggestedNewIndexes().isEmpty()
+ || !checker.getSuggestedMultiColumnIndexes().isEmpty()) {
checker.generateLiquibaseChangesFile();
} else {
System.out.println("\n⏭️ Skipping Liquibase generation - no new repositories were analyzed");
}Also applies to: 1205-1213
🤖 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 89 - 93, restoreCheckpointState() can refill
suggestedNewIndexes/suggestedMultiColumnIndexes from the checkpoint before any
repository is analyzed, but main() only writes the Liquibase file when analyze()
returns >0, causing resumed runs to never materialize restored suggestions;
update main() in QueryOptimizationChecker to also write the output when either
suggestedNewIndexes or suggestedMultiColumnIndexes is non-empty (or when a
"restored" flag set by restoreCheckpointState() is true) even if analyze()
returns zero, so that restored suggestions are persisted after
restoreCheckpointState() runs.
Comment on lines
+272
to
+277
| if (!lastResponseTruncated && !reports.isEmpty()) { | ||
| allReports.addAll(reports); | ||
| break; | ||
| } | ||
|
|
||
| logger.warn("Response empty or malformed on attempt {}; retrying full request", attempt + 1); |
Contributor
There was a problem hiding this comment.
Accept [] as a successful batch response.
A well-formed empty array means “no violations found”, but this loop only exits on non-empty reports. Clean batches will burn through every retry and then look indistinguishable from a failed parse.
💡 Suggested fix
- if (!lastResponseTruncated && !reports.isEmpty()) {
+ if (!lastResponseTruncated) {
allReports.addAll(reports);
break;
}🤖 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 272 - 277, The loop in SchemaNormalizationAnalyzer is treating an
empty but well-formed batch (reports == empty list) as a failure and keeps
retrying; update the exit condition to accept any non-truncated response (i.e.,
if (!lastResponseTruncated) { add the reports to allReports and break }) so that
an empty array is treated as "no violations" and stops retries; ensure you still
handle null/malformed responses separately before adding to allReports.
Comment on lines
+792
to
+795
| if (renameOldTableTo != null && !renameOldTableTo.isBlank()) { | ||
| String backupName = renameOldTableTo.replace("{sourceTable}", sourceTable); | ||
| String renameCs = buildRenameTableChangeset(sourceTable, backupName, generator); | ||
| if (renameCs != null) changesets.add(renameCs); |
Contributor
There was a problem hiding this comment.
Require the old table to move out of the way before creating the compatibility layer.
The rename step is optional, but the plan builder always creates the replacement view and triggers under the original table name immediately afterward. If rename_old_table_to is blank, the first view changeset collides with the still-existing table.
Also applies to: 1016-1036
🤖 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 792 - 795, Ensure the plan always moves the existing table out of
the way before adding the compatibility view/trigger changesets: when handling
renameOldTableTo (and the similar block around the other occurrence), only add
the view/trigger changesets for the original sourceTable after you have a
non-blank renameOldTableTo and have added the buildRenameTableChangeset result
to changesets; if renameOldTableTo is blank, skip or defer creating the
replacement view/trigger changesets so they don’t collide with the existing
table. Identify and modify the code paths that build the replacement
view/trigger changesets that follow renameOldTableTo handling to depend on the
successful addition of buildRenameTableChangeset (use the variables
renameOldTableTo, backupName, renameCs, and changesets to gate creation).
Comment on lines
+845
to
+853
| 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()); | ||
| } |
Contributor
There was a problem hiding this comment.
Join the child table here, not the parent table.
For address.customer_id -> customer.id this emits FROM customer JOIN customer ON address.customer_id = customer.id, which references address before it has been introduced and produces invalid SQL. Join table once and combine that table's FK predicates in the ON clause.
💡 Suggested fix
- 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());
- }
- }
+ if (fks != null && !fks.isEmpty()) {
+ String onClause = fks.stream()
+ .map(fk -> fk.fromTable() + "." + fk.fromColumn()
+ + " = " + fk.toTable() + "." + fk.toColumn())
+ .collect(Collectors.joining(" AND "));
+ sb.append(" JOIN ").append(table)
+ .append(" ON ").append(onClause);
+ }🤖 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 845 - 853, The code currently emits a JOIN per foreign-key using
fk.toTable(), causing wrong SQL and referencing the child table before it's in
FROM; instead, for each entry from order (variable table) fetch fks =
fksByFromTable.get(table), append a single " JOIN " + table, then build one ON
clause that joins that child table to its parents by concatenating each fk
predicate as table + "." + fk.fromColumn() + " = " + fk.toTable() + "." +
fk.toColumn(), joining multiple predicates with " AND ". Update the loop that
uses order, fksByFromTable and InsteadOfTriggerGenerator.ForeignKey to create
one JOIN per table (variable table) with a combined ON clause rather than
joining fk.toTable() for each fk.
Comment on lines
+1219
to
+1227
| if (isId) { | ||
| idEmitted = true; | ||
| fields.append(""" | ||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| @Column(name = "%s") | ||
| private %s %s; | ||
|
|
||
| """.formatted(cm.targetColumn(), javaType, fieldName)); |
Contributor
There was a problem hiding this comment.
Don't force every copied @Id to be identity-generated.
The profile already captures whether the source ID was auto-generated, but this branch always emits @GeneratedValue(strategy = GenerationType.IDENTITY). That rewrites assigned/UUID/natural keys into database-generated ones in the generated entity. Only add @GeneratedValue when isAutoGeneratedColumn(...) is true.
🤖 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 1219 - 1227, The current isId branch always emits
`@GeneratedValue`(strategy = GenerationType.IDENTITY) which forces every copied ID
to be DB-generated; change the logic in the isId branch (where idEmitted is set
and fields.append(...) is called for cm.targetColumn(), javaType and fieldName)
to only include the `@GeneratedValue` annotation when the column is actually
auto-generated by using isAutoGeneratedColumn(...) (pass the relevant
column/profile info as used elsewhere), and otherwise emit only the `@Id` and
`@Column` lines so assigned/UUID/natural keys are preserved.
Comment on lines
+1382
to
+1388
| int totalEntities = allReports.size(); | ||
| long entitiesWithIssues = allReports.stream() | ||
| .filter(r -> !r.issues().isEmpty()).count(); | ||
|
|
||
| System.out.printf("Entities analyzed : %d%n", totalEntities); | ||
| System.out.printf("Entities w/ issues : %d%n", entitiesWithIssues); | ||
| System.out.printf("Total violations : %d%n", totalRecommendations); |
Contributor
There was a problem hiding this comment.
Count analyzed entities from allProfiles, not allReports.
allReports only contains entities returned by the model, and the prompt explicitly omits clean entities. The summary therefore undercounts every successful run and prints 0 analyzed when everything is clean.
🤖 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 1382 - 1388, The summary currently uses allReports (which omits
clean entities) to compute totals; change totalEntities to use
allProfiles.size(), and compute entitiesWithIssues by building the set of entity
IDs from allReports where r.issues() is non-empty and then counting how many
profiles in allProfiles have their identifier present in that set (use the
profile identifier accessor, e.g., profile.id(), and the report identifier
accessor, e.g., report.entityId()); leave the Total violations
(totalRecommendations) as-is. This ensures totalEntities reflects all analyzed
profiles and entitiesWithIssues counts profiles that have corresponding reports
with issues.
Comment on lines
+74
to
+77
| for (String profileCol : profileColumns) { | ||
| if (!mappedViewColumns.contains(profileCol)) { | ||
| logger.error("Column '{}' from source profile is not mapped in columnMappings", profileCol); | ||
| } |
Contributor
There was a problem hiding this comment.
Use warn for this non-fatal coverage path.
The class doc explicitly describes missing source-column mappings as warning-only, and validate(...) still succeeds. Logging them at error level makes valid plans look failed and can trigger error-based monitoring.
🤖 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 (validate method) change the
non-fatal logging for unmapped source profile columns from logger.error to
logger.warn: when iterating profileColumns and checking mappedViewColumns (the
block referencing profileCol), replace the logger.error call with logger.warn
(keep the existing message text or slightly rephrase for warning context) so
missing source-column mappings are logged as warnings rather than errors.
Comment on lines
+727
to
+736
| if (dialectNames != null && !dialectNames.isEmpty()) { | ||
| Set<DatabaseDialect> parsedDialects = dialectNames.stream() | ||
| .map(DatabaseDialect::fromString) | ||
| .filter(Optional::isPresent) | ||
| .map(Optional::get) | ||
| .collect(Collectors.toSet()); | ||
| if (!parsedDialects.isEmpty()) { | ||
| dialects = parsedDialects; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject invalid supported_dialects values instead of defaulting to PostgreSQL/Oracle.
When the config explicitly provides supported_dialects but none of the values parse, this silently falls back to the default dialect set. A typo in YAML then generates the wrong migration artifacts instead of failing fast.
Suggested fix
if (dialectNames != null && !dialectNames.isEmpty()) {
Set<DatabaseDialect> parsedDialects = dialectNames.stream()
.map(DatabaseDialect::fromString)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toSet());
- if (!parsedDialects.isEmpty()) {
- dialects = parsedDialects;
- }
+ if (parsedDialects.isEmpty()) {
+ throw new IllegalArgumentException(
+ "No valid supported_dialects found in " + configSection + ": " + dialectNames);
+ }
+ dialects = parsedDialects;
}🤖 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 727 - 736, The code silently falls back to the default dialects
when config-provided supported_dialects (dialectNames) contains only
unrecognized values; change the logic in the block that uses
DatabaseDialect.fromString to reject the configuration instead of defaulting:
after mapping and collecting parsedDialects from dialectNames, if dialectNames
was non-null/non-empty but parsedDialects.isEmpty() then throw a clear
configuration exception (or return a validation error) indicating which input
values failed to parse (use the original dialectNames and
DatabaseDialect.fromString results to report invalid entries); only assign
dialects = parsedDialects when parsedDialects is non-empty as currently done.
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.
Foreign-key columns are being generated as generic VARCHAR(255) columns.
Only sourceProfile.fields() populate the metadata maps. Columns introduced by relations/FKs, like customer_id in the current example, never get type/nullability/id metadata, so they fall back to nullable VARCHAR(255) / varchar(255). view.foreignKeys() is only used for ordering, so the generated DDL also omits the actual FK constraint. That leaves child tables with mismatched key types and no referential integrity.
Please derive relationship-column metadata from sourceProfile.relationships() or the referenced target key before building each table, and emit the corresponding FOREIGN KEY / addForeignKeyConstraint clauses.
Also applies to: 125-205
🤖 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 populates column metadata from
sourceProfile.fields(), so foreign-key columns introduced by relationships get
default VARCHAR(255) and no constraints; update the logic that builds
columnJavaType/columnNullable/columnIsId/columnIsAutoGenerated to also iterate
sourceProfile.relationships() (and/or resolve the referenced target key via view
or the target table's fields) to copy the referenced column's
type/nullability/id info into the maps for the FK column name, and collect FK
constraint descriptors; then modify the per-table generation loop (the code that
calls buildRawSql and buildLiquibaseChangeset) to pass or compute those FK
descriptors so buildRawSql and buildLiquibaseChangeset emit proper FOREIGN KEY /
addForeignKeyConstraint clauses; reference symbols:
sourceProfile.relationships(), view.foreignKeys(), byTable,
TopologicalSorter.sort(...), buildRawSql(...), buildLiquibaseChangeset(...).
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
Documentation
Tests
CodeAnt-AI Description
Add schema normalization analyzer that produces migration artifacts and improves AI parsing/provider support
What Changed
Impact
✅ Clearer AI parsing errors and retries✅ Support for OpenRouter AI provider✅ Generates dependency-ordered migration SQL and Liquibase changesets💡 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.