fix(oracle): let a schema object register more than one introspection query - #476
Merged
Merged
Conversation
… query
On Oracle the migration path could only see a table's columns. ODP.NET will not
execute several statements from a single command, so ConfigureQueryCommand got one
query and Table spent it on columns -- leaving indexes, foreign keys and the
primary key invisible to SchemaMigration.DetermineAsync, which is what
ApplyChangesAsync, ApplyAllConfiguredChangesToDatabaseAsync and
AssertDatabaseMatchesConfigurationAsync all go through.
The practical effect: a declared index was created with the table and never
touched again. Add one to an existing table and nothing happened. Change one and
nothing happened. Remove one and it stayed. And db-assert reported a clean match
throughout.
OracleDbCommandBuilder already knew how to split a batch into one command per
StartNewCommand boundary -- that machinery went in for the batching work and
nothing used it here. What was missing is the other half: something to read across
the split. MultiCommandDataReader presents an ordered list of commands as one
continuous sequence of result sets, so NextResultAsync rolls over into the next
command and the code consuming the results cannot tell the difference.
The seams:
- IBatchedCommandBuilder, deliberately separate from ICommandBuilder<T> so that
adding it breaks nothing implementing that interface outside this repo.
- Migrator.CreateCommandBuilder, defaulting to the dialect-neutral builder.
Oracle is the only override.
- A SchemaMigration.DetermineAsync overload taking the builder, with
StartNewCommand between objects so a splitting builder gets a boundary there
too.
Everything except Oracle returns a single command from CompileCommands and
executes exactly as before.
Oracle's Table now registers all six queries it needs, and its introspection is
restructured so the batched path and FetchExistingAsync share the same SQL
constants and the same readers rather than having two copies that can drift.
InitialLONGFetchSize is set on both, because all_ind_expressions.column_expression
is a LONG and reads back empty without it -- the same trap weasel#450 hit.
Two workarounds go away with the cause. DetermineOracleMigrationAsync sniffed for
Tables.Table and rerouted it to FindDeltaAsync; it is now a one-line delegation
with no type test. And the Oracle rows of the index scenario matrix no longer
override how a delta is found -- they run the ordinary path, like every other
provider's.
Found by that matrix in weasel#449: Oracle failed eight of its eleven scenarios
where the other four passed, and querying all_indexes by hand showed the index had
been there the whole time.
Closes #474.
Co-Authored-By: Claude Opus 5 <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
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.
Closes #474.
The bug
On Oracle, the migration path could only see a table's columns.
ODP.NET will not execute several statements from a single command, so
ConfigureQueryCommandgot one query — andTablespent it on columns. Indexes, foreign keys and the primary key were invisible toSchemaMigration.DetermineAsync, which is whatApplyChangesAsync,ApplyAllConfiguredChangesToDatabaseAsyncandAssertDatabaseMatchesConfigurationAsyncall go through.What that meant in practice:
db-assertreports a clean match throughout.The fix
OracleDbCommandBuilderalready knew how to split a batch into one command perStartNewCommand()boundary — that machinery went in for the batching work and nothing used it here. What was missing was the other half: something to read across the split.MultiCommandDataReaderpresents an ordered list of commands as one continuous sequence of result sets.NextResultAsyncwalks the current command's result sets and then rolls over into the next command's — and since a freshly executed reader sits exactly whereNextResult()leaves a caller, the rollover is indistinguishable from an ordinary result-set boundary. Nothing consuming the results has to know.Three small seams carry it:
IBatchedCommandBuilderICommandBuilder<T>, so adding it breaks nothing that implements that interface outside this repoMigrator.CreateCommandBuilderSchemaMigration.DetermineAsync(conn, builder, ct, objects)StartNewCommand()between objects — not before the first, or a splitting builder opens with an empty statementEvery provider except Oracle returns a single command from
CompileCommands()and executes exactly as before.One trap worth naming: the single-command case cannot fall through to
Compile(). A splitting builder consumes its accumulated SQL while compiling, so a secondCompile()hands back a command with emptyCommandText— which is ORA-50029, and how I found it.Oracle's introspection is now one implementation
Tableregisters all six queries it needs (columns, PK, FKs, index metadata, index expressions, index columns), and the batched path andFetchExistingAsyncnow share the same SQL constants and the same readers rather than being two copies that can drift apart.InitialLONGFetchSizeis set on both —all_ind_expressions.column_expressionis a LONG and reads back empty without it, the same trap #450 hit.Two workarounds go away with the cause
DetermineOracleMigrationAsyncsniffed forTables.Tableand rerouted it toFindDeltaAsync, because the generic path would have missed everything. It is now a one-line delegation with no type test — and it batches the objects instead of looping one at a time.Tests
Weasel.Oracle.Tests/Tables/migration_path_sees_more_than_columns.cs— five tests going throughApplyChangesAsyncandDetermineAsyncdeliberately, since the point is the path;FetchExistingAsynccould always see all of this. Includingseveral_tables_in_one_batch_each_read_their_own_result_sets, because the reader now has to walk six result sets per table and land on the right boundary or the second table reads the first one's rows.The regression proof is #449's matrix: with the override removed and before this fix, Oracle failed 8 of its 11 scenarios with "missing 1", while querying
all_indexesby hand showed the index had been there the whole time. All 11 pass now through the ordinary path.Verified locally
Core 216, SQLite 418, PostgreSQL 879, SQL Server 440, Oracle 259, MySQL 277, EF Core 106. No failures.
🤖 Generated with Claude Code