fix: address high and medium priority code review issues

[devin-ai-integration]

fix: address high and medium priority code review issues

Summary

Fixes 6 issues identified during code review, covering bug fixes, safety improvements, and code deduplication:

1. Bug fix: find_top_level_from underscore handling — The condition || bytes[next_idx] == b'_' incorrectly matched FROM_TABLE as a SQL FROM keyword. Fixed to && bytes[next_idx] != b'_' so underscored identifiers are no longer misrecognized.

2. Bug fix: rewrite_mysql_quotes string literal awareness — The old sql.replace('', "")stripped backticks inside string literals, corrupting data values. Replaced with a state-machine parser (same pattern asrewrite_placeholders`) that only removes backticks outside string literals.

3. Safety: BigUnsigned → i64 overflow check — Previously *v as i64 silently overflowed for u64 values > i64::MAX. Now uses i64::try_from() and returns SpannerDbErr::TypeConversion. Applied in proxy.rs, executor.rs, transaction.rs, and the new shared bind.rs.

4. Deduplication: shared bind.rs module — Extracted identical bind_value / convert_statement logic from executor.rs and transaction.rs (3 copies totaling ~500 lines) into a single src/bind.rs module. Net -123 lines. Note: proxy.rs retains its own bind_value because it uses different Spanner wrapper types (e.g., SpannerNaiveDateTime, SpannerUuid).

5. Observability: begin/commit/rollback warnings — These are no-ops since Spanner uses callback-based transactions. Added tracing::warn! messages explaining the correct transaction API to use, since the upstream trait signature (async fn begin(&self)) doesn't allow returning errors.

6. Strictness: bind_array error handling — Replaced filter_map (which silently dropped None and type-mismatched elements) with fallible map that returns SpannerDbErr::TypeConversion with the element index. Also added BigUnsigned overflow checking inside arrays.

Updates since last revision

• Merged main into the branch (v0.1.0 release changes)
• Registered orphaned modules in lib.rs: connection, executor, query_result, and transaction existed as source files but were never declared as modules, which caused bind.rs functions to appear as dead code (the original CI lint failure). All four are now declared as pub mod.
• Updated to current gcloud-spanner API: The orphaned modules used a stale API. Key fixes:
  • read_write_transaction closure now takes 1 argument (no CancellationToken), matching current gcloud-spanner 1.8.0 API
  • connection.rs::close() changed from &self to self to handle Arc<Client> ownership via Arc::try_unwrap
  • Removed obsolete gcloud_gax::cancel::CancellationToken import
• make fmt applied across all changed files
• cargo clippy --all-features -- -D warnings now passes cleanly

Review & Testing Checklist for Human

• Verify close(self) panic safety: SpannerConnection::close() now uses Arc::try_unwrap(...).expect(...) which panics at runtime if other Arc<Client> references exist. Verify all call sites ensure the connection is the sole owner before calling close(), or consider changing to a graceful error return.

• Verify bind_array behavior change is acceptable: The switch from silently dropping invalid/null array elements to returning errors is a breaking behavioral change. If any existing callers rely on the old filter-and-drop behavior (e.g., passing arrays with None elements), they will now get errors. Check existing usage patterns.

• Review find_top_level_from logic carefully: The condition was inverted from || bytes[next_idx] == b'_' to && bytes[next_idx] != b'_'. Verify the original bug claim is correct (that FROM_TABLE was incorrectly matching as a FROM keyword).

• Test rewrite_mysql_quotes with edge cases: The new state machine should preserve backticks inside string literals (e.g., SELECT 'foo`bar' FROM `table`). Test with escaped quotes ('', "") and mixed quote types.

• Verify public API surface of newly-exported modules: connection, executor, query_result, and transaction are now pub mod. Confirm that exposing SpannerConnection, SpannerExecutor, SpannerReadWriteTransaction, SpannerReadOnlyTransaction, and SpannerQueryResult as public types is intentional and the API is stable enough.

Test Plan

1. Run cargo clippy --all-features -- -D warnings (already passing in CI)
2. Run the existing test suite with the Spanner emulator (cargo test)
3. Add specific tests for:
   • find_top_level_from("SELECT * FROM_TABLE") should NOT match
   • rewrite_mysql_quotes("SELECT 'foo`bar' FROM `table`") should preserve the backtick inside the string
   • bind_value with BigUnsigned(u64::MAX) should return an error
   • bind_array with [Some(1), None, Some(2)] should return an error (not [1, 2])
   • SpannerConnection::close() when other Arc references exist (should it panic or error?)

Notes

• The bind.rs module uses SpannerTimestamp/SpannerOptionalTimestamp wrapper types for chrono conversion, while the old inline code used bare chrono::DateTime<Utc>. This is a pre-existing architectural difference between the proxy path and the executor/transaction path.
• Tests require the Spanner emulator and fail locally without it; CI provides the emulator environment.

Link to Devin session: https://app.devin.ai/sessions/0efd4aabb4604cf18b3663ff8daefd67
Requested by: @devgony

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR that start with 'DevinAI' or '@devin'.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

[Copilot]

Pull request overview

This PR refines the SeaORM ↔ Google Cloud Spanner adapter by fixing SQL parsing/binding bugs, improving type-safety for parameter conversion, deduplicating binding logic, and updating code to the current gcloud-spanner transaction API.

Changes:

• Fixes SQL parsing/rewriting edge cases (e.g., FROM_TABLE detection and backticks inside string literals).
• Adds overflow-safe u64→i64 conversion for BigUnsigned (including inside arrays) and makes bind_array fail fast on invalid elements.
• Deduplicates executor/transaction binding into a shared src/bind.rs module and registers previously-orphaned modules in lib.rs.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/bind.rs	New shared statement conversion + value binding logic (dedup + overflow checks).
src/connection.rs	Updates close semantics and adapts read_write_transaction callback signature to newer gcloud-spanner.
src/executor.rs	Switches to shared binder and updates transaction closure signature.
src/transaction.rs	Switches to shared binder and removes duplicated bind logic.
src/proxy.rs	Makes MySQL backtick rewrite string-literal-aware; improves array binding strictness; fixes FROM detection; adds begin/commit/rollback warnings.
src/lib.rs	Declares modules (incl. new bind) and publicly exports connection/executor/query_result/transaction modules.
src/query_result.rs	Import formatting-only change.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SpannerConnection::close(self) will panic at runtime if the Arc<Client> has been cloned anywhere else (Arc::try_unwrap(...).expect(...)). Since SpannerConnection is Clone and now publicly exported, this is easy to trigger unintentionally and would crash consumer applications. Consider returning a Result<(), DbErr> (or logging and returning) when try_unwrap fails instead of panicking, or provide a separate infallible close_if_unique(self) helper if you want to preserve strict behavior.

Suggested change

	pub async fn close(self) {
	Arc::try_unwrap(self.client)
	.ok()
	.expect("Cannot close: other references to Client exist")
	.close()
	.await;
	pub async fn close(self) -> Result<(), DbErr> {
	match Arc::try_unwrap(self.client) {
	Ok(client) => {
	client.close().await;
	Ok(())
	}
	Err(_) => Err(DbErr::Custom(
	"Cannot close: other references to Client exist".to_owned(),
	)),
	}

[Copilot]

begin() logs at warn! level despite being called through ProxyDatabaseTrait, which can be part of normal control flow if callers use SeaORM’s transaction APIs. This can create very noisy logs in production. Consider lowering to debug!/info!, emitting the warning only once (e.g., via a OnceLock), or gating it behind a feature flag so users can opt in to the observability.