Answers what schema version this package expects - #85
Merged
Conversation
A host that delegates its migrations here never needs the number: `Ecto.Migrator` records the migration and `up/1`'s default target carries it to the newest version. A host whose schema is hand-written DDL has to check for itself, and had nothing to check against. `Migrations.expected_version/0` is that number, the newest key of the version map. There is no `assert_version!/1` beside it: this package records no version marker in a repo's schema - no versions table, no marker row, no column - and Ecto's own `schema_migrations` holds the host's migration timestamps, which say nothing about which of V01..V05 a hand-written schema matches. Growing one means writing a marker every host carries, including the ones that delegate and already know, for a comparison the host can make itself. So the package answers the half it knows and the docs say why the other half is not here. ADR-0010 takes a note on the second question: whether a host needs V05's table at all. An adapter that does not export the optional input-log callbacks never touches it - above the seam `Storage.append_input/4` answers `:not_supported` without calling the adapter, and below it only `append_input/3` and `list_inputs/2` name the table - so such a host caps its migration at V04 in both directions rather than carrying an empty table. `Storage.Ecto` is the other case: it declares support unconditionally, so its hosts need V05 whether or not they ever replay. Refs: sp-b0g
The pass-1 direction review on PR 85 found the second assertion in each new case tautological: `expected_version/0` returned `@current_version` and `validate_version!/2` bounds on the same attribute, so `up(version: expected_version() + 1)` raised whatever the migration map held, and the comment claiming it moved with the map was false. Cured at the root. `@current_version` is now read off `@migrations` (`Map.keys |> Enum.max`) rather than written beside it, so the attribute cannot drift from the map it is meant to summarize and `expected_version/0` is literally the newest key, as the bead asks. The redundant assertion is dropped from both cases and the comments say what actually moves them red: a V06 entry moves the function to 6 and the literal 5 fails. Sabotage re-run after the change (expected_version/0 pointed at `@initial_version`): both cases red, "45 tests, 2 failures", reverted from a copy, full gate green after. Refs: sp-b0g
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.
What
Two remainders of Theme 7, from
sp-b0g.StatifierPersistence.Ecto.Migrations.expected_version/0- the newestmigration version this package knows, for a host whose schema is
hand-written DDL rather than a delegated migration and which therefore
has to check for itself that its tables are current.
does not implement the optional input-log callbacks needs V05's table at
all.
Why there is no
assert_version!/1The bead allows either the function or the documented reason it cannot
exist. It cannot: this package records no version marker in a repo's
schema - no versions table, no marker row, no version column (
grepoverlib/finds the stringexpected_versionnowhere before this branch and nomarker of any other spelling). Ecto's own
schema_migrationsholds thehost's migration timestamps, which say nothing about which of V01..V05 a
hand-written schema matches. Growing such an assertion means starting to
write a marker - a new table, in a new migration, carried by every host
including the ones that delegate and already know - which is new stored
surface bought for a comparison the host can make itself. So
expected_version/0is the whole deliverable, and its@docsays why.The Note's answer, from the code
and its host does not need V05. Above the seam there is one write site
(
Runs.stepped/6viaStorage.append_input/4, which answers:not_supportedwithout calling the adapter whenStorage.input_log_supported?/1is false) and one read site(
Runs.inputs/2viaStorage.list_inputs/2). Below it,Storage.Ectoqueries the inputs table only in
append_input/3andlist_inputs/2-their private helpers
next_slot/2,insert_input/5andinput_rows/2are called from those two functions and nowhere else
(
lib/statifier_persistence/storage/ecto.ex:494,:499,:502,:520).Everywhere else the table is a name, not a query:
Ecto.Config's@table_keys,KeyGenerator.table/0, the:uxidprefix map, and thegenerated
Inputschema. Such a host caps at V04 in both directions withthe moduledoc's capped recipe rather than carrying an empty table.
Storage.Ectoneeds V05 unconditionally:supports_input_log?/1answerstruewithout probing, so the firststepped event appends and a failed append fails the step.
input_log_cap:is no opt-out either - the cap's last slot is still a written row. The
Note names that as the trigger, and the fix if it fires (a declaration at
init/1), without reopening the decision.ADR-0010 is already accepted, so the Note is amend-by-addition:
git diff origin/main -- docs/adr/is 36 added, 0 removed.Tests
test/statifier_persistence/ecto/migrations_test.exs-expected_version/0is 5. It moves with the map because
@current_versionis derived from@migrations(Map.keys |> Enum.max): a V06 entry moves the function to 6and the literal fails.
test/statifier_persistence/ecto/sqlite_migrations_test.exs- the sameassertion off Postgres, because the number is backend-independent by
construction (it is derived from the migration map and reads no repo).
expected_version/0pointed at@initial_version): red,those two cases alone, "45 tests, 2 failures"; reverted from a copy taken
beforehand, green after.
The pass-1 direction review caught the first shape of these cases: the
original second assertion (
up(version: expected_version() + 1)raises) wastautological, since
validate_version!/2bounds on the same attribute thefunction returned. The cure commit derives
@current_versionfrom the map,which is what makes "the newest key of the version map" true of the code and
not only of the docs, and drops the redundant assertion.
Provenance
migrations.ex's moduledocgains one paragraph pointing at the new function - the bead allows the
moduledoc as a home for the Note, so a pointer there is inside its scope.
docs/adr/0006,docs/adr/0009and the ADRREADME index; this branch touches none of them.
Gate
Full
mix qualitygreen on this HEAD: 482 of 482 tests, 95.7% coverage,dialyzer and credo clean;
mix quality.verifyattested. Sabotage scanreports no missing notes.
Refs: sp-b0g