Repository navigation
fix(reporting-storage): refuse a ledger upgraded to storage contract 2 with the real remedy #1857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
130 changes: 130 additions & 0 deletions
130
tests/conformance/reporting/test_reporting_storage_contract2_refusal_pg.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| """A ledger upgraded to storage contract 2 is refused clearly by this release (#1831). | ||
|
|
||
| adcp-reporting-storage 0.2.0b1+ upgrades a 0.1.x ledger in place: migration | ||
| 0002 adds ``offering_id`` and widens the configuration key and the obligation | ||
| period key with it. If a process of this release keeps running during a | ||
| rolling deploy, or restarts afterwards, it must name the actual problem and | ||
| remedy rather than fail with a raw driver error or point at the unrelated | ||
| caller-ownership migration. | ||
|
|
||
| ``_widen`` applies the same key changes as 0002 on ``next`` | ||
| (``reporting_ledger_account_generations.sql``). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from adcp.reporting.ledger import LedgerConflictError | ||
|
|
||
| from ._generation_support import configuration, isolated_reporting_pool, obligation_for | ||
|
|
||
| _WIDEN = """ | ||
| ALTER TABLE reporting_configurations ADD COLUMN offering_id TEXT COLLATE "C" NOT NULL DEFAULT ''; | ||
| ALTER TABLE reporting_configurations DROP CONSTRAINT reporting_configurations_pkey CASCADE; | ||
| ALTER TABLE reporting_configurations ADD CONSTRAINT reporting_configurations_pkey | ||
| PRIMARY KEY (account_id, consumer_id, delivery_config_id, delivery_config_version, offering_id); | ||
| ALTER TABLE reporting_obligations ADD COLUMN offering_id TEXT COLLATE "C" NOT NULL DEFAULT ''; | ||
| DROP INDEX reporting_obligations_period_key; | ||
| CREATE UNIQUE INDEX reporting_obligations_period_key | ||
| ON reporting_obligations | ||
| (account_id, consumer_id, delivery_config_id, delivery_config_version, offering_id, | ||
| period_start, period_end); | ||
| ALTER TABLE reporting_consumer_statuses ADD COLUMN offering_id TEXT COLLATE "C" NOT NULL DEFAULT ''; | ||
| """ | ||
|
|
||
|
|
||
| async def _upgraded_store(pool): | ||
| from adcp_reporting_storage.ledger.pg import PgReportingLedgerStore | ||
|
|
||
| store = PgReportingLedgerStore(pool=pool) | ||
| await store.create_schema() | ||
| await store.put_configuration(configuration("acct_a")) | ||
| async with pool.connection() as connection: | ||
| await connection.execute(_WIDEN) | ||
| return store | ||
|
|
||
|
|
||
| def _assert_remedy(message: str) -> None: | ||
| assert "storage contract 2" in message | ||
| assert "adcp>=10.0.0b7 with adcp-reporting-storage>=0.2.0b1" in message | ||
| assert "restore the backup" in message | ||
| assert "do not run migrate_legacy_reporting" in message | ||
|
|
||
|
|
||
| def test_only_a_missing_conflict_key_is_a_contract_2_refusal() -> None: | ||
| from adcp_reporting_storage.migration import schema_ahead_error | ||
|
|
||
| class _Error(Exception): | ||
| def __init__(self, sqlstate: str) -> None: | ||
| super().__init__(sqlstate) | ||
| self.sqlstate = sqlstate | ||
|
|
||
| assert schema_ahead_error(_Error("42P10")) is not None | ||
| assert schema_ahead_error(_Error("23505")) is None # unique violation | ||
| assert schema_ahead_error(ValueError("no sqlstate")) is None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_create_schema_refuses_an_upgraded_ledger_and_changes_nothing() -> None: | ||
| from adcp_reporting_storage import ReportingSchemaAheadError | ||
|
|
||
| async with isolated_reporting_pool(autocommit=True) as pool: | ||
| store = await _upgraded_store(pool) | ||
| async with pool.connection() as connection: | ||
| before = await ( | ||
| await connection.execute( | ||
| "SELECT pg_get_constraintdef(oid) FROM pg_constraint" | ||
| " WHERE conname = 'reporting_configurations_pkey'" | ||
| " AND connamespace = current_schema()::regnamespace" | ||
| ) | ||
| ).fetchall() | ||
| with pytest.raises(ReportingSchemaAheadError) as refused: | ||
| await store.create_schema() | ||
| _assert_remedy(str(refused.value)) | ||
| async with pool.connection() as connection: | ||
| after = await ( | ||
| await connection.execute( | ||
| "SELECT pg_get_constraintdef(oid) FROM pg_constraint" | ||
| " WHERE conname = 'reporting_configurations_pkey'" | ||
| " AND connamespace = current_schema()::regnamespace" | ||
| ) | ||
| ).fetchall() | ||
| assert after == before and "offering_id" in before[0][0] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_the_ddl_alone_names_contract_2_not_the_ownership_migration() -> None: | ||
| """For operators who apply the packaged SQL themselves.""" | ||
| from importlib.resources import files | ||
|
|
||
| ddl = ( | ||
| files("adcp_reporting_storage.ledger") | ||
| .joinpath("reporting_ledger_account_generations.sql") | ||
| .read_text() | ||
| ) | ||
| async with isolated_reporting_pool(autocommit=True) as pool: # skips without psycopg | ||
| from psycopg import errors | ||
|
|
||
| await _upgraded_store(pool) | ||
| async with pool.connection() as connection: | ||
| with pytest.raises(errors.RaiseException) as raised: | ||
| await connection.execute(ddl) | ||
| diagnostic = raised.value.diag | ||
| assert "storage contract 2" in (diagnostic.message_primary or "") | ||
| assert "Do not run migrate_legacy_reporting" in (diagnostic.message_hint or "") | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_a_running_process_names_the_upgrade_on_every_widened_write() -> None: | ||
| from adcp_reporting_storage import ReportingSchemaAheadError | ||
|
|
||
| async with isolated_reporting_pool(autocommit=True) as pool: | ||
| store = await _upgraded_store(pool) | ||
| with pytest.raises(ReportingSchemaAheadError) as refused: | ||
| await store.put_configuration(configuration("acct_b")) | ||
| _assert_remedy(str(refused.value)) | ||
| with pytest.raises(LedgerConflictError) as conflict: | ||
| await store.commit_obligation(obligation_for(configuration("acct_a"))) | ||
| assert conflict.value.code == "REPORTING_SCHEMA_AHEAD" | ||
| _assert_remedy(str(conflict.value)) |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium: This status-checkpoint refusal arm ships untested.
_WIDENin the new conformance test widensreporting_configurations,reporting_obligations, and adds a bareoffering_idcolumn toreporting_consumer_statuses— it never touchesreporting_status_scope_checkpoints, so theON CONFLICT ({_KEY})here never hits 42P10 in any test. The PR body lists the status-checkpoint write as one of the three sites that fail with a rawInvalidColumnReferenceon a contract-2 ledger, yet the only path this branch handles is the one with no coverage. If the sqlstate detection regresses here, an old pod degrades to a raw 42P10 on a durable async path with no test to catch it. Extend_WIDENto widen the checkpoint key and add a case exercising_lock_scopes_on.