Repository navigation
fix: refuse bytes against a text column on Postgres (#785) - #834
Conversation
…mn on Postgres (#785) The per-table catalog records each column's live storage class; writes and fetches that cross the text/bytes boundary raise DataError before sending. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ext-column-refusal
0x054
left a comment
There was a problem hiding this comment.
Spec: FAIL
- Important · [ticket#Acceptance criteria] ·
src/operations.rs:3675at6a46b66· Missing · The fetch check only looks at projected columns whosepathis empty. A projection that reaches the mismatched column through a relation is not checked and still corrupts the value without an error. Reproduced on Postgres 18 at this head:Vault.secretdeclaredSecretBytes, livevarcharholding'\xff006b', andKey(vault FK).await Key.select(lambda key: key.vault.secret).all()returnsRow(secret=SecretBytes(...))whoseget_secret_value()isb'\\xff006b', whileVault.get(1)refuses. The ticket says "a fetch whose selection includes such a column (... a projection of it, a hop that loads it)" must refuse. The PR body lists this as a limit, but the ticket gives no Narrows line for it. Fix: for aColumnwith a non-emptypath, resolve the table throughjoin_plan.prefix_table, then check it againstplan.hop_registrationsand that table's cached catalog.populate_hop_bind_contexthas already loaded that catalog, so this adds no round trip. Add a test for it.
Cannot verify: none
Accepted as declared: src/errors.rs (data_error) and src/state.rs (model_label_for_table) fall outside Files owned, and both are needed for the message. TableCatalog.storage_type is an extra field, needed for the live type name in the text. StorageClass::Other exists but is never stored, because a missing key reads as Other, which is equivalent for every reader. .sql() of a refused fetch also refuses, because it shares build_fetch and so stays consistent with the rendered-statement parity pin. The DataError text matches the ticket's Interfaces line exactly (test_save_refuses_and_leaves_the_table_unchanged checks it with ==). The hop catalog read at src/operations.rs:3683 is a cache hit, since populate_hop_bind_context (:409) already loaded every join-plan table. tests/test_catalog_cache.py passes and the new happy-path count assert holds.
Quality: FAIL
- Important · [AGENTS.md#I-6] ·
src/operations.rs:3675at6a46b66· The PR body's Laws line claims "I-6: refuses loudly ... no warning/skip path", but it also documents "a projection that traverses a relation ... not checked" as a known limit. That limit is a silent-corruption path (see the Spec finding), which is the "documented residual risk" I-6 forbids. The fix is the same as in the Spec finding.
Verify: PR head 6a46b66 passed (pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgres: 36 passed, all 8 Postgres-marked new tests ran on Postgres 18; cargo test -q --no-default-features --features testing catalog passed). The ticket's cargo test -q -p ferro-orm catalog fails with "package ID specification did not match", so the PR's Verify block corrected: yes is accurate.
Trial merge: not run (base unchanged; the head already contains origin/main cf1ee43f)
…rage class (#785) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
0x054
left a comment
There was a problem hiding this comment.
Re-review: 6a46b66..3ae15e8
- Spec finding (relation-path projection unchecked,
src/operations.rs:3675): ADDRESSED. Atsrc/operations.rs:3671-3696at3ae15e8, a projected column with a non-empty path is resolved throughjoin_plan.prefix_tableandplan.hop_registrationsand checked against the hop table's cached catalog. My Postgres 18 reproduction (Key.select(lambda key: key.vault.secret).all()over avarcharhop column) now raisesDataError: Vault.secret is declared bytea but the live column is varchar; .... The newtest_projection_through_a_relation_checks_the_hop_column[varchar|bytea]covers both the refusal and the matching case. - Quality finding (I-6 Laws claim contradicted by the documented limit): ADDRESSED. The relation-projection limit is gone from the code and from the PR body, so the I-6 line now holds.
No new Critical or Important problems in the fix diff. The hop catalog read at :3694 hits the cache, because populate_hop_bind_context loaded every join-plan table, and tests/test_catalog_cache.py still passes.
Spec: PASS
Cannot verify: none
Quality: PASS
Verify: PR head 3ae15e8 passed (pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgres: 38 passed, 10 new-file tests on Postgres 18; cargo test -q --no-default-features --features testing catalog passed)
Trial merge: not run (base unchanged; merge base with origin/main is cf1ee43f)
Closes #785
What
On Postgres, an operation that would move bytes into a text column (or text into a
byteacolumn) now raisesDataErrorbefore any statement is sent, instead of storing hex text. The per-table catalog records each column's live storage class from its existing probe (typcategory), so matching columns cost no new round trip. Coverssave,bulk_create, instance update, queryupdate(), and fetches that select the column (root, projection of it,include()hops). SQLite is unaffected. The 0.26 upgrade note describes the refusal.Verify
Ran the ticket's Verify block on
3ae15e8(merged with origin/main cf1ee43):pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgrespassed (Postgres 18);cargo test -q --no-default-features --features testing catalogpassed. Full suite before the merge: 6435 passed, 1 failed (test_postponed_annotations, fixed on main by #831, passes after the merge).TDD: the test file was written alongside the implementation, not run red first (the red state is the issue's reported corruption); GREEN 9/9 new tests on sqlite+postgres.
Verify block corrected: yes:
cargo test -q -p ferro-orm catalogdoes not link; rancargo test -q --no-default-features --features testing catalog(the CI form).Interfaces
Interfaces changed: none to the ticket's Produces;
TableCatalogalso gainsstorage_type: HashMap<String, String>(live type name, for the message).StorageClass::Othercolumns are absent from the map rather than stored.Laws
Notes for the reviewer
Outside Files owned:
src/errors.rs(data_errorconstructor, there is no Python-side change soexceptions.pyis untouched) andsrc/state.rs(model_label_for_table, for the model name in the message). A projection through a relation (key.vault.secret) is checked against the hop table catalog; aggregates are not checked. The refusal also applies to.sql()rendering of a fetch since it sharesbuild_fetch.🤖 Generated with Claude Code