feat: Bring back cassandra adapter using gocqlx v2#138
Conversation
c0eca16 to
ab098e3
Compare
ab098e3 to
660546c
Compare
f93c21b to
7bc3587
Compare
deanefrati
left a comment
There was a problem hiding this comment.
Some comments from my AI assisted initial review, i still need to go through it in mode detail myself and check it in a few of my micro-services to make sure it doesn't break anything which i will do in the next couple of days.
Design Review: Cassandra Adapter Implementation
Overview
This PR reintroduces the Cassandra adapter using gocqlx/v2. The implementation correctly follows the storage adapter interface and integrates well with the existing codebase. However, the design differs from other adapters in several ways - some are intentional and appropriate for Cassandra/gocqlx, while others should be addressed for consistency.
Design Consistency Analysis
✅ Strengths
- Adapter Interface Compliance: Correctly implements all required
StorageAdaptermethods - Singleton Pattern: Consistent with other adapters (SQL, DynamoDB, CosmosDB)
- Error Handling: Uses
errors.Joinfor proper error wrapping - Table Metadata: Pre-loads table metadata using gocqlx, which is appropriate for Cassandra
- Session Management: On-demand session creation with proper cleanup (
defer s.Close()) is reasonable for Cassandra
🔄 Design Differences (Intentional vs. Inconsistencies)
1. Initialization Pattern
- Current: Inline initialization in
GetCassandraAdapter()with error return - Other Adapters: Separate
OpenConnection()method, no error return - Impact: API inconsistency - Cassandra is the only adapter that can fail during factory creation
- Recommendation: Consider standardizing factory method signature across adapters, or document why Cassandra differs
2. Connection Management
- Current: On-demand session creation per operation
- Other Adapters: Persistent connections/clients stored in struct
- Impact: Different but acceptable for Cassandra;
gocql.ClusterConfigmanages connection pool - Status: ✅ Acceptable design choice
3. Table Metadata Pre-loading
- Current: Pre-loads all table metadata in
initializeTableMappers()during initialization - Other Adapters: Compute table names dynamically from type names
- Impact: Requires keyspace and tables to exist at initialization time
- Status: ✅ Appropriate for gocqlx, but less flexible than other adapters
🐛 Issues Found
Critical Bugs
1. Migration Table Handling (Lines 236, 261)
t, e := c.getTableForItem("migrations") // ❌ Won't work- Problem:
getTableForItem()expects a struct type, not a string. Passing"migrations"will fail becausetypeName("migrations")returns empty string. - Fix: Use direct table lookup:
c.tables["migrations"]or create a migration struct type
2. Count Method Returns Wrong Value (Line 395)
return -1, t.SelectBuilder().CountAll().Query(s).BindStruct(dest).ExecRelease()- Problem: Always returns
-1instead of actual count. The count result is not captured. - Fix: Capture and return the count value from the query result
3. Wrong Error Variable (Line 272)
return -1, errors.Join(
fmt.Errorf("failed creating a session"),
e, // Should be sErr
)- Problem: Uses
e(from line 261) instead ofsErr(from line 268) - Fix: Change
etosErr
Minor Issues
4. Typo in storage/reflection.go:20
- "fot" should be "for"
5. Max("id") Usage (Line 276)
- Verify that
gocqlx/v2SelectBuildersupportsMax()method. May need raw CQL query instead.
💡 Recommendations
High Priority
- ✅ Fix migration table handling - use direct table lookup or migration struct
- ✅ Fix Count method - return actual count value
- ✅ Fix error variable bug in
GetLatestMigration()
Medium Priority
⚠️ Consider API consistency - either:- Make all factory methods return
(Adapter, error), OR - Make Cassandra's factory method match others (no error return, handle errors differently)
- Make all factory methods return
⚠️ Add unit tests for Cassandra adapter methods, especially edge cases
Low Priority
- 📝 Fix typo in
reflection.go - 📝 Verify
Max()method usage or use alternative approach forGetLatestMigration() - 📝 Consider adding connection pooling documentation explaining why sessions are created per-operation
📊 Overall Assessment
The implementation is solid and follows gocqlx patterns correctly. The main concerns are:
- Three bugs that need fixing (migration table, Count method, error variable)
- API inconsistency in factory method signature
- Some design differences that are acceptable but should be documented
Recommendation: Address the critical bugs before merging. The API consistency issue can be handled in a follow-up if needed, but documenting the differences would be helpful.
Use SHA for github actions to avoid issues with caching and to ensure that the correct version of the action is used.
cfee533 to
7201d44
Compare
7201d44 to
97c8d10
Compare
| CASSANDRA_HOSTS: "localhost" | ||
| CASSANDRA_KEYSPACE: "testkeyspace" | ||
| COSMOSDB_ENDPOINT: "http://localhost:8081/" | ||
| COSMOSDB_KEY: "C2y6yDjf5/R+ob0N8A7Cgv30VRDJIWEHLM+4QDU5DE2nQ9nDuVTqobD4b8mGGg==" |
There was a problem hiding this comment.
nit-pick: worth adding a comment here (and in storage_test.go) to ack that this is indeed an emulator key to prevent security scans/reviews etc from flagging this unnescessarily
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces Cassandra as a new storage adapter provider by implementing a complete Cassandra-backed adapter, wiring it into the factory, adding reflection utilities for type-agnostic field resolution, provisioning Cassandra infrastructure in CI with health checks, and activating integration tests with environment-backed configuration. ChangesCassandra Storage Adapter Introduction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/main.go`:
- Line 53: The provider example blocks in examples/main.go use inconsistent
declarations for the shared variable config (some use `:=`, one uses `=`) which
can cause shadowing or undefined variable errors; pick one pattern and apply it
consistently across all provider examples: either keep the top-level `config :=
map[string]string{}` and change all provider blocks (Cassandra, PostgreSQL,
DynamoDB, CosmosDB) to use `=` assignment, or comment out the top-level `config`
and change all provider blocks to use `:=`; update the examples referencing the
`config` variable (the provider example blocks and the top-level declaration) so
they all use the same declaration style.
In `@go.mod`:
- Line 47: The go.mod entry for github.com/jackc/pgx/v5 is pinned to a
vulnerable indirect version; update the dependency to the latest patched v5
release (replace the version for github.com/jackc/pgx/v5) by running a module
upgrade (e.g., go get github.com/jackc/pgx/v5@latest and go mod tidy), then
verify the upgraded module in go.sum/go.mod and run your test suite; confirm the
CVE-2026-33815/GHSA fixes by checking the pgx v5 release notes or the module's
changelog/repository/security advisory for a mention of those CVEs.
In `@storage/cassandra.go`:
- Around line 183-199: The CreateSchema method currently hardcodes
replicationClass="SimpleStrategy" and replicationFactor=1 which is unsafe for
production; update CassandraAdapter.CreateSchema to read replication_class and
replication_factor from the adapter config (e.g. c.config["replication_class"]
and c.config["replication_factor"]), validate and coerce types, fall back to
sensible defaults only if missing, and when falling back emit a warning log (via
the adapter logger or standard log) indicating defaults are being used; then use
those variables in the fmt.Sprintf passed to c.Execute (keeping GetSchemaName
and the existing error wrapping behavior).
- Around line 337-348: In Count (CassandraAdapter.Count) the call to
t.SelectBuilder().CountAll().Query(c.session).GetRelease(count) passes count by
value so gocqlx cannot scan into it; change the call to pass a pointer
(GetRelease(&count)) so the scanned count is written into the local variable and
returned; ensure you only change the argument to GetRelease in that code path
inside Count.
- Around line 45-69: GetCassandraAdapter currently assigns the singleton
instance early (instance = &CassandraAdapter{config: config}) before calling
initConfig, createSession, CreateSchema, and initializeTableMappers, causing a
partially-initialized singleton on failures; change the flow so you only set
instance (and keep it protected by cassandraAdapterLock) after all
initialization steps (initConfig, createSession, CreateSchema,
initializeTableMappers) succeed, or if you keep the early assignment ensure you
reset instance = nil on every error path before returning; update code around
GetCassandraAdapter, CassandraAdapter, initConfig, createSession, CreateSchema,
initializeTableMappers and use cassandraAdapterLock to prevent races.
- Around line 94-102: The code shadows package constants named username and
password by declaring local variables with the same names in the c.config
lookup; change the local variable names (e.g., uname and pwd or foundUser and
foundPass) so they do not mask the package-level constants, update the
subsequent checks to use those new local names for existence and values, and
then set c.clusterConfig.Authenticator = gocql.PasswordAuthenticator{Username:
uname, Password: pwd} (still reading the actual credential values from c.config
using the package constants) to avoid confusion and ensure the correct map keys
are used.
- Around line 125-153: The initializeTableMappers method writes into the
c.tables map but never allocates it, causing a nil-map panic; fix by ensuring
c.tables is initialized (e.g., make(map[string]*table.Table) or equivalent)
before any assignments—either initialize c.tables in the CassandraAdapter
constructor/GetCassandraAdapter or at the start of initializeTableMappers;
locate CassandraAdapter, GetCassandraAdapter and initializeTableMappers and add
the allocation for c.tables prior to the loop that sets c.tables[t.Name].
- Around line 281-288: The primary-key exclusion loop uses
slices.Contains(t.PrimaryKeyCmp(), qb.Eq(k)) which relies on comparing qb.Cmp
structs (from t.PrimaryKeyCmp() and qb.Eq) and can fail if qb.Cmp is not
reliably comparable; instead extract the primary key column names and compare
strings. Update the logic around t.PrimaryKeyCmp(), qb.Eq and the loop building
columns from jsonMap to first build a set/map[string]bool of primary key names
(by iterating t.PrimaryKeyCmp() and extracting the column name) and then check
that map when deciding to append k to columns, leaving the existing columns
slice and jsonMap iteration intact.
- Around line 213-217: In UpdateMigrationTable, don't call
getTableForItem("migrations") (which passes a string and breaks
typeName/CamelToSnakeASCII logic); instead look up the migrations table directly
from the adapter's table map (e.g. access CassandraAdapter's tables map by the
"migrations" key), check for existence and return a clear error if missing;
update the code path that currently uses getTableForItem to use the direct map
lookup and preserve the existing error wrapping/handling so UpdateMigrationTable
still returns a formatted error on failure. Reference symbols:
UpdateMigrationTable, CassandraAdapter, getTableForItem, typeName,
CamelToSnakeASCII.
- Around line 309-331: The List method currently returns the input cursor
instead of the next page state; update CassandraAdapter.List to execute the
query via Iter() rather than SelectRelease(), call iter.Select(dest) to populate
results, then call iter.PageState() to get the next page bytes, base64-encode
those bytes and return that encoded string (or empty string if nil). Ensure you
still handle cursor decoding with base64.StdEncoding.DecodeString(cursor) and
preserve existing error wrapping from getTableForItem and query execution;
replace the SelectRelease usage with q.Iter(), iter.Select(dest) and
iter.PageState() to produce the correct next-cursor value.
In `@storage/reflection.go`:
- Around line 36-50: The typeName function can panic when item is nil and
returns an empty string for anonymous types; fix it by guarding
reflect.TypeOf(item) == nil and returning "nil" (or a clear fallback)
immediately, use valueOf.IsValid() and valueOf.IsNil() checks before calling
Elem() when testing pointer kinds (use reflect.Ptr), and when typeOf.Name() is
empty return typeOf.String() (or another stable identifier) so anonymous
structs/interfaces don't yield an empty name; apply these checks inside typeName
to keep the existing pointer->slice->elem logic but defensively handle
nil/anonymous cases.
- Around line 23-29: The GetValue function currently returns (nil, nil) when a
field isn't found, which makes "missing field" indistinguishable from "found
field with nil value"; change GetValue to return a sentinel error (e.g., declare
var ErrFieldNotFound = errors.New("field not found")) and return (nil,
ErrFieldNotFound) when the loop finishes without a match, update the function
signature's error return usage accordingly, and adjust all callers of GetValue
to check for ErrFieldNotFound versus a nil error to handle the two cases
explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc2dc423-ff0c-430b-ac55-176cd83fa2e7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/ci.ymlexamples/main.gogo.modstorage/cassandra.gostorage/cassandra.go.backupstorage/reflection.gostorage/reflection.go.backupstorage/storage.gostorage/storage_test.go
💤 Files with no reviewable changes (2)
- storage/reflection.go.backup
- storage/cassandra.go.backup
| // } | ||
|
|
||
| // config := map[string]string{ | ||
| // config = map[string]string{ |
There was a problem hiding this comment.
Inconsistent variable declaration pattern across provider examples.
The Cassandra configuration example now uses = (assignment), while the PostgreSQL (line 27), DynamoDB (line 37), and CosmosDB (line 46) examples still use := (short declaration). This inconsistency creates confusion:
- With line 25's
config := map[string]string{}active, using=is correct, but then the other three examples would shadow the variable with:= - If users comment out line 25 to use a single provider example,
=would fail (undefined variable) while:=would work
For clarity and consistency, standardize all commented provider examples to use the same pattern.
Recommendation: Either make all examples use = (assuming line 25 stays), or make all examples use := (and document that line 25 should be commented out when using a specific provider).
♻️ Proposed fix for consistency
Option 1: Make all provider examples use = (keeps line 25 as-is):
- // config := map[string]string{
+ // config = map[string]string{
// "provider": "postgresql",
// "host": "host.docker.internal",- // config := map[string]string{
+ // config = map[string]string{
// "provider": "dynamodb",
// "region": "us-west-2",- // config := map[string]string{
+ // config = map[string]string{
// "provider": "cosmosdb",
// "endpoint": "https://your-cosmosdb-account.documents.azure.com:443/",Option 2: Make all provider examples use := and comment out line 25:
- config := map[string]string{}
+ // config := map[string]string{}- // config = map[string]string{
+ // config := map[string]string{
// "provider": "cassandra",
// "endpoint": "localhost",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/main.go` at line 53, The provider example blocks in examples/main.go
use inconsistent declarations for the shared variable config (some use `:=`, one
uses `=`) which can cause shadowing or undefined variable errors; pick one
pattern and apply it consistently across all provider examples: either keep the
top-level `config := map[string]string{}` and change all provider blocks
(Cassandra, PostgreSQL, DynamoDB, CosmosDB) to use `=` assignment, or comment
out the top-level `config` and change all provider blocks to use `:=`; update
the examples referencing the `config` variable (the provider example blocks and
the top-level declaration) so they all use the same declaration style.
| github.com/jackc/pgpassfile v1.0.0 // indirect | ||
| github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect | ||
| github.com/jackc/pgx/v5 v5.6.0 // indirect | ||
| github.com/jackc/pgx/v5 v5.8.0 // indirect |
There was a problem hiding this comment.
Critical security vulnerabilities in github.com/jackc/pgx/v5 v5.8.0 require immediate upgrade.
Static analysis has flagged multiple critical CVEs affecting this version:
- CVE-2026-33815, CVE-2026-33816 (memory safety)
- GHSA-j88v-2chj-qfwx (SQL injection via placeholder confusion)
Even though this is an indirect dependency, these vulnerabilities—particularly the SQL injection vector—pose significant risk to any PostgreSQL-backed storage paths.
What is the latest version of github.com/jackc/pgx/v5 Go library and does it fix CVE-2026-33815?
🧰 Tools
🪛 OSV Scanner (2.3.6)
[CRITICAL] 47-47: github.com/jackc/pgx/v5 5.8.0: CVE-2026-33815 in github.com/jackc/pgx
(GO-2026-4771)
[CRITICAL] 47-47: github.com/jackc/pgx/v5 5.8.0: CVE-2026-33816 in github.com/jackc/pgx
(GO-2026-4772)
[CRITICAL] 47-47: github.com/jackc/pgx/v5 5.8.0: Memory-safety vulnerability in github.com/jackc/pgx/v5.
[CRITICAL] 47-47: github.com/jackc/pgx/v5 5.8.0: pgx: SQL Injection via placeholder confusion with dollar quoted string literals
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 47, The go.mod entry for github.com/jackc/pgx/v5 is pinned to
a vulnerable indirect version; update the dependency to the latest patched v5
release (replace the version for github.com/jackc/pgx/v5) by running a module
upgrade (e.g., go get github.com/jackc/pgx/v5@latest and go mod tidy), then
verify the upgraded module in go.sum/go.mod and run your test suite; confirm the
CVE-2026-33815/GHSA fixes by checking the pgx v5 release notes or the module's
changelog/repository/security advisory for a mention of those CVEs.
| func GetCassandraAdapter(config map[string]string) (*CassandraAdapter, error) { | ||
| if instance == nil { | ||
| cassandraAdapterLock.Lock() | ||
| defer cassandraAdapterLock.Unlock() | ||
| if instance == nil { | ||
| instance = &CassandraAdapter{config: config} | ||
| err := instance.initConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("initConfig failed: %w", err) | ||
| } | ||
| if session, err := instance.createSession(); err != nil { | ||
| return nil, fmt.Errorf("failed to create session: %w", err) | ||
| } else { | ||
| instance.session = session | ||
| } | ||
|
|
||
| // The call to createSchema will set clusterConfig.Keyspace to the | ||
| // actual Keyspace, this is why its here. | ||
| if createSchemaErr := instance.CreateSchema(); createSchemaErr != nil { | ||
| return nil, fmt.Errorf("failed to create schema: %w", createSchemaErr) | ||
| } | ||
| if err := instance.initializeTableMappers(); err != nil { | ||
| return nil, fmt.Errorf("failed to initialize table mappers: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Singleton assignment before initialization completes creates inconsistent state on failure.
Line 50 assigns instance = &CassandraAdapter{...} before initConfig(), createSession(), and CreateSchema() succeed. If any of these fail, subsequent calls to GetCassandraAdapter will return nil errors because the outer instance == nil check passes, but the inner check finds a partially-initialized instance.
Move assignment after successful initialization, or reset instance = nil on failure:
Proposed fix
func GetCassandraAdapter(config map[string]string) (*CassandraAdapter, error) {
if instance == nil {
cassandraAdapterLock.Lock()
defer cassandraAdapterLock.Unlock()
if instance == nil {
- instance = &CassandraAdapter{config: config}
+ adapter := &CassandraAdapter{
+ config: config,
+ tables: make(map[string]*table.Table),
+ }
- err := instance.initConfig()
+ err := adapter.initConfig()
if err != nil {
return nil, fmt.Errorf("initConfig failed: %w", err)
}
- if session, err := instance.createSession(); err != nil {
+ if session, err := adapter.createSession(); err != nil {
return nil, fmt.Errorf("failed to create session: %w", err)
} else {
- instance.session = session
+ adapter.session = session
}
- if createSchemaErr := instance.CreateSchema(); createSchemaErr != nil {
+ if createSchemaErr := adapter.CreateSchema(); createSchemaErr != nil {
return nil, fmt.Errorf("failed to create schema: %w", createSchemaErr)
}
- if err := instance.initializeTableMappers(); err != nil {
+ if err := adapter.initializeTableMappers(); err != nil {
return nil, fmt.Errorf("failed to initialize table mappers: %w", err)
}
+ instance = adapter
}
}
return instance, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetCassandraAdapter(config map[string]string) (*CassandraAdapter, error) { | |
| if instance == nil { | |
| cassandraAdapterLock.Lock() | |
| defer cassandraAdapterLock.Unlock() | |
| if instance == nil { | |
| instance = &CassandraAdapter{config: config} | |
| err := instance.initConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("initConfig failed: %w", err) | |
| } | |
| if session, err := instance.createSession(); err != nil { | |
| return nil, fmt.Errorf("failed to create session: %w", err) | |
| } else { | |
| instance.session = session | |
| } | |
| // The call to createSchema will set clusterConfig.Keyspace to the | |
| // actual Keyspace, this is why its here. | |
| if createSchemaErr := instance.CreateSchema(); createSchemaErr != nil { | |
| return nil, fmt.Errorf("failed to create schema: %w", createSchemaErr) | |
| } | |
| if err := instance.initializeTableMappers(); err != nil { | |
| return nil, fmt.Errorf("failed to initialize table mappers: %w", err) | |
| } | |
| } | |
| func GetCassandraAdapter(config map[string]string) (*CassandraAdapter, error) { | |
| if instance == nil { | |
| cassandraAdapterLock.Lock() | |
| defer cassandraAdapterLock.Unlock() | |
| if instance == nil { | |
| adapter := &CassandraAdapter{ | |
| config: config, | |
| tables: make(map[string]*table.Table), | |
| } | |
| err := adapter.initConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("initConfig failed: %w", err) | |
| } | |
| if session, err := adapter.createSession(); err != nil { | |
| return nil, fmt.Errorf("failed to create session: %w", err) | |
| } else { | |
| adapter.session = session | |
| } | |
| // The call to createSchema will set clusterConfig.Keyspace to the | |
| // actual Keyspace, this is why its here. | |
| if createSchemaErr := adapter.CreateSchema(); createSchemaErr != nil { | |
| return nil, fmt.Errorf("failed to create schema: %w", createSchemaErr) | |
| } | |
| if err := adapter.initializeTableMappers(); err != nil { | |
| return nil, fmt.Errorf("failed to initialize table mappers: %w", err) | |
| } | |
| instance = adapter | |
| } | |
| } | |
| return instance, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 45 - 69, GetCassandraAdapter currently
assigns the singleton instance early (instance = &CassandraAdapter{config:
config}) before calling initConfig, createSession, CreateSchema, and
initializeTableMappers, causing a partially-initialized singleton on failures;
change the flow so you only set instance (and keep it protected by
cassandraAdapterLock) after all initialization steps (initConfig, createSession,
CreateSchema, initializeTableMappers) succeed, or if you keep the early
assignment ensure you reset instance = nil on every error path before returning;
update code around GetCassandraAdapter, CassandraAdapter, initConfig,
createSession, CreateSchema, initializeTableMappers and use cassandraAdapterLock
to prevent races.
| if username, ok := c.config[username]; ok { | ||
| if password, ok := c.config[password]; !ok { | ||
| return fmt.Errorf("password is required when username is provided") | ||
| } else { | ||
| c.clusterConfig.Authenticator = gocql.PasswordAuthenticator{ | ||
| Username: username, | ||
| Password: c.config[password], | ||
| } | ||
| } |
There was a problem hiding this comment.
Variable shadowing: local username and password mask package constants.
Lines 94 and 95 declare username and password as local variables that shadow the package-level constants defined at lines 30-31. This compiles but creates confusion—c.config[password] at line 100 references the constant, while the conditional uses the shadowed variable.
Proposed fix
- if username, ok := c.config[username]; ok {
- if password, ok := c.config[password]; !ok {
+ if user, ok := c.config[username]; ok {
+ if pass, ok := c.config[password]; !ok {
return fmt.Errorf("password is required when username is provided")
} else {
c.clusterConfig.Authenticator = gocql.PasswordAuthenticator{
- Username: username,
- Password: c.config[password],
+ Username: user,
+ Password: pass,
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if username, ok := c.config[username]; ok { | |
| if password, ok := c.config[password]; !ok { | |
| return fmt.Errorf("password is required when username is provided") | |
| } else { | |
| c.clusterConfig.Authenticator = gocql.PasswordAuthenticator{ | |
| Username: username, | |
| Password: c.config[password], | |
| } | |
| } | |
| if user, ok := c.config[username]; ok { | |
| if pass, ok := c.config[password]; !ok { | |
| return fmt.Errorf("password is required when username is provided") | |
| } else { | |
| c.clusterConfig.Authenticator = gocql.PasswordAuthenticator{ | |
| Username: user, | |
| Password: pass, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 94 - 102, The code shadows package
constants named username and password by declaring local variables with the same
names in the c.config lookup; change the local variable names (e.g., uname and
pwd or foundUser and foundPass) so they do not mask the package-level constants,
update the subsequent checks to use those new local names for existence and
values, and then set c.clusterConfig.Authenticator =
gocql.PasswordAuthenticator{Username: uname, Password: pwd} (still reading the
actual credential values from c.config using the package constants) to avoid
confusion and ensure the correct map keys are used.
| func (c *CassandraAdapter) initializeTableMappers() error { | ||
| metadata, mErr := c.session.KeyspaceMetadata(c.config[keyspace]) | ||
| if mErr != nil { | ||
| return fmt.Errorf("failed reading tables metadata: %w", mErr) | ||
| } | ||
|
|
||
| for _, t := range metadata.Tables { | ||
| tableMetadata := table.Metadata{ | ||
| Name: t.Name, | ||
| Columns: t.OrderedColumns, | ||
| } | ||
| if len(t.PartitionKey) > 0 { | ||
| partitionKeys := []string{} | ||
| for _, k := range t.PartitionKey { | ||
| partitionKeys = append(partitionKeys, k.Name) | ||
| } | ||
| tableMetadata.PartKey = partitionKeys | ||
| } | ||
| if len(t.ClusteringColumns) > 0 { | ||
| sortKeys := []string{} | ||
| for _, k := range t.ClusteringColumns { | ||
| sortKeys = append(sortKeys, k.Name) | ||
| } | ||
| tableMetadata.SortKey = sortKeys | ||
| } | ||
| c.tables[t.Name] = table.New(tableMetadata) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: c.tables is never initialized—nil map write will panic.
The initializeTableMappers method writes to c.tables on line 150, but this map is never allocated. The first assignment will cause a runtime panic.
Proposed fix in GetCassandraAdapter
if instance == nil {
instance = &CassandraAdapter{config: config}
+ instance.tables = make(map[string]*table.Table)
err := instance.initConfig()Or within initializeTableMappers:
func (c *CassandraAdapter) initializeTableMappers() error {
+ c.tables = make(map[string]*table.Table)
metadata, mErr := c.session.KeyspaceMetadata(c.config[keyspace])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *CassandraAdapter) initializeTableMappers() error { | |
| metadata, mErr := c.session.KeyspaceMetadata(c.config[keyspace]) | |
| if mErr != nil { | |
| return fmt.Errorf("failed reading tables metadata: %w", mErr) | |
| } | |
| for _, t := range metadata.Tables { | |
| tableMetadata := table.Metadata{ | |
| Name: t.Name, | |
| Columns: t.OrderedColumns, | |
| } | |
| if len(t.PartitionKey) > 0 { | |
| partitionKeys := []string{} | |
| for _, k := range t.PartitionKey { | |
| partitionKeys = append(partitionKeys, k.Name) | |
| } | |
| tableMetadata.PartKey = partitionKeys | |
| } | |
| if len(t.ClusteringColumns) > 0 { | |
| sortKeys := []string{} | |
| for _, k := range t.ClusteringColumns { | |
| sortKeys = append(sortKeys, k.Name) | |
| } | |
| tableMetadata.SortKey = sortKeys | |
| } | |
| c.tables[t.Name] = table.New(tableMetadata) | |
| } | |
| return nil | |
| } | |
| func (c *CassandraAdapter) initializeTableMappers() error { | |
| c.tables = make(map[string]*table.Table) | |
| metadata, mErr := c.session.KeyspaceMetadata(c.config[keyspace]) | |
| if mErr != nil { | |
| return fmt.Errorf("failed reading tables metadata: %w", mErr) | |
| } | |
| for _, t := range metadata.Tables { | |
| tableMetadata := table.Metadata{ | |
| Name: t.Name, | |
| Columns: t.OrderedColumns, | |
| } | |
| if len(t.PartitionKey) > 0 { | |
| partitionKeys := []string{} | |
| for _, k := range t.PartitionKey { | |
| partitionKeys = append(partitionKeys, k.Name) | |
| } | |
| tableMetadata.PartKey = partitionKeys | |
| } | |
| if len(t.ClusteringColumns) > 0 { | |
| sortKeys := []string{} | |
| for _, k := range t.ClusteringColumns { | |
| sortKeys = append(sortKeys, k.Name) | |
| } | |
| tableMetadata.SortKey = sortKeys | |
| } | |
| c.tables[t.Name] = table.New(tableMetadata) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 125 - 153, The initializeTableMappers
method writes into the c.tables map but never allocates it, causing a nil-map
panic; fix by ensuring c.tables is initialized (e.g.,
make(map[string]*table.Table) or equivalent) before any assignments—either
initialize c.tables in the CassandraAdapter constructor/GetCassandraAdapter or
at the start of initializeTableMappers; locate CassandraAdapter,
GetCassandraAdapter and initializeTableMappers and add the allocation for
c.tables prior to the loop that sets c.tables[t.Name].
| columns := []string{} | ||
| for k := range jsonMap { | ||
| if !slices.Contains(t.PrimaryKeyCmp(), qb.Eq(k)) { | ||
| columns = append(columns, k) | ||
| } else { | ||
| slog.Debug("Column is part of primary key, excluding from columns slice", slog.String("pKeyColumn", k)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Primary key exclusion via slices.Contains comparing qb.Cmp structs may not work reliably.
t.PrimaryKeyCmp() returns []qb.Cmp, and you're checking if qb.Eq(k) is contained. This relies on struct equality, which works only if qb.Cmp is comparable and both sides produce identical struct values. If qb.Cmp contains unexported fields or function pointers, this comparison may fail unexpectedly.
Consider extracting primary key column names directly:
Proposed refactor
+ // Build a set of primary key column names for exclusion
+ pkColumns := make(map[string]struct{})
+ for _, pk := range append(t.Metadata().PartKey, t.Metadata().SortKey...) {
+ pkColumns[pk] = struct{}{}
+ }
+
columns := []string{}
for k := range jsonMap {
- if !slices.Contains(t.PrimaryKeyCmp(), qb.Eq(k)) {
+ if _, isPK := pkColumns[k]; !isPK {
columns = append(columns, k)
} else {
slog.Debug("Column is part of primary key, excluding from columns slice", slog.String("pKeyColumn", k))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| columns := []string{} | |
| for k := range jsonMap { | |
| if !slices.Contains(t.PrimaryKeyCmp(), qb.Eq(k)) { | |
| columns = append(columns, k) | |
| } else { | |
| slog.Debug("Column is part of primary key, excluding from columns slice", slog.String("pKeyColumn", k)) | |
| } | |
| } | |
| // Build a set of primary key column names for exclusion | |
| pkColumns := make(map[string]struct{}) | |
| for _, pk := range append(t.Metadata().PartKey, t.Metadata().SortKey...) { | |
| pkColumns[pk] = struct{}{} | |
| } | |
| columns := []string{} | |
| for k := range jsonMap { | |
| if _, isPK := pkColumns[k]; !isPK { | |
| columns = append(columns, k) | |
| } else { | |
| slog.Debug("Column is part of primary key, excluding from columns slice", slog.String("pKeyColumn", k)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 281 - 288, The primary-key exclusion loop
uses slices.Contains(t.PrimaryKeyCmp(), qb.Eq(k)) which relies on comparing
qb.Cmp structs (from t.PrimaryKeyCmp() and qb.Eq) and can fail if qb.Cmp is not
reliably comparable; instead extract the primary key column names and compare
strings. Update the logic around t.PrimaryKeyCmp(), qb.Eq and the loop building
columns from jsonMap to first build a set/map[string]bool of primary key names
(by iterating t.PrimaryKeyCmp() and extracting the column name) and then check
that map when deciding to append k to columns, leaving the existing columns
slice and jsonMap iteration intact.
| func (c *CassandraAdapter) List(dest any, sortKey string, filters map[string]any, limit int, cursor string, params ...map[string]any) (string, error) { | ||
| t, tableErr := c.getTableForItem(dest) | ||
| if tableErr != nil { | ||
| return "", fmt.Errorf("list failed getting table for item: %w", tableErr) | ||
| } | ||
| q := t.SelectQuery(c.session).BindMap(filters) | ||
| if cursor != "" { | ||
| bytes, err := base64.StdEncoding.DecodeString(cursor) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid cursor: %w", err) | ||
| } | ||
| q = q.PageState(bytes) | ||
| } | ||
| if limit > 0 { | ||
| q = q.PageSize(limit) | ||
| } | ||
| // TODO: Verify pagination behavior | ||
| err := q.SelectRelease(dest) | ||
| if err != nil { | ||
| return "", fmt.Errorf("list failed selecting release: %w", err) | ||
| } | ||
| return cursor, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify gocqlx Queryx methods for pagination support
ast-grep --pattern $'func ($_ *Queryx) Iter() $_'Repository: tink3rlabs/magic
Length of output: 42
🏁 Script executed:
# First, let's examine the cassandra.go file to see the current implementation
cd storage && head -15 cassandra.go && echo "---" && sed -n '309,331p' cassandra.goRepository: tink3rlabs/magic
Length of output: 966
🏁 Script executed:
# Search for gocqlx imports and usage in the repository
rg "gocqlx|Queryx|SelectRelease" -t go --max-count 20Repository: tink3rlabs/magic
Length of output: 504
🏁 Script executed:
# Look for any documentation or go.mod to understand gocqlx version
cat go.mod | grep gocqlxRepository: tink3rlabs/magic
Length of output: 98
🏁 Script executed:
# Search for SelectRelease usage and related pagination code
rg "SelectRelease|PageState|Iter\(\)" -B 2 -A 2 -t goRepository: tink3rlabs/magic
Length of output: 526
🏁 Script executed:
# Look for table.Table interface definition or similar pagination examples
rg "SelectQuery" -B 3 -A 3 -t goRepository: tink3rlabs/magic
Length of output: 449
🏁 Script executed:
# Check if there are any tests for pagination that show expected behavior
fd "cassandra" -name "*.go" -type f | xargs rg -l "Test.*List|pagination" 2>/dev/nullRepository: tink3rlabs/magic
Length of output: 416
🏁 Script executed:
# Check if there are test files specifically for cassandra
fd "test" storage/ -name "*.go" -type fRepository: tink3rlabs/magic
Length of output: 291
🏁 Script executed:
# Look for any gocqlx examples or usage of Iter method
rg "\.Iter\(\)|\.PageState" -t goRepository: tink3rlabs/magic
Length of output: 106
🌐 Web query:
gocqlx v2 Queryx pagination PageState Iter SelectRelease
💡 Result:
In gocqlx v2, stateful pagination is done by configuring the underlying iterator with a page size and a PageState cursor, then reading the next PageState back from the iterator. Key APIs you’re looking for (v2): 1) Set PageState (incoming cursor) and PageSize on the query builder before calling Iter - q.PageState(page) sets the incoming paging cursor value you received from the previous call.[1] - q.PageSize(itemsPerPage) sets the number of rows the page should return.[1] 2) Read the next PageState from the iterator - after iter:= q.Iter, you can fetch nextPage state via iter.PageState and return it to the caller.[1][2] 3) Typical pagination flow - Accept page []byte (the client cursor) as input. - Configure q.PageState(page) and q.PageSize(N). - Execute page fetch via iter.Select(&slice) (or iter.Select(&slice) depending on your style). - Return the results and iter.PageState as the next cursor. Example pattern (from an in-tree style usage): func getUserVideos(userID int, page []byte) (videos []Video, nextPage []byte, err error) { q:= videoTable.SelectQuery(session).Bind(userID) defer q.Release q.PageState(page) q.PageSize(itemsPerPage) iter:= q.Iter return videos, iter.PageState, iter.Select(&videos) } This “send page state to caller” pattern is explicitly shown, including iter.PageState usage.[1] Official gocqlx example showing stateful paging forwarding PageState: - The gocqlx repo test example defines getUserVideos(userID, page []byte) that calls q.PageState(page), q.PageSize(itemsPerPage), creates iter:= q.Iter, then returns iter.PageState to the caller.[2] Clarification on your term “Queryx pagination PageState Iter SelectRelease” - In gocqlx v2, the pagination cursor is PageState (typically []byte). - You apply it on the query object before Iter. - You retrieve it from the iterator after execution. - Release variants: gocqlx has SelectRelease for fetching all rows into a slice (non-iterative), while the iterator approach uses iter.Select(...) and you also release the query object (q.Release).[3][2] If you share the exact type/signature you mean by “Queryx” vs “Queryx::Iter” in your codebase, I can map it precisely to the correct gocqlx/v2 objects and methods—gocqlx’s paging is implemented around Query/PageState/Iter/iter.PageState in the examples above.
Citations:
- 1: https://stackoverflow.com/questions/71366029/scylla-gocqlx-how-to-implement-pagination-similar-to-a-cursor
- 2: https://forum.scylladb.com/t/gocql-query-paging-issue/1497
- 3: https://pkg.go.dev/github.com/scylladb/gocqlx/v2
List returns the input cursor instead of the new page state, breaking pagination.
Line 330 returns cursor (the input), not the query's resulting page state. This prevents pagination—callers receive the same cursor repeatedly and cannot advance through pages.
The gocqlx v2 pagination pattern requires using Iter() to access page state after query execution. The current SelectRelease() approach doesn't expose the iterator's page state; you'll need to switch to Iter() followed by iter.Select() and iter.PageState() to retrieve the next cursor for the caller.
Correct approach
func (c *CassandraAdapter) List(dest any, sortKey string, filters map[string]any, limit int, cursor string, params ...map[string]any) (string, error) {
t, tableErr := c.getTableForItem(dest)
if tableErr != nil {
return "", fmt.Errorf("list failed getting table for item: %w", tableErr)
}
q := t.SelectQuery(c.session).BindMap(filters)
if cursor != "" {
bytes, err := base64.StdEncoding.DecodeString(cursor)
if err != nil {
return "", fmt.Errorf("invalid cursor: %w", err)
}
q = q.PageState(bytes)
}
if limit > 0 {
q = q.PageSize(limit)
}
- // TODO: Verify pagination behavior
- err := q.SelectRelease(dest)
+ iter := q.Iter()
+ err := iter.Select(dest)
if err != nil {
return "", fmt.Errorf("list failed selecting release: %w", err)
}
- return cursor, nil
+ newPageState := iter.PageState()
+ iter.Close()
+ if len(newPageState) == 0 {
+ return "", nil
+ }
+ return base64.StdEncoding.EncodeToString(newPageState), nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 309 - 331, The List method currently
returns the input cursor instead of the next page state; update
CassandraAdapter.List to execute the query via Iter() rather than
SelectRelease(), call iter.Select(dest) to populate results, then call
iter.PageState() to get the next page bytes, base64-encode those bytes and
return that encoded string (or empty string if nil). Ensure you still handle
cursor decoding with base64.StdEncoding.DecodeString(cursor) and preserve
existing error wrapping from getTableForItem and query execution; replace the
SelectRelease usage with q.Iter(), iter.Select(dest) and iter.PageState() to
produce the correct next-cursor value.
| func (c *CassandraAdapter) Count(dest any, filter map[string]any, params ...map[string]any) (int64, error) { | ||
| t, tableErr := c.getTableForItem(dest) | ||
| if tableErr != nil { | ||
| return -1, fmt.Errorf("count failed getting table for item: %w", tableErr) | ||
| } | ||
| var count int64 | ||
| err := t.SelectBuilder().CountAll().Query(c.session).GetRelease(count) | ||
| if err != nil { | ||
| return -1, fmt.Errorf("count failed executing count query: %w", err) | ||
| } | ||
| return count, nil | ||
| } |
There was a problem hiding this comment.
GetRelease requires a pointer receiver; passing count by value won't scan.
Line 343 passes count (an int64 value) to GetRelease, but gocqlx requires a pointer to write the result. The count will always be zero.
Proposed fix
func (c *CassandraAdapter) Count(dest any, filter map[string]any, params ...map[string]any) (int64, error) {
t, tableErr := c.getTableForItem(dest)
if tableErr != nil {
return -1, fmt.Errorf("count failed getting table for item: %w", tableErr)
}
var count int64
- err := t.SelectBuilder().CountAll().Query(c.session).GetRelease(count)
+ err := t.SelectBuilder().CountAll().Query(c.session).GetRelease(&count)
if err != nil {
return -1, fmt.Errorf("count failed executing count query: %w", err)
}
return count, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/cassandra.go` around lines 337 - 348, In Count
(CassandraAdapter.Count) the call to
t.SelectBuilder().CountAll().Query(c.session).GetRelease(count) passes count by
value so gocqlx cannot scan into it; change the call to pass a pointer
(GetRelease(&count)) so the scanned count is written into the local variable and
returned; ensure you only change the argument to GetRelease in that code path
inside Count.
| for fIdx := range val.NumField() { | ||
| f := val.Type().Field(fIdx) | ||
| if strings.EqualFold(f.Name, field) || strings.EqualFold(strings.Split(f.Tag.Get("json"), ",")[0], field) { | ||
| return val.Field(fIdx).Interface(), nil | ||
| } | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Ambiguous return semantics: (nil, nil) conflates "field not found" with "field has nil value".
When GetValue cannot locate a matching field, returning (nil, nil) is indistinguishable from finding a field whose value is legitimately nil. This ambiguity can lead to silent bugs in callers that need to differentiate between "missing field" and "present but nil."
Consider returning a sentinel error (e.g., ErrFieldNotFound) to disambiguate:
Proposed fix
+var ErrFieldNotFound = errors.New("field not found")
+
func GetValue(item any, field string) (any, error) {
val := reflect.ValueOf(item)
if val.Kind() == reflect.Pointer {
val = val.Elem()
}
if val.Kind() != reflect.Struct {
return nil, fmt.Errorf("expected struct for %s", val.Kind())
}
for fIdx := range val.NumField() {
f := val.Type().Field(fIdx)
if strings.EqualFold(f.Name, field) || strings.EqualFold(strings.Split(f.Tag.Get("json"), ",")[0], field) {
return val.Field(fIdx).Interface(), nil
}
}
- return nil, nil
+ return nil, ErrFieldNotFound
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/reflection.go` around lines 23 - 29, The GetValue function currently
returns (nil, nil) when a field isn't found, which makes "missing field"
indistinguishable from "found field with nil value"; change GetValue to return a
sentinel error (e.g., declare var ErrFieldNotFound = errors.New("field not
found")) and return (nil, ErrFieldNotFound) when the loop finishes without a
match, update the function signature's error return usage accordingly, and
adjust all callers of GetValue to check for ErrFieldNotFound versus a nil error
to handle the two cases explicitly.
| func typeName(item any) string { | ||
| typeOf := reflect.TypeOf(item) | ||
| valueOf := reflect.ValueOf(item) | ||
| if valueOf.Kind() == reflect.Pointer { | ||
| elemVal := valueOf.Elem() | ||
| if elemVal.Kind() == reflect.Slice { | ||
| sliceType := elemVal.Type() | ||
| sliceElemType := sliceType.Elem() | ||
| if sliceElemType.Kind() == reflect.Pointer { | ||
| typeOf = sliceElemType.Elem() | ||
| } | ||
| } | ||
| } | ||
| return typeOf.Name() | ||
| } |
There was a problem hiding this comment.
typeName may panic on nil input or return empty string for anonymous types.
When item is nil, reflect.TypeOf(item) returns nil, and calling typeOf.Name() on line 49 will panic. Additionally, for anonymous structs or interface types, Name() returns an empty string—potentially causing downstream table-lookup failures.
A defensive nil-check and fallback would harden this utility:
Proposed fix
func typeName(item any) string {
+ if item == nil {
+ return ""
+ }
typeOf := reflect.TypeOf(item)
valueOf := reflect.ValueOf(item)
if valueOf.Kind() == reflect.Pointer {
elemVal := valueOf.Elem()
if elemVal.Kind() == reflect.Slice {
sliceType := elemVal.Type()
sliceElemType := sliceType.Elem()
if sliceElemType.Kind() == reflect.Pointer {
typeOf = sliceElemType.Elem()
}
}
+ } else if valueOf.Kind() == reflect.Pointer && !valueOf.IsNil() {
+ typeOf = typeOf.Elem()
}
+ if typeOf == nil {
+ return ""
+ }
return typeOf.Name()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/reflection.go` around lines 36 - 50, The typeName function can panic
when item is nil and returns an empty string for anonymous types; fix it by
guarding reflect.TypeOf(item) == nil and returning "nil" (or a clear fallback)
immediately, use valueOf.IsValid() and valueOf.IsNil() checks before calling
Elem() when testing pointer kinds (use reflect.Ptr), and when typeOf.Name() is
empty return typeOf.String() (or another stable identifier) so anonymous
structs/interfaces don't yield an empty name; apply these checks inside typeName
to keep the existing pointer->slice->elem logic but defensively handle
nil/anonymous cases.
Use SHA for github actions to avoid issues with
caching and to ensure that the correct version of
the action is used.