Conversation
find_top_level_from() only checked the byte *after* a candidate FROM, so a SELECT column whose name ends in _from (e.g. rotated_from) matched the embedded FROM and truncated the column list. Require a word boundary on both sides — neither neighbouring byte may be an identifier char — which also fixes columns starting with from_ (the old next-byte == '_' check was inverted). Adds unit tests for the parser.
Documents the exact change needed in sea-query (src/value.rs) so a typed integer can be read from any integer Value variant via checked conversion, which is what lets sea-orm-spanner read i32 fields from INT64 columns. Includes verified macro, tests, tradeoffs, and the sea-orm-scoped alternative.
The lenient int_type_to_value! macro must name the trait explicitly (<$type as TryFrom<_>>::try_from) because ValueType is in scope inside sea-query's value.rs and also defines try_from — a bare call is E0034 ambiguous. Verified by compiling the patched macro against sea-query 1.0.1.
sea-orm's proxy read is strict: i32::try_from(Value::BigInt) fails, so a model field declared as i32 over a Spanner INT64 column could not be read (it surfaced as 'Missing value for column'). No single Value variant can satisfy both i32 and i64, and the orphan rule blocks fixing it in-crate. Fix it upstream via lenient integer coercion in sea-query, consumed here by: - bumping sea-orm rc.37 -> rc.41 (whose sea-query req relaxed to ~1.0.0) and sea-query rc.31 -> 1.0; MSRV is now 1.94 (already effectively 1.85). - a temporary [patch.crates-io] onto the sea-query PR branch (devgony/sea-query lenient-integer-value-type) until it is released. - implementing the new required QueryBuilder::prepare_explain_statement (no-op; Spanner doesn't EXPLAIN through this builder). all_types now declares int32_val/int32_nullable as i32, exercised by the existing type tests. Also relocates proxy unit tests to end-of-file to satisfy clippy::items_after_test_module on the newer toolchain. Remove the patch once the coercion ships in a released sea-query 1.0.x.
The examples pinned sea-orm to git tag 2.0.0-rc.32, which conflicted with the path crates now requiring rc.41. Switch to crates.io rc.41 and add the same temporary [patch.crates-io] sea-query branch so they build against the same lenient-integer sea-query as the library. Both examples compile on 1.94.
There was a problem hiding this comment.
Pull request overview
This PR fixes a SQL SELECT-column parsing bug in the Spanner proxy (where identifiers containing from could be mis-detected as the FROM clause), and updates the dependency stack to enable reading narrower Rust integer types (i32 etc.) from Spanner’s INT64 via a temporary patched sea-query.
Changes:
- Harden
find_top_level_from()with identifier-boundary checks and add regression tests insrc/proxy.rs. - Switch the
all_typestest entity and type tests to usei32forint32_*fields (exercising lenient integer coercion). - Bump SeaORM/SeaQuery versions, raise MSRV to 1.94, and add temporary
[patch.crates-io]entries to consume the upstreamsea-queryfix.
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/proxy.rs |
Fixes top-level FROM detection and adds regression unit tests for column parsing. |
tests/entity/all_types.rs |
Changes int32_* model fields to i32/Option<i32> to validate coercion behavior. |
tests/type_tests.rs |
Updates test fixtures/assertions to use i32 values for int32_* fields. |
sea-query-spanner/src/query_builder.rs |
Adds required prepare_explain_statement impl for updated sea-query API. |
sea-query-spanner/Cargo.toml |
Bumps MSRV and moves sea-query dependency from RC to 1.0. |
Cargo.toml |
Bumps MSRV and SeaORM/SeaQuery versions; adds temporary sea-query patch override. |
Cargo.lock |
Updates dependency resolution/lock format to match the new dependency set. |
README.md |
Updates docs to describe checked coercion for narrower integer fields. |
sea-query-int-coercion-contribution.md |
Adds write-up describing the upstream sea-query change and rationale. |
sea-orm-migration-spanner/Cargo.toml |
Bumps MSRV + SeaORM deps and applies temporary sea-query patch override. |
sea-orm-migration-spanner/Cargo.lock |
Updates lockfile for the migration workspace dependency changes. |
examples/basic-crud/Cargo.toml |
Bumps SeaORM dependency and applies temporary sea-query patch override. |
examples/basic-crud/Cargo.lock |
Updates example lockfile for the new dependency graph. |
examples/migration/Cargo.toml |
Switches from git-tag SeaORM to crates.io RC and applies temporary sea-query patch override. |
examples/migration/Cargo.lock |
Updates example migration lockfile for the new dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
prepare_explain_statement was a no-op that would emit empty/invalid SQL if ever called. Panic with unimplemented!() so misuse fails loudly instead of producing confusing runtime errors.
Replace the moving 'lenient-integer-value-type' branch reference with the exact commit (dca2f6a) already captured in Cargo.lock so builds stay reproducible even without --locked.
Match the root workspace by pinning to commit dca2f6a instead of the moving branch, keeping the example build reproducible.
Match the root workspace by pinning to commit dca2f6a instead of the moving branch, keeping the example build reproducible.
This separate workspace also tracked the moving branch. Pin to commit dca2f6a (matching the lockfile) for reproducible builds.
Cargo applies [patch.crates-io] only from the top-level workspace, so crates.io consumers of sea-orm-spanner must add the patch themselves. Document the exact rev-pinned patch entry to copy.
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
Fixes two issues and prepares the dependency stack for the upstream fix that makes the first one possible.
rotated_fromSELECT-column parser bug (src/proxy.rs):find_top_level_from()only checked the byte after a candidateFROM, so a column whose name ends in_from(e.g.rotated_from) matched the embeddedFROMand truncated the column list. It now requires a word boundary on both sides (neither neighbouring byte may be an identifier char), which also fixes columns starting withfrom_(the old next-byte== '_'check was inverted). Added unit tests.i32over anINT64column failed read coercion: sea-orm's proxy read is strict —i32::try_from(Value::BigInt)fails — so a model field declaredi32over a SpannerINT64column surfaced as"Missing value for column". No singleValuevariant satisfies bothi32andi64, and the orphan rule blocks fixing it in-crate. Resolved via lenient integer coercion in sea-query, consumed here by:sea-ormrc.37 → rc.41 (whose sea-query requirement relaxed to~1.0.0) andsea-queryrc.31 →1.0; MSRV is now 1.94 (already effectively 1.85 via rc.37).[patch.crates-io]onto the sea-query PR branch (devgony/sea-querylenient-integer-value-type) until it is released. Documented for removal in every patchedCargo.toml.QueryBuilder::prepare_explain_statement(no-op; Spanner doesn'tEXPLAINthrough this builder).all_typestest entity now declaresint32_val/int32_nullableasi32, exercised by the existing type tests.Bumped both example crates and the migration crate onto rc.41 + the same patch so the whole repo is consistent.
Added
sea-query-int-coercion-contribution.mddocumenting the exact upstream change (verified macro + tests + tradeoffs).Test plan
Run against the Spanner emulator with Rust 1.94 (new MSRV):
SPANNER_EMULATOR_HOST=localhost:9010 cargo +1.94 test --all-features— main suite 88 pass (incl. 5 new parser unit tests and i32 type tests)cargo +1.94 test --all-features— 22 passcargo +1.94 clippy --all-features --all-targets -- -D warnings— clean (both workspaces)cargo +nightly fmt --check— cleanbasic-crud,migration) build on 1.94