-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH]: Database and Tenant Crud enhancements #6119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH]: Database and Tenant Crud enhancements #6119
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
It also consolidates the Spanner tenant and database CRUD flow into a single transactional path with richer validation and error handling, broadens the SysDb error surface, aligns the shared request and response types with chroma_types exports, and refreshes the accompanying tests to cover the new behavior. Affected Areas• This summary was automatically generated by @propel-code-bot |
rust/spanner-migrations/migrations/0003-create_databases_unique_index.spanner.sql
Outdated
Show resolved
Hide resolved
| .map_err(SysDbError::FailedToReadColumn)?; | ||
|
|
||
| // resource_name can be NULL, so we handle the error as None | ||
| let resource_name: Option<String> = row.column_by_name("resource_name").ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Logic] Using .ok() here swallows all errors, including schema mismatches (e.g., if the resource_name column is missing from the query result).
Since resource_name is nullable, requesting it as Option<String> will correctly handle SQL NULLs by returning Ok(None), while still propagating errors for missing columns or type mismatches.
| let resource_name: Option<String> = row.column_by_name("resource_name").ok(); | |
| // resource_name can be NULL, so we handle the error as None | |
| let resource_name: Option<String> = row | |
| .column_by_name("resource_name") | |
| .map_err(SysDbError::FailedToReadColumn)?; |
Context for Agents
Using `.ok()` here swallows all errors, including schema mismatches (e.g., if the `resource_name` column is missing from the query result).
Since `resource_name` is nullable, requesting it as `Option<String>` will correctly handle SQL NULLs by returning `Ok(None)`, while still propagating errors for missing columns or type mismatches.
```suggestion
// resource_name can be NULL, so we handle the error as None
let resource_name: Option<String> = row
.column_by_name("resource_name")
.map_err(SysDbError::FailedToReadColumn)?;
```
File: rust/types/src/tenant.rs
Line: 34
tanujnay112
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with one comment

Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Added tests
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
None
Observability plan
None
Documentation Changes
None