session/database: respect GORM NamingStrategy.TablePrefix for table names
- Dominant language
- Go
- Stars
- 8.8k
- Forks
- 1k
- Avg merge
- 3d 18h
- Merged PRs (30d)
- 88
Description
** Please make sure you read the contribution guide and file the issues in the right place. **
[Contribution guide.](https://google.github.io/adk-docs/contributing-guide/)
## 🔴 Required Information
_Please ensure all items in this section are completed to allow for efficient
triaging. If an item is not applicable to you - please mark it as N/A_
### Is your feature request related to a specific problem?
Yes. I am unable to configure table names (specifically applying a `TablePrefix`) when using the `session/database` package with GORM.
My use case requires running multiple versions of an application (e.g., different versions of an agent) against the same Google Cloud Spanner instance. To avoid data collisions, I need to prefix table names (e.g., `v1_sessions`, `v2_sessions`).
However, the internal models (e.g., `storageSession`, `storageEvent`) appear to define a `TableName()` method or force a specific table name. This hardcoding overrides the `gorm.Config{NamingStrategy: ...}` settings passed into `NewSessionService`. Additionally, because `AutoMigrate` migrates these unexported structs directly, I cannot intervene to rename the tables during migration.
### Proposed Solution
I propose modifying the internal models in the `database` package to respect the GORM `NamingStrategy`.
**Preferred approach:**
Remove the `TableName()` method from the internal structs (`storageSession`, `storageEvent`, etc.). This would allow the `gorm.Config` passed into `NewSessionService` to control naming (including `TablePrefix`, `SingularTable`, etc.) naturally.
**Alternative approach:**
If hardcoded names are required for some internal reason, please expose a configuration option in `NewSessionService` or a separate functional option to set a table prefix explicitly.
### Impact on your work
This is a blocker for our multi-tenancy and versioning strategy. We are currently unable to deploy new versions of our services side-by-side in the same database instance without risking data corruption or collisions.
We are currently blocked from proceeding with our release pipeline until we can isolate these tables.
---
## 🟡 Recommended Information
### Alternatives Considered
1. **Wrapping the Dialector:** I attempted to wrap the `gorm.Dialector` to intercept `Register` callbacks and rewrite `db.Statement.Table` on the fly. This works for runtime queries but fails for `AutoMigrate`. `AutoMigrate` does not trigger standard query callbacks, meaning it still creates tables with the hardcoded names.
2. **Manual DDL:** Since the models (`storageSession`) are unexported, I cannot run migration manually on them. I would have to reverse-engineer the schema and write raw SQL DDL to create the prefixed tables, which is brittle and prone to breaking on library updates.
### Willingness to contribute
Yes, I am willing to submit a PR if given guidance on the preferred approach (e.g., whether to remove `TableName()` or add a configuration field).
### Proposed API / Implementation
If `TableName()` is removed from internal models, no API change is needed. The existing code would simply start respecting the user's config:
```go
// User code
dbService, _ := database.NewSessionService(dialector, &gorm.Config{
NamingStrategy: schema.NamingStrategy{
TablePrefix: "v2_", // This would now correctly result in "v2_sessions"
},
})
```
If `TableName()` cannot be removed, we could add an option:
```go
// Inside internal/database/models.go
func (s *storageSession) TableName() string {
// Ideally dynamic based on some injected config
return s.config.Prefix + "sessions"
}
```
### Additional Context
Currently using the Spanner dialector. The issue seems to stem from the precedence GORM places on the `TableName()` interface over `NamingStrategy` config. Since we cannot access the unexported structs to override this method, we are locked into the default names.
Contributor guide
Assessment
This issue has not been assessed yet.