fix(spanner): make types module public for typed param construction#5912
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the visibility of the types module in google-cloud-spanner from pub(crate) to pub and adds integration tests to verify that its public constructors are accessible externally. The reviewer recommends adding a doc comment to the newly public types module to comply with the repository's style guide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request changes the visibility of the types module in the google-cloud-spanner crate from internal (pub(crate)) to public (pub), exposing Spanner primitive type constructors for typed parameter binding. Additionally, it introduces a new integration test suite types_module_visibility.rs to verify the public accessibility of these constructors and prevent future visibility regressions. There are no review comments, so I have no feedback to provide.
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5912 +/- ##
=======================================
Coverage 97.90% 97.90%
=======================================
Files 233 233
Lines 59202 59202
=======================================
Hits 57962 57962
Misses 1240 1240 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ding The `google_cloud_spanner::types` module was declared `pub(crate)` and lived in the `// Internal modules` section of `lib.rs`, so the documented primitive constructors (`int64()`, `string()`, `timestamp()`, `bool()`, `bytes()`, `date()`, `float32()`, `float64()`, `json()`, `numeric()`, `uuid()`, `interval()`, plus the PostgreSQL variants `pg_numeric()`, `pg_jsonb()`, `pg_oid()`) were unreachable from external crates. Downstream code calling `StatementBuilder::add_typed_param` was forced to construct values via the generated `google_cloud_spanner::model::Type` protobuf struct and field mutation, coupling external code to a `#[non_exhaustive]` generated type. The module was the only thing wrong: every constructor is already `pub fn` inside the module, already documented at its declaration site in `src/spanner/src/types.rs`, and already used internally by the SDK's own tests at `src/spanner/src/statement.rs:390-394` via `crate::types::string()`. This was a classic Rust visibility gotcha where items declared `pub` were hidden by a `pub(crate)` module, made worse by the module being filed in the `// Internal modules` block of `lib.rs` even though its contents were part of the documented public surface. Moved `pub(crate) mod types` out of the internal block and re-declared it as `pub mod types` in the `// Public domain modules` block, next to its semantic peer `value` (which re-exports the type and traits but never the constructors). Added a regression test in `tests/types_module_visibility.rs` that exercises the constructors from an external-crate perspective, so any future regression that re-hides the module fails compilation with the same `E0603` error downstream users reported. Towards googleapis#5911
@olavloite can you re-trigger this? Thanks. |
|
/gcbrun |
3 participants
The
google_cloud_spanner::typesmodule was declaredpub(crate), so thedocumented primitive constructors (
int64(),string(),timestamp(),bool(),bytes(),date(),float32(),float64(),json(),numeric(),uuid(),interval(), plus the PostgreSQL variantspg_numeric(),pg_jsonb(),pg_oid()) were unreachable from external crates. Downstream code callingStatementBuilder::add_typed_paramwas forced to construct values via the generatedgoogle_cloud_spanner::model::Typeprotobuf struct and field mutation, coupling external code to a
#[non_exhaustive]generated type.The module visibility was the only thing wrong: every constructor is already
pub fninside the module, already documented at its declaration site insrc/spanner/src/types.rs, and already used internally by the SDK's own tests atsrc/spanner/src/statement.rs:390-394viacrate::types::string().This was a classic Rust visibility gotcha where items declared
pubwerehidden by a
pub(crate)module.Changed
pub(crate) mod typestopub mod typesatsrc/spanner/src/lib.rs:92.Added an integration test (
tests/types_module_visibility.rs) that exercisesthe constructors from an external-crate perspective, so any future regression
that re-hides the module fails compilation with the same
E0603error downstream users reported.Fixes #5911