DeployedIndexes slim: deferred-only tracking (A/B alternative to #375)#376
Open
michal-morzywolek wants to merge 131 commits intomainfrom
Open
DeployedIndexes slim: deferred-only tracking (A/B alternative to #375)#376michal-morzywolek wants to merge 131 commits intomainfrom
michal-morzywolek wants to merge 131 commits intomainfrom
Conversation
Implements 14 test methods covering: - Valid steps with @Version and package-based versioning - Sequence ordering and validation - Error detection for missing/duplicate sequences - Invalid version format detection - Package name validation - Multiple validation error accumulation Increases coverage from 56% to 94% instruction coverage. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- DeferredIndexStatus enum: PENDING, IN_PROGRESS, COMPLETED, FAILED - DeferredIndexOperationType enum: ADD - DeferredIndexOperation domain class representing a row from the DeferredIndexOperation table plus ordered column names - DeferredIndexOperationDAO for all CRUD operations on the deferred index queue, following the UpgradeStatusTableServiceImpl pattern (SqlScriptExecutorProvider + SqlDialect, enum values stored via .name()) - 10 unit tests using ArgumentCaptor to verify DSL statement structure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ce split - Add DeferredAddIndex: SchemaChange for deferred index creation; apply() updates metadata only, reverse() removes index from metadata, isApplied() checks actual DB schema first then DeferredIndexOperation PENDING queue via DAO - Split DeferredIndexOperationDAO into interface (@ImplementedBy) + DAOImpl class, following UpgradeStatusTableService/Impl convention - Wire DeferredAddIndex into SchemaChangeVisitor (visit method), AbstractSchemaChangeVisitor (updates schema, no DDL), SchemaChangeSequence.InternalVisitor, and SchemaChangeAdaptor (default + Combining) - Add 11 tests for DeferredAddIndex covering apply, reverse, isApplied, and accept - Rename TestDeferredIndexOperationDAO -> TestDeferredIndexOperationDAOImpl Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add addIndexDeferred() to SchemaEditor interface and SchemaChangeSequence.Editor: creates a DeferredAddIndex carrying the step's @uuid, calls visitor.visit() only (no schemaAndDataChangeVisitor — no DDL runs on the target table during upgrade) - AbstractSchemaChangeVisitor.visit(DeferredAddIndex): write INSERT SQL into DeferredIndexOperation and DeferredIndexOperationColumn as part of the upgrade script; upgradeUUID read from DeferredAddIndex rather than stored visitor state - DeferredAddIndex: add upgradeUUID field + getter; updated toString() to include it - HumanReadableStatementProducer: implement addIndexDeferred() via generateAddIndexString - Tests: TestInlineTableUpgrader, TestSchemaChangeSequence, TestDeferredAddIndex updated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces DeferredIndexChangeService (interface + DeferredIndexChangeServiceImpl) to track pending deferred ADD INDEX operations within an upgrade session and emit the compensating SQL when subsequent schema changes interact with them: - RemoveIndex on a pending deferred ADD → cancel (DELETE) instead of DROP INDEX - RemoveTable → cancel all pending deferred indexes on that table - RemoveColumn → cancel pending deferred indexes referencing that column - RenameTable → UPDATE tableName in PENDING rows and update in-memory tracking - ChangeColumn (rename) → UPDATE columnName in PENDING column rows AbstractSchemaChangeVisitor delegates entirely to DeferredIndexChangeService, keeping SQL construction out of the visitor. The service is independently tested with 19 unit tests covering edge cases: multiple indexes on the same table, partial column matches, case-insensitivity, and single-UPDATE coverage for multi-index column renames. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Stage 7: DeferredIndexExecutor picks up PENDING operations, builds indexes via SqlDialect.deferredIndexDeploymentStatements(), and manages retry with exponential backoff. Progress logged at 30s intervals. Stage 8: awaitCompletion() polls the queue for multi-instance deployments where non-executor nodes must block until index builds finish. Stage 9: DeferredIndexRecoveryService detects stale IN_PROGRESS operations (exceeded staleThresholdSeconds) and resets or completes them based on whether the index exists in the schema. Stage 10: DeferredIndexValidator force-executes any PENDING operations before a new upgrade runs, ensuring no missing indexes. Supporting changes: DeferredIndexTimestamps utility, retryBaseDelayMs config field, DAO.hasNonTerminalOperations(), SqlDialect base method for deferred index DDL. 28 tests (10 executor, 6 recovery, 4 validator, 8 unit) all passing. Coverage: Executor 87%/81%, Recovery 100%, Validator 100%, Timestamps 100%. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Override deferredIndexDeploymentStatements() in PostgreSQLDialect (CREATE INDEX CONCURRENTLY) and OracleDialect (ONLINE PARALLEL NOLOGGING) so deferred index builds avoid table-level write locks. DeferredIndexExecutor.buildIndex() now uses a dedicated autocommit connection because PostgreSQL's CONCURRENTLY cannot run inside a transaction block. This is harmless for other platforms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
10 H2 integration tests covering: pending row creation, executor completion, auto-cancel, unique/multi-column/new-table indexes, populated table builds, multiple indexes per step, executor idempotency, and recovery-to-execution pipeline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…x/RenameIndex deferred handling - Fix DeferredIndexChangeServiceImpl to use DeferredIndexTimestamps.currentTimestamp() instead of System.currentTimeMillis() for createdTime (yyyyMMddHHmmss format) - Fix DeferredIndexOperationDAOImpl to use literal(boolean)/getBoolean() for the indexUnique column instead of literal(int)/getInt() - Add ChangeIndex/RenameIndex handling for pending deferred indexes in AbstractSchemaChangeVisitor: ChangeIndex cancels the deferred op and adds the new index immediately; RenameIndex updates the queued index name - Add updatePendingIndexName() to DeferredIndexChangeService/Impl - Add unit tests for deferred branches in both visitor test classes - Add integration tests for deferred-add-then-change and deferred-add-then-rename Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add retryMaxDelayMs config (default 5 min) to DeferredIndexConfig to cap exponential backoff and prevent overflow at high retry counts - DeferredIndexValidator now throws IllegalStateException when forced execution fails, blocking the upgrade until the issue is resolved - Update integration test to expect the exception on failed forced execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ry tracking - Add retryMaxDelayMs config (default 5 min) to cap exponential backoff and prevent overflow at high retry counts (#4) - DeferredIndexValidator throws IllegalStateException when forced execution fails, blocking the upgrade until resolved (#5) - updatePendingColumnName/updatePendingTableName now rebuild in-memory DeferredAddIndex entries so cancelPendingReferencingColumn finds indexes by renamed column/table names (#7) - Add unit and integration tests for all three fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace STRING(100) operationId PK on DeferredIndexOperation with BIG_INTEGER id column. Change DeferredIndexOperationColumn FK from STRING(100) to BIG_INTEGER to match. Update domain class, DAO interface/impl, services, executor, recovery service, and all tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
findOperationsByStatus and findStaleInProgressOperations now use a single LEFT OUTER JOIN query to fetch operations with their column names, eliminating per-operation column lookups. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DeferredAddIndex.apply() now uses equalsIgnoreCase() for index name comparison, consistent with reverse() and other SchemaChange classes. DeferredIndexChangeServiceImpl SQL statements now use the original casing from stored DeferredAddIndex entries rather than the caller's casing, ensuring SQL WHERE clauses match rows on case-sensitive databases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tableName, indexName, and columnName were STRING(30) which is too narrow for the validated maximum identifier length of 60 characters. Made SchemaValidator.MAX_LENGTH public and referenced it directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove existsByUpgradeUUIDAndIndexName from DAO interface and implementation — no production code calls it. Update stale comment in SchemaChangeSequence that referenced a future stage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cted directly The DAO is never injected via Guice — all consumers construct it directly with ConnectionResources. Remove the misleading annotations to match the actual usage pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
addIndexDeferred now prefixes "Deferred: " so the output is distinguishable from a regular addIndex operation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each service now validates the config fields it consumes at construction time: threadPoolSize, maxRetries, retry delays, staleThresholdSeconds, and operationTimeoutSeconds. Also fixes stale comment in SchemaChangeSequence and distinguishes deferred index in human-readable output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vate Introduce DeferredIndexService as the single public entry point for adopters. The facade orchestrates recovery, execution, and failure detection in one execute() call, and provides awaitCompletion() for passive nodes. Internal classes (Executor, RecoveryService, Validator, Operation, OperationType, Status) are now package-private. Config validation is consolidated in DeferredIndexServiceImpl. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update column and index name assertions to match actual table structure after previous refactoring (operationId → id, updated index names). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TestDeferredIndexValidatorUnit (5 tests): validates empty queue shortcut, successful execution, failure exception with count - TestDeferredIndexRecoveryServiceUnit (6 tests): stale recovery for index-exists, index-absent, table-missing, multiple ops, and case-insensitive index name matching - TestDeferredIndexExecutorUnit additions (8 tests): empty queue, single success, retry-then-success, permanent failure, getStatus before/after execution, awaitCompletion true/false paths - Added test constructors to DeferredIndexValidator and DeferredIndexRecoveryService for mock injection - Made DeferredIndexValidator.createExecutor() overridable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TestDeferredIndexOperation (2 tests): full POJO getter/setter coverage including nullable fields → 100% line coverage - TestDeferredAddIndex additions (4 tests): toString(), apply/reverse with existing other indexes, isApplied with non-matching index → 100% line coverage - TestDeferredIndexExecutorUnit additions (3 tests): unique index reconstruction, SQLException from getConnection, zero-timeout awaitCompletion → 83% line coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace internal factory methods with proper Guice @Inject/@singleton wiring so that DeferredIndexService can be injected by adopters. All services now share a single DAO instance instead of each creating its own. Bind DeferredIndexConfig in MorfModule. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deployers can now configure a set of index names that should be built immediately during upgrade even when the upgrade step uses addIndexDeferred(). This allows overriding deferral for critical indexes without modifying upgrade steps. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deployers can now configure a set of index names that should be deferred even when the upgrade step uses addIndex(). This enables retroactive deferred index creation on old upgrade steps without modifying them. Includes conflict validation that throws if an index name appears in both forceImmediateIndexes and forceDeferredIndexes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ChangeColumn.applyChange() rebuilds indexes that reference the renamed column but previously only preserved isUnique(). isDeferred() was reset to false, causing getDeferredIndexStatements() to miss the renamed index after a cross-step column rename. Also add two integration tests: - testPrepopulationPopulatesExistingIndexes: verifies CreateDeployedIndexes populates pre-existing physical indexes. - testCrossStepColumnRenameOnNonDeferredIndex: verifies column rename on a non-deferred index updates indexColumns in DeployedIndexes. Strengthen testCrossStepColumnRename to assert the renamed column is now properly reflected in getDeferredIndexStatements(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The app runs deferred-index SQL itself on this branch — Morf no longer owns a thread pool. The field, getter, and setter had no callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- SqlDialect.buildCreateIndexStatement: drop unused afterIndexKeyword parameter (no caller ever passed a non-empty value). - PostgreSQLDialect.buildPostgreSqlCreateIndex: replace the String "afterIndexKeyword" with a clearer `boolean concurrent` flag. - Rename EnrichedIndex -> ObservedIndex and expand the class javadoc with a concise reason-for-existence (declarative vs observed state). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the ObservedIndex decorator pattern with a Data/Context split. The Index interface goes back to being purely declarative (name, columns, unique, deferred) and the runtime-observed fact "is this index physically present?" moves to a new DeployedIndexState object produced by the enricher alongside the schema. Why: the decorator + default-method combo was incoherent — Index had a runtime-only property (isPhysicallyPresent) as a leaky default, and the ObservedIndex class existed only to override that one method. With the split, data is data and operational state is state, and no type needs to straddle both. Changes: - Add DeployedIndexState with two explicit queries — isKnownPhysicallyPresent (for the getDeferredIndexStatements scan) and isKnownPhysicallyAbsent (for the visitor's DDL decisions). Different defaults for unknown reflect what each caller actually wants. - Refactor DeployedIndexesModelEnricher: enrichSchema(Schema)->Schema becomes enrich(Schema)->Result (schema + state). Physical indexes are rebuilt via SchemaUtils.index() to carry the declarative deferred flag from the tracking table; deferred-not-yet-built entries appear as virtual indexes in the schema; presence is recorded in the state. - Remove Index.isPhysicallyPresent() default method and delete ObservedIndex + TestObservedIndex. - Thread DeployedIndexState through AbstractSchemaChangeVisitor and InlineTableUpgrader (new overload; empty-state default preserved). - Upgrade.findPath wires the state from enricher to upgrader and to the final deferred-SQL scan. Tests: TestDeployedIndexesModelEnricher rewritten against the new API with more precise assertions (schema fact + state fact). Stale isPhysicallyPresent mocks removed from visitor/upgrader tests; those mocks were load-bearing under the old code but redundant now because the visitor gets the same answer from the state's default. All 27 deployed-index integration tests + full module build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, inline parsing
- Drop the unused upgradeUUID column from the DeployedIndexes table and
from every plumbing layer (DAO, change service, entity, visitor call
sites, Editor). The column was never populated anywhere; wiring it up
properly was out of scope for now.
- Widen indexColumns from 2000 to 4000 chars, matching the comfortable
headroom we want for composite indexes.
- Rename DeployedIndexEntry -> DeployedIndex. The POJO is the row type;
the "Entry" suffix was noise (mirroring the DeployedViews convention,
which has no separate entry class at all).
- Remove the static DeployedIndex.parseColumns / joinColumns helpers.
parseColumns had one caller (the DAO); inline it there as
Arrays.asList(s.split(",")). joinColumns was dead. Deletes the two
matching tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two-method boolean API (isKnownPhysicallyPresent, isKnownPhysicallyAbsent)
encoded a three-state value (present, absent, unknown) via a nullable Boolean
in the internal map. Readers had to mentally decode what false meant in each
method, and !isKnownPhysicallyPresent was not the complement of
isKnownPhysicallyAbsent (both false when UNKNOWN).
Replace with an explicit enum: IndexPresence { PRESENT, ABSENT, UNKNOWN }.
Single getPresence(tableName, indexName) method. Call sites use explicit
!= PRESENT / != ABSENT comparisons — the tri-state is now visible in every
read and write.
- New IndexPresence enum.
- DeployedIndexState: Map<String, IndexPresence> internally.
- Drop both boolean wrapper methods — don't reintroduce the ambiguity.
- Update AbstractSchemaChangeVisitor and Upgrade.findPath call sites.
- Enricher records PRESENT/ABSENT explicitly.
- TestDeployedIndexesModelEnricher collapses two-asserts-per-case into
single assertEquals on the enum.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract Result to its own top-level class EnrichedModel. Returned type is now a first-class public API surface rather than a nested-and-generic inner class. - Split DeployedIndexesModelEnricher.enrich() into five private methods: fastPathEmpty, enrichTable, processPhysicalIndexes, processRemainingTrackingEntries, logOrphanTrackingRows. enrich() is now a ~20-line orchestrator. - Add WHY comments for the non-obvious invariants: _PRF exclusion, the "consumed tracking entry is removed" invariant that makes the leftover loop correct, why missing non-deferred is a hard error, why orphan tracking rows are a warn. Callers updated: Upgrade.findPath, TestDeployedIndexesModelEnricher, TestUpgrade.mockEnricher. No behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tatements
UpgradePath.getDeferredIndexStatements() used to return a List<String> of
raw CREATE INDEX SQL. The adopter then had no reliable way to recover the
(tableName, indexName) needed for DeployedIndexTracker calls — parsing
the SQL with regex, maintaining a side-map, or querying pending entries
separately and regenerating SQL were the only workarounds.
Change the return type to List<DeferredIndexJob>. Each job carries:
- tableName
- indexName
- sql: the statements (usually one, sometimes more e.g. PostgreSQL's
COMMENT ON INDEX) to build that one index
The adopter loop becomes:
for (DeferredIndexJob job : path.getDeferredIndexStatements()) {
tracker.markStarted(job.getTableName(), job.getIndexName());
try {
for (String sql : job.getSql()) executeSql(sql);
tracker.markCompleted(job.getTableName(), job.getIndexName());
} catch (Exception e) {
tracker.markFailed(job.getTableName(), job.getIndexName(), e.getMessage());
}
}
Also update TestDeployedIndexesIntegration — assertions that previously
did .anyMatch(s -> s.toUpperCase().contains("INDEX_NAME")) now target
job.getIndexName() / job.getTableName() directly; assertions that really
needed to peek at the SQL content (UNIQUE keyword, renamed-column name
in CREATE INDEX) flatMap into job.getSql().
Breaking change but no adopter consumed this API on this branch yet.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gate all DSL Replace the scattered DSL construction in DAOImpl and ChangeServiceImpl with a single source of truth. The two classes now have one responsibility each, distinct column constants are consolidated, and there's one place to look for "how does Morf talk to the DeployedIndexes table". DeployedIndexesStatementFactory (new): - Owns every DSL statement targeting the DeployedIndexes table — reads, status updates executed by the DAO, and tracking DML used by the visitor. - Column-name constants live here; the other classes no longer have their own copies. - Methods grouped: read queries, status updates, tracking DML. DeployedIndexesDAOImpl: - Now a thin executor. Each public method asks the factory for DSL, converts via the dialect, executes. Still owns the ResultSet mapper. - No DSL construction inline. DeployedIndexesChangeServiceImpl: - In-memory session state + orchestration only. - All returned Statement lists come from factory calls; no insert/update/ delete DSL built here. Both the DAO and the ChangeService accept an explicit factory constructor for tests; default constructors create their own. No behavioural change — all 4,700+ tests across 11 modules pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… on tracker The DAO and StatementFactory are internal machinery — no external adopter should need to construct them. Tighten the API surface: - DeployedIndexesDAO interface, DeployedIndexesDAOImpl, and DeployedIndexesStatementFactory go from public to package-private. Constructors too. - Add DeployedIndexesModelEnricher.create(ConnectionResources, UpgradeConfigAndContext) factory method. Upgrade.java used to hand-wire the DAO with a cross-package `new DeployedIndexesDAOImpl(...)`; now it calls the factory, keeping the DAO invisible. - Add resetInProgress() to DeployedIndexTracker. TestDeployedIndexesIntegration now uses tracker.resetInProgress() for the crash-recovery test instead of constructing a DAO directly — matches how adopters would use the API. The integration tests happen to still compile because they live in the same Java package name as the classes (even though they're in a separate Maven module), so package-private access still works. For actual external adopters these types are now invisible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three new unit-test classes covering previously-untested machinery, and negative/edge-path additions to three existing test files. New test classes: - TestDeployedIndexState — tri-state enum behaviour (empty/PRESENT/ABSENT/ UNKNOWN) and case-insensitive keys. 5 tests. - TestDeployedIndexesStatementFactory — asserts DSL shape for every statement method (read, status update, tracking DML), plus multi-column CSV join and deferred/non-deferred status mapping. 17 tests. - TestDeployedIndexTrackerImpl — mocks the DAO and verifies pure delegation for every tracker method including resetInProgress. 6 tests. - TestDeployedIndexesDAOImpl — mocks the factory + dialect + executor; verifies each DAO method calls the correct factory method, converts via the dialect, and executes; plus ResultSet mapping including null startedTime/completedTime via wasNull. 9 tests. Negative/edge additions to existing tests: - TestDeployedIndexesChangeServiceImpl — no-op assertions for removeIndex / removeAllForTable / removeIndexesReferencingColumn / updateTableName / updateColumnName / updateIndexName on untracked tables; updateIndexName on unknown index; case-insensitive column matching in updateColumnName; multi-column INSERT shape verification. - TestDeployedIndexesModelEnricher — isUnique + multi-column order preservation in rebuildIndex; non-deferred + physical path directly asserted; mixed physical + virtual on one table; multiple tables; orphan-row tolerance (log.warn, no throw). - TestDeployedIndex — composite column order preservation in toIndex(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Real bug caught by the tightened test assertions:
- DeployedIndexesStatementFactory.statementToMark{Started,Completed,Failed}
and statementToBumpRetryCount used .where(X.eq(a).and(Y.eq(b))). Because
Criterion.and is statically imported and Criterion has no instance .and(),
the compiler resolved .and(Y.eq(b)) as a static call and silently discarded
the LHS X.eq(a). Bytecode confirmed with a `pop`. Result: the UPDATE
filtered by indexName only, ignoring tableName. Two indexes with the same
name on different tables would cross-update. Fixed to use the explicit
and(c1, c2) form.
Review fixes:
- Remove dead deferredIndexes field + getDeferredIndexes() in
AbstractSchemaChangeVisitor (populated but never read).
- Replace the unreachable-but-misleading else-branch in
visitDeployedIndexesStatement with an IllegalStateException. Aligns with
the class's own documented "no schema validation" contract.
- Delete speculative static DeployedIndexesStatementFactory.allColumns() and
its self-referential test.
New tests + stronger assertions:
- TestDeferredIndexJob (new): constructor null-safety via Objects.requireNonNull
on all three args; multi-statement SQL order; unmodifiable and decoupled
sql list; per-arg NPE messages.
- TestDeployedIndexesStatementFactory: factory tests now verify SET-field
aliases and literal values, WHERE AND structure and leaf values — not
just counts. This is what caught the tracker WHERE bug.
- TestDeployedIndexesChangeServiceImpl.testUpdateColumnName: now verifies
UPDATE sets the right indexColumns value and targets the correct index.
- TestDeployedIndexTrackerImpl: markStarted/markCompleted now assert a
[before, after] bounds check on the captured timestamp; no more anyLong().
- TestDeployedIndexesIntegration.testDisabledFeatureBuildsDeferredImmediately:
captures UpgradePath and asserts jobs list is empty.
- TestDeployedIndexesIntegration new tests:
testAppSideAdopterFlowBuildsAndMarksCompleted — exercises the full
documented adopter loop (markStarted → execute job SQL → markCompleted);
testAppSideAdopterFlowMarksFailed — verifies markFailed flips status and
persists errorMessage.
- Shared newTracker() helper across integration tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s handling into buildUpgradePath Upgrade.java: - Replace fully-qualified org.alfasoftware.morf.upgrade.deployedindexes.* references with imports. - Fold deferredIndexJobs setup into buildUpgradePath (now takes List<DeferredIndexJob>) rather than setting it at the call site; findPath simplifies to a single return. - Revert unnecessary hoisting of the InlineTableUpgrader local variable. DeployedIndexesChangeServiceImpl.java: - Remove accidental "jjv" typo between field declarations. SchemaChangeAdaptor.java, SchemaChangeSequence.java: - Trim trailing blank lines (no behaviour change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… into graph-based visitor Removes silent-empty-state / silent-null-schema / silent-new-factory footguns across the branch and fixes a latent production bug where GraphBasedUpgradeSchemaChangeVisitor was constructed via its 4-arg super call, silently substituting DeployedIndexState.empty() and degrading the visitor's model-based DDL decisions to default behaviour. Constructors deleted (shorter-arg convenience forms that silently default a dependency): - AbstractSchemaChangeVisitor (4-arg → gone; 5-arg required) - InlineTableUpgrader (5-arg → gone; 6-arg required) - SchemaChangeSequence (1-arg and 2-arg → gone; 3-arg required) - UpgradePathFinder.getSchemaChangeSequence() no-arg → gone; 1-arg required - DeployedIndexesDAOImpl 2-arg → gone; Guice wires factory via @Inject on 3-arg - DeployedIndexesChangeServiceImpl no-arg → gone; 1-arg required Editor.getSourceSchema() silent SchemaUtils.schema() fallback is removed: sourceSchema is now non-null via Objects.requireNonNull in the surviving constructor. Real DeployedIndexState threaded through the graph-based chain: - GraphBasedUpgradeSchemaChangeVisitor constructor + factory - GraphBasedUpgradeBuilder field + constructor + factory - Upgrade.findPath passes deployedIndexState to graphBasedUpgradeBuilderFactory.create(...) DeployedIndexesStatementFactory is now public (needed by AbstractSchemaChangeVisitor to construct DeployedIndexesChangeServiceImpl without reflection). P1.6 will split this into interface + impl. SchemaEditor.getSourceSchema() Javadoc now honestly documents the retrofit: a read method on an otherwise-pure-write interface, added for infrastructure steps like CreateDeployedIndexes that need the pre-upgrade schema. DeployedIndexState gains two public test-friendly factories (of/with) so tests outside the package can construct populated states. Verified by a new regression test testRemoveIndexVisitRespectsAbsentStateForGraphBasedPath in TestGraphBasedUpgradeSchemaChangeVisitor: when state reports an index as ABSENT, the graph-based visitor no longer emits spurious DROP INDEX DDL. Updated callers (tests): - UpgradeTestHelper, TestInlineTableUpgrader, TestGraphBasedUpgradeSchemaChangeVisitor, TestGraphBasedUpgradeBuilder, TestSchemaChangeSequence (6 sites), TestUpdateToCtasAdaptor, TestUpgradePathFinder, TestDeployedIndexesChangeServiceImpl, TestDeployedIndexTracker, TestDeployedIndexesIntegration — all pass DeployedIndexState.empty() / SchemaUtils.schema() / DeployedIndexesStatementFactory explicitly where relevant. Verification: morf-core unit tests (2737, +1 new), deployed-indexes integration tests (29), checkstyle, spotbugs, javadoc all clean on morf-core verify. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ment, hard-fail on orphans DeployedIndexesModelEnricher: - Replace Optional-returning fastPathEmpty(...) with boolean shouldSkipEnrichment(...). Eliminates the triple-return and the double dao.findAll() call (one call in enrich, the "table empty" check becomes an inline early-return against the fetched list). - Add missing debug log on the feature-disabled branch. - Delete the isMorfInfrastructureTable skip in enrich() and in the orphan check. Morf tables (UpgradeAudit, DeployedViews, DeployedIndexes) are now enriched like any other table; consistency validation applies. _PRF indexes remain the only carve-out. - Rename local entryMap → trackingRowsByTable (and buildEntryMap → buildTrackingRowsByTable). Rename presence → observedPresence across enrich and helper method parameters. - Collapse the enrichTable result block using orElse + |=. - Replace soft-warn logOrphanTrackingRows with hard-fail validateNoOrphanTrackingRows: throws IllegalStateException on first orphan. Orphans cannot be produced by correct Morf operation (RemoveTable/RenameTable always emit matching tracking DML), so the same severity class as non-deferred-missing-from-DB (already a hard error) applies. - processRemainingTrackingEntries now clears the inner map after consuming entries — the new hard-fail requires all consumed entries to be cleared so only truly-orphan tables remain for validation. - Delete isMorfInfrastructureTable helper (no remaining callers). CreateDeployedIndexes: - Delete the isMorfTable skip in the prepopulation loop and the helper method. Morf-table indexes are now prepopulated alongside user-table indexes. Since this branch is pre-release, no migration step is needed for existing deployments — fresh CreateDeployedIndexes handles all tables on first run. Tests: - TestDeployedIndexesModelEnricher: testOrphanRowForMissingTableDoesNotThrow flipped to testOrphanRowForMissingTableThrows with @test(expected = IllegalStateException.class). New tests confirm Morf infrastructure tables ARE enriched: indexes on UpgradeAudit recorded PRESENT in state, and an untracked physical index on DeployedViews triggers consistency validation. - TestDeployedIndexesIntegration: updated testPrepopulationPopulatesExistingIndexes Javadoc to reflect that Morf tables are no longer excluded. Verification: morf-core 2739 tests pass, checkstyle + spotbugs + javadoc clean, all 26 TestDeployedIndexesIntegration tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ployedIndexState Adds a package-private IndexKey(tableName, indexName) value type that upper-cases both names on construction. Replaces the Map<String, IndexPresence> keyed by a string-concat convention (TABLE:INDEX) with Map<IndexKey, IndexPresence>. Benefits: - Type-safe: callers can't accidentally pass a pre-formatted string where a key was expected. - Canonicalisation (upper-casing) is encapsulated in the key type; consumers no longer need to know the key format. - Collisions are impossible (no TABLE:INDEX ambiguity if a name contains ':'). - Eliminates the last static helper on DeployedIndexState (the package-private key() method). Changes: - New IndexKey.java (with equals/hashCode/toString, null-rejection). - DeployedIndexState internal map is now Map<IndexKey, IndexPresence>; the static key() helper is deleted. Public API (getPresence/empty/of/with) unchanged. - DeployedIndexesModelEnricher uses new IndexKey(...) instead of DeployedIndexState.key(...). Tests: - New TestIndexKey covers equals/hashCode/case-insensitivity/toString/null-rejection. - TestDeployedIndexState updated to construct Map<IndexKey, IndexPresence> via new IndexKey(...) directly — existing case-insensitivity tests still pass. Verification: morf-core 2747 tests pass (+8 new TestIndexKey), checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundle of related refactors in the DDL-emission path. CreateDeployedIndexes: - @sequence(2) → @sequence(1). Explicit about "runs first"; Morf-internal steps all use Unix-epoch-seconds values (billions), so they naturally sort after it. AbstractSchemaChangeVisitor: - Replace the visitDeployedIndexesStatement(Statement) instanceof chain with three typed overloads — writeDeployedIndexesDml(Insert/Update/Delete). New DML types force a missing-overload compile error rather than a runtime IllegalStateException. - Rename isPhysicallyPresent → willBePhysicallyPresentAtThisEmission. Nothing has hit the DB yet at visitor time — the method is a projection ("will the index exist at this emission point"), not a query. New name reflects that. - Delete the defensive currentSchema.tableExists(tableName) branch; all callers invoke this before SchemaChange.apply() which would fail loudly on a missing table anyway. - Add capture-before-mutate comments at each local-variable capture (RemoveIndex, ChangeIndex, RenameIndex): both isTrackedDeferred and the enricher state would be out of sync by DDL-emission time otherwise, so inlining would silently produce wrong DDL. - Split visit(AddIndex) into four named helpers: shouldEmitPhysicalIndexDdl, emitAddIndexOrRename, findMatchingIgnoredIndex, trackInDeployedIndexes. The top-level method collapses to apply+DDL+track — reads as prose. - Collapse visit(ChangeIndex) to reuse the same helpers. Tracking call that previously appeared twice (deferred vs immediate branches) is now one call. DeployedIndexesChangeService(Impl): - Tighten return types: List<Statement> → List<InsertStatement> / List<DeleteStatement> / List<UpdateStatement> on each method. DeployedIndexesStatementFactory already returned concrete types; the ChangeService's List<Statement> was leaking implementation laxity into the contract. SchemaChangeSequence.Editor: - Simplify resolveDeferred: split into resolveTargetDeferred(index) → boolean pure predicate + two-line rebuild-on-mismatch call. The triple-repeated index.isDeferred() ? X : Y ternary collapses into one equality check. - Add private isForcedImmediate/isForcedDeferred helpers reading the config's case-insensitive stored sets directly. These replace calls to the config's now-deleted predicate methods. UpgradeConfigAndContext: - Delete isForceImmediateIndex(String) and isForceDeferredIndex(String) — behavioural predicates don't belong on a data/setter class. Setter-side case-folding stays as pure normalisation. Public getForceImmediate/DeferredIndexes getters stay. Tests: - TestSchemaChangeSequence: testIsForceImmediateIndex / testIsForceDeferredIndex replaced with testForceImmediate/DeferredIndexesStoredCaseInsensitively — they now assert only the storage contract (normalised, deduped). Behavioural coverage is already provided by the existing testAddIndexDeferredWithForceImmediateProducesAddIndex / testAddIndexWithForceDeferredProducesDeferredAddIndex / case-insensitive variants, which exercise resolveDeferred end-to-end. - TestDeployedIndexesChangeServiceImpl: local List<Statement> declarations widen to List<? extends Statement> to match the tightened return types. Verification: morf-core 2747 tests pass, checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… Javadoc Upgrade.java: - Extract collectDeferredIndexJobs(schemaChangeSequence, sourceSchema, deployedIndexState, dialect) → List<DeferredIndexJob>. The ~20-line scan that used to live inline in findPath becomes a named, individually-testable helper with a documented early-return on isDeferredIndexCreationEnabled(). - Extract enrichSourceSchema(sourceSchema) → EnrichedModel. The pair-mutation of sourceSchema + deployedIndexState now happens at one named boundary. Null-guard on deployedIndexesModelEnricher stays (legacy test paths). - findPath's two ~20-line blocks collapse to two one-liners. - buildUpgradePath passes deferredIndexJobs through the factory (see below); the if(!isEmpty) guard + setDeferredIndexStatements setter call disappear. Upgrade.performUpgrade Javadoc: - Document the void → UpgradePath return-type change: callers access UpgradePath.getDeferredIndexStatements() for the list of DeferredIndexJob entries the application must execute asynchronously via DeployedIndexTracker. - Note these static entry points are simplified and primarily used by tests/examples; production code typically goes through Upgrade.Factory. UpgradePath.java: - Add import for DeferredIndexJob; replace three inline fully-qualified references with the imported name. - Make deferredIndexJobs field final — initialised only in the constructor. - Delete setDeferredIndexStatements(...) setter. - Widen the 6-arg UpgradePath constructor to 7-arg with List<DeferredIndexJob>. - Keep the 4-arg and 5-arg public constructors as explicit "no-deferred-jobs" shapes (empty-path sentinel and simpler test/build paths, per the plan). Their Javadoc documents the no-jobs contract. - Widen UpgradePathFactory.create 4-arg → 5-arg (+ List<DeferredIndexJob>). The 1-arg and 2-arg overloads stay unchanged — they're for paths that genuinely can't have deferred jobs; not convenience defaults. CreateDeployedIndexes: - Build the DeployedIndexes table as a local variable so the prepopulation loop can iterate its OWN indexes too. Without this, the table's physical indexes (DeployedIdx_1, DeployedIdx_2) would be untracked, and the P1.2 enricher hard-fail on untracked physical indexes would trigger on any subsequent upgrade run. Caught by TestDeployedIndexesIntegration.testReUpgradeIsIdempotent + testSequentialUpgradeIncludesPreviousDeferred. - Extract a small sourceSchema(SchemaEditor) helper to keep the main loop one-liner-shaped. Tests: - TestUpgrade.upgradePathFactory() mock signature updated: create(..., anyList(), anyList(), nullable(builder), anyList(), anyList()). - TestUpgradePath.testFactoryCreateUpgradeWithInitialisationSql: the factory call grows an ImmutableList.of() for deferredIndexJobs. Verification: morf-core 2747 tests pass, checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…exes package Renames: - DeployedIndexesChangeService → DeployedIndexesService. The word "Change" was misleading: Morf uses "Change" only as a noun for SchemaChange objects (ChangeColumn, ChangeIndex, SchemaChangeVisitor), never as a service verb. Service classes use noun-based names (UpgradeStatusTableService, DatabaseUpgradePathValidationService). git mv applied to both production files and the test file; sed applied class + field-name references (deployedIndexesChangeService → deployedIndexesService). Interface + impl splits: - DeployedIndexesStatementFactory promoted to interface with @ImplementedBy. New DeployedIndexesStatementFactoryImpl carries the bodies. Column-name constants moved to the interface as static final (allowed in Java 8+ interfaces). Callers updated to instantiate Impl for non-Guice contexts (visitor, tests, static create factories). - DeployedIndexesModelEnricher promoted to interface with @ImplementedBy. New DeployedIndexesModelEnricherImpl carries the bodies. The static create(...) factory moves to the interface (Java 8+). LogFactory.getLog() switches to the Impl class. Test file renamed to TestDeployedIndexesModelEnricherImpl. Integration test fixture fix: - TestDeployedIndexesIntegration.INITIAL_SCHEMA pre-creates the DeployedIndexes table via schemaManager, bypassing the CreateDeployedIndexes step. Since P1.2 removed the Morf-table skip and made the enricher hard-fail on untracked physical indexes, tests that trigger enrichment on the pre-created DeployedIndexes table (testReUpgradeIsIdempotent, testSequentialUpgradeIncludesPreviousDeferred) would fail. Added seedDeployedIndexesOwnTrackingRows() in setUp that inserts tracking rows for DeployedIdx_1 and DeployedIdx_2 — mirrors what CreateDeployedIndexes would have done in production. CreateDeployedIndexes: - Removed the redundant "dedicated loop" added in P1.5 that inserted tracking rows for DeployedIdx_1/DeployedIdx_2. The visitor's visit(AddTable) path already tracks these via deployedIndexesChangeService.trackIndex(...) when the step calls schema.addTable(deployedIndexesTable). The P1.5 dedicated loop was double-inserting and triggering unique-constraint violations. Verification: morf-core 2747 tests pass, checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
IndexPresence.java: - Rewrite UNKNOWN Javadoc to distinguish happy path from bug path. - Happy path: UNKNOWN is the normal result for indexes that first appear during the current upgrade session — the enricher runs once against the source schema + tracking table and does not see subsequent in-memory mutations. Callers decide meaning in context (AbstractSchemaChangeVisitor.willBePhysicallyPresentAtThisEmission treats UNKNOWN as "present"; Upgrade.collectDeferredIndexJobs treats UNKNOWN as "needs building"). - Bug path: UNKNOWN should NOT appear for an index that was live in the source schema or had a tracking row when the enricher ran — that would indicate the enricher skipped work (a bug). - Class-level Javadoc lifts the distinction so readers don't conflate the two meanings. DeployedIndexState.getPresence Javadoc: - Added a one-line pointer to IndexPresence.UNKNOWN for the full happy-path-vs-bug distinction and caller interpretations. mvn javadoc:javadoc clean (no warnings anywhere in morf-core). The earlier commits (P1.1–P1.6) already added Javadoc to every new method in the deployedindexes package and every modified method across the branch — this commit completes the pass by addressing the one remaining ambiguity (UNKNOWN semantics) and cross-referencing it from the sole consumer of its return value. Verification: morf-core 2747 tests pass, checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ulation assertions Caught during self-audit after the Phase 1 push: TestSchemaChangeSequence: - Add testAddIndexDeferredWithKillSwitchOffProducesImmediate — covers the resolveDeferred kill-switch-off branch (setDeferredIndexCreationEnabled(false) rebuilds a declared-deferred index as non-deferred). Missing from the existing coverage which only hit the force-immediate / force-deferred / default-passthrough branches. TestDeployedIndexesIntegration.testPrepopulationPopulatesExistingIndexes: - Add assertions that DeployedIdx_1 and DeployedIdx_2 (the DeployedIndexes table's own indexes) are recorded in the tracking table after CreateDeployedIndexes runs. Exercises the P1.2 removal of the Morf-infrastructure-table skip + the visitor's visit(AddTable) auto-tracking behaviour end-to-end. Verification: morf-core 2748 tests pass (+1), checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a third parameter to UpgradeStep.execute — an UpgradeContext providing
read-only upgrade-time state — and deletes the SchemaEditor.getSourceSchema()
retrofit added in Phase 1.
Rationale: SchemaEditor is a command interface ("tell me what to change").
Adding a read method to it was a retrofit for one consumer (CreateDeployedIndexes).
UpgradeContext is the principled home for read-only state steps may need;
future needs (dialect, UpgradeConfigAndContext, step metadata) grow the
context rather than polluting SchemaEditor further.
Contract:
- Framework always invokes the 3-arg execute(schema, data, context).
- The 2-arg execute stays abstract — adopters must implement it.
- The new 3-arg execute has a default implementation that bridges to the
2-arg form, so existing adopter steps that only override 2-arg keep
working unchanged.
- Steps needing context override 3-arg and provide a throwing stub for
2-arg (never called when 3-arg is overridden).
- Override ONE of the two, not both — documented in Javadoc on both
overloads. Overriding both is legal but not useful: only 3-arg runs;
the 2-arg body is dormant unless the 3-arg override invokes it
explicitly via execute(schema, data).
Backwards compatibility:
- Source- and binary-compatible for the common case (existing adopter
steps overriding only 2-arg) — the new 3-arg method is a default,
invisible unless opted into.
- One adopter in the repo (CreateDeployedIndexes) migrates to the 3-arg
form; all other steps untouched.
- HumanReadableStatementProducer's anonymous SchemaEditor never called
getSourceSchema, so no change needed there.
Files changed:
- New: morf-core/.../UpgradeContext.java — interface with getSourceSchema().
- UpgradeStep.java: add default 3-arg execute + explicit "override one"
Javadoc on both overloads.
- SchemaEditor.java: delete the getSourceSchema() default method (retrofit
removed) + unused Schema import.
- SchemaChangeSequence.java: Editor drops its sourceSchema field and
getSourceSchema override; constructor loop builds UpgradeContext as a
lambda (() -> sourceSchema) and invokes the 3-arg execute.
- CreateDeployedIndexes.java: 2-arg execute becomes a throwing stub;
3-arg execute overrides and reads context.getSourceSchema().
- UpgradePathFinder.java: update the stale {@link SchemaEditor#getSourceSchema}
Javadoc reference to point at the 3-arg execute.
Verification: morf-core 2748 tests pass, checkstyle + spotbugs + javadoc
clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker
3/3 pass end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…port On dialects where SqlDialect.supportsDeferredIndexCreation() returns false (MySQL, SQL Server, and the base SqlDialect default), a declared-deferred index was being handled inconsistently: - Visitor emitted CREATE INDEX immediately (correct — shouldEmitPhysicalIndexDdl returns true when dialect doesn't support deferred). - But trackInDeployedIndexes passed the original declared flag to statementToTrackIndex, which unconditionally wrote indexDeferred=true / status=PENDING based on the declared flag. - Upgrade.collectDeferredIndexJobs then saw the deferred flag in the final schema and (state = UNKNOWN because the new index didn't exist at enrichment time) added it to the app-side executor's jobs list. - The app would then issue a second CREATE INDEX → duplicate-index error. This was not caught by integration tests because morf-h2v2 explicitly overrides supportsDeferredIndexCreation() to return true (so the test harness exercises the full deferred pipeline even on H2). Fix — two parts: AbstractSchemaChangeVisitor: - New effectiveIndex(Index) helper: on dialects without deferred support, rebuild a declared-deferred index without the deferred flag. The returned index flows into shouldEmitPhysicalIndexDdl (still emits CREATE) and trackInDeployedIndexes (now tracks as COMPLETED / indexDeferred=false). - Applied in visit(AddIndex), visit(ChangeIndex) (for toIndex), and visit(AddTable) (per-index in the loop). Upgrade.collectDeferredIndexJobs: - Early-return empty list when dialect.supportsDeferredIndexCreation() is false. The app-side executor would otherwise receive jobs for indexes already built at upgrade time, causing duplicate-CREATE errors. Tests: - TestInlineTableUpgrader.testVisitAddIndexDeferredOnDialectWithoutDeferredSupport: mock dialect with supportsDeferredIndexCreation=false, verify CREATE INDEX emitted AND tracking INSERT carries indexDeferred=false / status=COMPLETED (not PENDING). - TestInlineTableUpgrader.testVisitChangeIndexToDeferredOnDialectWithoutDeferredSupport: same shape, ChangeIndex immediate→declared-deferred path. - Two small helpers findBooleanLiteralByAlias / findStringLiteralByAlias inspect the captured InsertStatement's values without SQL parsing. Verification: morf-core 2750 tests pass (+2), checkstyle + spotbugs + javadoc clean, TestDeployedIndexesIntegration 26/26 + TestDeployedIndexTracker 3/3 pass end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Part 1 of the deferred-tracking-only redesign on experimental/deferred-indexes-slim. AbstractSchemaChangeVisitor: - visit(AddTable): gate trackInDeployedIndexes on effectiveIndex(idx).isDeferred(). Non-deferred indexes on a newly-added table produce no tracking row. - visit(AddIndex): gate trackInDeployedIndexes on effectiveIndex(newIndex).isDeferred(). - visit(ChangeIndex): track toIndex only if its effective form is deferred. removeIndex call for fromIndex stays unconditional — the DELETE WHERE is a no-op for non-existent tracking rows. DeployedIndexesService / Impl: - New prime(DeployedIndex) method. Populates the in-session tracking map from a persisted row WITHOUT emitting DML. Called by the enricher (SP2) at upgrade start for every persisted deferred row, so subsequent remove/rename/column operations correctly produce DML against rows from prior upgrades. Rebuilds the Index with isDeferred()=true (slim invariant: every tracking row is a deferred index). Tests: - TestInlineTableUpgrader: the two dialect-support tests added in 1e8f9e5 (testVisitAddIndexDeferredOnDialectWithoutDeferredSupport, testVisitChangeIndexToDeferredOnDialectWithoutDeferredSupport) flip from "assert tracking row carries indexDeferred=false / status=COMPLETED" to "assert no tracking INSERT is emitted at all". Slim makes the previously- latent bug structurally impossible — no tracking row → app-side executor has nothing to double-CREATE. Helper methods findBooleanLiteralByAlias / findStringLiteralByAlias are no longer used; deleted. - TestDeployedIndexesServiceImpl: new testPrimeSeedsInSessionStateWithoutEmittingDml verifies prime seeds the map and subsequent removeIndex produces DML. Integration tests (TestDeployedIndexesIntegration): - testCrossStepColumnRenameOnNonDeferredIndex renamed to testCrossStepColumnRenameOnNonDeferredIndexDoesNotTrack; assertions flipped to verify no tracking row exists (slim: non-deferred not tracked). - testNonDeferredIndexBuiltImmediately: assertions flipped — physical index exists, no tracking row. - testForceImmediateBypassesDeferral: assertions flipped — when force-immediate ends up non-deferred, no tracking row is created. - testPrepopulationPopulatesExistingIndexes: DELETED (SP3 work pulled forward to keep the branch green; prepopulation doesn't exist in slim). Verification: morf-core 2751 tests pass (+1), TestDeployedIndexesIntegration 25/25 + TestDeployedIndexTracker 3/3 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…only
Drops the 295-line consistency-validation-heavy enricher in favour of two
responsibilities:
1. Prime the per-session DeployedIndexesService with every persisted row
so visitor remove/rename/column operations against prior-upgrade
tracking rows emit correct DML.
2. Virtualize unbuilt deferred indexes (status != COMPLETED) into the
schema so SchemaHomology.schemasMatch treats them as declared.
Physical-vs-declared drift for non-deferred indexes is no longer this
class's concern — SchemaHomology handles it at upgrade-path-finding time.
Enricher's enrich() grows a DeployedIndexesService parameter so the
enricher and visitor share one primed service instance. Service is now
constructed once per run in Upgrade.findPath and threaded through the
InlineTableUpgrader / GraphBasedUpgradeBuilder / AbstractSchemaChangeVisitor
chain (replacing the inline `new DeployedIndexesServiceImpl(...)` that
lived in the abstract visitor).
TestDeployedIndexesModelEnricherImpl rewritten: dropped 11 tests for
deleted consistency-validation behaviours, kept 4, added coverage for
priming (verify-via-mock + primed-state-is-tracked) and
COMPLETED-row-not-virtualized.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The slim invariant ($DeployedIndexes$ = deferred-only) eliminates the
rationale for the Phase-2 UpgradeContext mechanism: there's nothing to
prepopulate — non-deferred indexes live only in the physical DB, where
SchemaHomology handles them.
Consequently:
- Delete UpgradeContext.java entirely.
- Revert UpgradeStep to the single 2-arg execute(schema, data). Drop the
default 3-arg bridge and the "override one not both" Javadoc.
- Revert SchemaChangeSequence: ctor drops the sourceSchema param; loop
reverts to step.execute(editor, editor).
- Revert UpgradePathFinder: getSchemaChangeSequence drops its Schema
param; determinePath callsite reverts.
- Strip CreateDeployedIndexes down to a CREATE TABLE: deletes the 3-arg
override, the prepopulation loop, and all DataEditor / UUID / Index /
DatabaseMetaDataProviderUtils imports.
- Drop the seedDeployedIndexesOwnTrackingRows fixture from the
integration-test setUp — slim has no non-deferred tracking rows for
DeployedIdx_1 / DeployedIdx_2 to seed.
- Update test call sites (SchemaChangeSequence / getSchemaChangeSequence)
to the simpler signatures.
Verified: 2743 core tests + 25 integration tests green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t DAO
The plan called for replacing the schema-scan with a DAO-driven approach,
but that has a timing bug: dao.findAll() queries the DB *before* the
visitor's tracking-row INSERT DML has executed, so any row tracked in
this session would be invisible to the DAO call.
The existing schema-scan + DeployedIndexState approach is already
slim-correct:
- Prior-session unbuilt deferred rows are virtualized into sourceSchema
by the enricher and marked ABSENT in state → final schema sees them,
`!= PRESENT` guard picks them up.
- This-session newly-declared deferred indexes land in the final schema
with UNKNOWN state → `!= PRESENT` picks them up.
- Visitor renames / column updates mutate the schema in place, so jobs
carry the current shape.
Rewritten the method's Javadoc to explain this explicitly (and why DAO
doesn't work), corrected the dialect-fallback comment to match slim
semantics ("tracks nothing" rather than "tracks as COMPLETED"), and
updated the DAO Javadoc to reflect the slim deferred-only invariant.
No behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under the slim invariant every DeployedIndexes row is a deferred index, so
the indexDeferred boolean is always true and adds no information. Drop:
- Column from `deployedIndexesTable()` and `CreateDeployedIndexes`.
- `indexDeferred` field + getter + setter on DeployedIndex POJO.
- `COL_INDEX_DEFERRED` constant on the statement factory interface.
- The literal value from `statementToTrackIndex` + the column from the
`selectAllColumns()` SELECT.
- `setIndexDeferred` call in DeployedIndexesDAOImpl.mapEntries.
- Stale "indexDeferred=false" Javadoc in AbstractSchemaChangeVisitor's
effectiveIndex — replaced with the slim semantics (no tracking row
under dialect fallback, so nothing for the app-side executor).
- `statementToTrackIndex` no longer branches on isDeferred() — always
emits PENDING (visitor only calls it for deferred indexes in slim).
Test impact: dropped non-deferred-expectations in TestDeployedIndex /
TestDeployedIndexesDAOImpl / TestDeployedIndexesModelEnricherImpl /
TestDeployedIndexesServiceImpl / TestDeployedIndexesStatementFactoryImpl;
dropped the `indexDeferred` column-field assertion from the
`testGetDeferredIndexStatementsReturnsSQL` integration test.
Verified: 2742 core tests + 25 integration tests green; full
mvn verify -DskipTests pipeline clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under the slim invariant every tracked row is deferred, so the two service methods were equivalent. isTracked() had no production caller (only tests); the visitor uses only isTrackedDeferred(). Collapsed into the single isTrackedDeferred() method with updated Javadoc explaining the equivalence. Tests migrated; one obsolete test (testTrackNonDeferredIndex, which exercised a path the slim visitor never takes) deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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
1 participant
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.
Summary
Slim redesign of the
DeployedIndexestracking table: rows exist only for deferred indexes. Non-deferred indexes live only in the physical DB and are policed by the framework's existingSchemaHomology.schemasMatchcomparison (same machinery that validates upgrade paths).The full-tracking alternative is #375. Same adopter API, same per-database semantics, same configuration — just a narrower scope and a lot less code.
Why this is simpler
UpgradeStep.executeoverload plus a supporting context interface so the seeding step could read pre-upgrade schema. Slim has nothing to seed → the whole side-API is deleted.PENDING→IN_PROGRESS→COMPLETED/FAILEDis the one state schema comparison can't see (a declared-deferred index legitimately isn't physical yet). Slim keeps all of it, including cross-upgrade carry-forward of unbuilt deferred indexes.Size
−574 net lines across 35 files vs. #375 (1 130 deletions, 556 insertions).
CreateDeployedIndexesstep: ~140 → ~85 linesUpgradeContextinterface: deletedPhase breakdown
isDeferred(); addsservice.prime()for cross-session stateUpgradeContext, 3-argexecute, prepopulation loopcollectDeferredIndexJobsstays schema-scan (DAO approach has a timing bug)indexDeferredcolumn as redundant under slim invariantTest plan
TestDeployedIndexesIntegration(25 tests) — deferred lifecycle on H2.DeployedIndexesModelEnricherImplagainst the full-model version (DeployedIndexes: unified tracking table for all indexes (full model) #375) side-by-side; verify every deleted check is somethingSchemaHomologyalready catches.getDeferredIndexStatements()after B returns SQL with the renamed column.🤖 Generated with Claude Code