Skip Values construct in FROM clause instead of raising NotImplementedError - #43
Merged
Conversation
…dError A sqlalchemy.values() VALUES (...) row set appearing as a top-level FROM entry has no underlying table to soft-delete-filter, so analyze_from() now returns the statement unchanged for it, exactly like the TableClause/TextClause branch. Previously it fell through to the final raise. This also fixes the common indirect trigger: an ORM bulk UPDATE referencing a VALUES emits a synchronize-session SELECT carrying that VALUES in its FROM, which tripped the rewriter on the spawned SELECT. Closes #42
Rewrite the three regression tests to actually execute the statements (as the rest of the suite does against PostgreSQL in CI) and assert real behavior: the VALUES SELECT returns its rows, the ORM UPDATE takes effect, and a real table alongside a top-level VALUES is still soft-delete filtered. The statements skip on SQLite (which only accepts VALUES inside a CTE), mirroring the existing test_insert_with_returning skip pattern.
On SQLAlchemy 1.4 the default synchronize strategy evaluates the WHERE criteria in Python, which cannot evaluate a VALUES column reference and raises UnevaluatableError before any SQL is emitted -- a SQLAlchemy limitation unrelated to the rewriter. Forcing 'fetch' reconciles the identity map via a spawned SELECT, which is exactly the indirect code path this issue is about, and works uniformly on 1.4 and 2.0.
The root CHANGELOG.md was abandoned (last entry 0.2.1 while releases are at v0.9.0) and not maintained per-PR; release notes live in GitHub Releases. Also remove docs/changelog.md, which only existed to include the root file.
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.
Summary
SoftDeleteQueryRewriter.analyze_from()raisedNotImplementedErrorwhenever a rewrittenSELECThad a SQLAlchemyValuesconstruct (sqlalchemy.values()) as a top-level entry in its FROM clause. AVALUES (...)row set has no underlying table, so there is nothing to soft-delete-filter — it should be skipped (returned unchanged), exactly like the existingTableClause/TextClausebranch.This fix adds a
Valuesbranch toanalyze_from()that returns the statement unchanged.Why it matters
Two ways to hit this:
SELECTwhose FROM contains a top-levelValues.UPDATE(update(Model)…) referencing aValuesin itsWHERE/SET. The library only rewrites SELECTs, but the ORM emits a synchronize-session SELECT to reconcile the identity map, and that spawned SELECT carries theValuesin its FROM — so the rewriter tripped on it. TheUPDATE t SET col = v.x FROM (VALUES …) AS v WHERE t.id = v.idpattern is a common single-round-trip bulk-update idiom.Changes
analyze_from()now returns the statement unchanged for aValuesFROM entry (noNotImplementedError).tests/default_config/test_queries.pythat execute against the database (PostgreSQL in CI) and assert real behavior:test_select_with_top_level_values_from_is_not_rewritten— a top-levelValuesSELECT runs and returns its rows.test_orm_update_referencing_values_does_not_trip_rewriter— the ORMUPDATE-with-VALUESexecutes and the update takes effect.test_table_alongside_values_is_still_soft_delete_filtered— a soft-deletable table alongside a top-levelValuesstill excludes soft-deleted rows (deleted_at IS NULL) while theValuesrows pass through.All three fail on
NotImplementedErrorwithout the fix and pass with it (verified against PostgreSQL). They skip on SQLite — which only acceptsVALUESinside a CTE, not as a top-level FROM — mirroring the existingtest_insert_with_returningskip pattern, so the quickmake test(SQLite) stays green.Test plan
make test-pg(PostgreSQL) → 73 passeduv run pytest(SQLite quick) → 3 new tests skip cleanly, rest passruff check/ruff format --checkcleanmypycleanCloses #42