Add validation for queries #98
Conversation
MarcusGoldschmidt
requested review from
btipling and
laurenleach
as code owners
WalkthroughAdds internal static validation for SQLSyncer configurations, identifier sanitization and query-variable extraction, per-structure staticValidate implementations, a SQLSyncer.Validate orchestration method, connector-level invocation of per-syncer validation before DB ping, and related unit tests. Changes
sequenceDiagram
participant Connector
participant SQLSyncer
participant Validator as StaticValidator
participant DB as Database
Connector->>SQLSyncer: GetSQLSyncers()
loop per syncer
Connector->>SQLSyncer: Validate(ctx)
SQLSyncer->>Validator: staticValidate on configured sections
Validator->>SQLSyncer: parse queries -> extract vars -> validateVarsInQuery
alt undefined variable or invalid config
Validator-->>SQLSyncer: return validation error
SQLSyncer-->>Connector: propagate error
else all validations pass
SQLSyncer-->>Connector: return nil
end
end
Connector->>DB: PingContext(ctx)
DB-->>Connector: ping result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds validation for SQL queries in the connector configuration to ensure that query variables are properly defined before connector execution. The validation checks that all variables used in queries (except reserved ones like limit, offset, and cursor) are declared in the corresponding Vars map.
Key changes:
- Implements
StaticValidatemethods for various query configuration types to validate variable usage - Integrates validation into the connector's
Validatemethod to run checks during connector initialization - Adds a
queryVarshelper function to extract variable names from query strings
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/bsql/validate.go | Adds validation functions for queries across different configuration types (ListQuery, EntitlementsQuery, GrantsQuery, etc.) to ensure all used variables are defined |
| pkg/bsql/validate_test.go | Adds basic unit tests for query variable validation with valid and invalid cases |
| pkg/bsql/sql_syncer.go | Implements the Validate method on SQLSyncer to orchestrate validation across all configuration fields |
| pkg/connector/connector.go | Integrates query validation into the connector's initialization flow by calling Validate on each syncer |
| pkg/bsql/query.go | Adds queryVars helper function to extract variable names from query strings using regex parsing |
| pkg/bsql/config.go | Adds staticValidator interface and validate:"required" struct tags to query fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/bsql/sql_syncer.go
Outdated
| rsTypeId := s.resourceType.Id | ||
|
|
||
| return fmt.Errorf("validation error for resource type %q, field %q: %w", rsTypeId, field, err) |
There was a problem hiding this comment.
This method has a potential nil pointer dereference bug. When validating actions (lines 95-101), if an error occurs, validateFormatErr is called which accesses s.resourceType.Id. However, NewActionSyncer (line 63-71) creates a syncer with resourceType: nil, which will cause a panic when validating actions with errors.
You should check if resourceType is nil before accessing it:
func (s *SQLSyncer) validateFormatErr(field string, err error) error {
if s.resourceType != nil {
return fmt.Errorf("validation error for resource type %q, field %q: %w", s.resourceType.Id, field, err)
}
return fmt.Errorf("validation error for field %q: %w", field, err)
}| rsTypeId := s.resourceType.Id | |
| return fmt.Errorf("validation error for resource type %q, field %q: %w", rsTypeId, field, err) | |
| if s.resourceType != nil { | |
| return fmt.Errorf("validation error for resource type %q, field %q: %w", s.resourceType.Id, field, err) | |
| } | |
| return fmt.Errorf("validation error for field %q: %w", field, err) |
| func TestValidate(t *testing.T) { | ||
| tcases := []struct { | ||
| name string | ||
| validator staticValidator | ||
| expectErr bool | ||
| }{ | ||
| { | ||
| name: "valid list query", | ||
| validator: &ListQuery{ | ||
| Query: "SELECT * FROM users WHERE id = ?<userid> LIMIT ?<Limit> OFFSET ?<Offset>", | ||
| Vars: map[string]string{ | ||
| "userid": "string", | ||
| }, | ||
| }, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "invalid list query", | ||
| validator: &ListQuery{ | ||
| Query: "SELECT * FROM users WHERE id = ?<unknown> LIMIT ?<Limit> OFFSET ?<Offset>", | ||
| Vars: map[string]string{ | ||
| "userid": "string", | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tcases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx := t.Context() | ||
|
|
||
| syncer := &SQLSyncer{} | ||
|
|
||
| err := tc.validator.StaticValidate(ctx, syncer) | ||
| if tc.expectErr { | ||
| require.Error(t, err) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage is incomplete for the validation functionality. The current test only covers ListQuery validation with two scenarios. Consider adding test cases for:
EntitlementsQuery.StaticValidateEntitlementMapping.StaticValidate(with Grant/Revoke provisioning)GrantsQuery.StaticValidateAccountProvisioning.StaticValidate(Create and Validate queries)CredentialRotation.StaticValidate(Update queries)ActionConfig.StaticValidate(with Vars and Arguments)- Edge cases like empty queries, nil vars, and valid reserved variables (limit, offset, cursor)
pkg/bsql/sql_syncer.go
Outdated
| if err := s.validateInternal(ctx, s.config.StaticEntitlements); err != nil { | ||
| return s.validateFormatErr("static_entitlements", err) |
There was a problem hiding this comment.
The validation logic for StaticEntitlements is incorrect. s.config.StaticEntitlements is of type []*EntitlementMapping (a slice), but validateInternal expects a single item that implements staticValidator. The slice itself doesn't implement this interface, only individual *EntitlementMapping items do.
This code will silently skip validation instead of validating each entitlement mapping. You should iterate through the slice and validate each element:
if s.config.StaticEntitlements != nil {
for i, entitlement := range s.config.StaticEntitlements {
if err := s.validateInternal(ctx, entitlement); err != nil {
return s.validateFormatErr(fmt.Sprintf("static_entitlements[%d]", i), err)
}
}
}| if err := s.validateInternal(ctx, s.config.StaticEntitlements); err != nil { | |
| return s.validateFormatErr("static_entitlements", err) | |
| for i, entitlement := range s.config.StaticEntitlements { | |
| if err := s.validateInternal(ctx, entitlement); err != nil { | |
| return s.validateFormatErr(fmt.Sprintf("static_entitlements[%d]", i), err) | |
| } |
pkg/bsql/sql_syncer.go
Outdated
| if err := s.validateInternal(ctx, s.config.Grants); err != nil { | ||
| return s.validateFormatErr("grants", err) |
There was a problem hiding this comment.
The validation logic for Grants is incorrect. s.config.Grants is of type []*GrantsQuery (a slice), but validateInternal expects a single item that implements staticValidator. The slice itself doesn't implement this interface, only individual *GrantsQuery items do.
This code will silently skip validation instead of validating each grant query. You should iterate through the slice and validate each element:
if s.config.Grants != nil {
for i, grant := range s.config.Grants {
if err := s.validateInternal(ctx, grant); err != nil {
return s.validateFormatErr(fmt.Sprintf("grants[%d]", i), err)
}
}
}| if err := s.validateInternal(ctx, s.config.Grants); err != nil { | |
| return s.validateFormatErr("grants", err) | |
| for i, grant := range s.config.Grants { | |
| if err := s.validateInternal(ctx, grant); err != nil { | |
| return s.validateFormatErr(fmt.Sprintf("grants[%d]", i), err) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/bsql/config.go(4 hunks)pkg/bsql/query.go(1 hunks)pkg/bsql/sql_syncer.go(2 hunks)pkg/bsql/validate.go(1 hunks)pkg/bsql/validate_test.go(1 hunks)pkg/connector/connector.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/validate_test.gopkg/bsql/query.gopkg/bsql/sql_syncer.gopkg/connector/connector.gopkg/bsql/validate.gopkg/bsql/config.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/bsql/validate_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-27T00:53:43.641Z
Learnt from: CR
Repo: ConductorOne/baton-sql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T00:53:43.641Z
Learning: Applies to **/*_test.go : Tests should be table-driven using testify/require; name tests `TestStructName_methodName`
Applied to files:
pkg/bsql/validate_test.go
🧬 Code graph analysis (3)
pkg/bsql/validate_test.go (2)
pkg/bsql/config.go (1)
ListQuery(106-119)pkg/bsql/sql_syncer.go (1)
SQLSyncer(21-28)
pkg/bsql/query.go (1)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(21-28)
pkg/bsql/validate.go (2)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(21-28)pkg/bsql/config.go (7)
ListQuery(106-119)EntitlementsQuery(242-255)EntitlementMapping(258-286)GrantsQuery(310-323)AccountProvisioning(358-367)CredentialRotation(427-432)ActionConfig(434-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (14)
pkg/bsql/config.go (3)
16-18: LGTM!The
staticValidatorinterface provides a clean abstraction for validation logic across different configuration types.
112-112: LGTM!Adding
validate:"required"to theQueryfield ensures that list queries are always defined.
248-248: LGTM!Adding
validate:"required"to theQueryfield ensures that entitlement queries are always defined.pkg/bsql/query.go (1)
98-110: LGTM!The
queryVarsmethod cleanly extracts variable placeholders from SQL queries by reusing the existingparseTokenlogic. The implementation is straightforward and handles errors appropriately.pkg/bsql/validate_test.go (1)
9-51: LGTM!The test follows the table-driven pattern using testify/require as specified in the coding guidelines. It covers both valid and invalid query scenarios appropriately.
Based on learnings, tests should be table-driven using testify/require.
pkg/bsql/sql_syncer.go (2)
73-86: LGTM!The
validateInternalhelper cleanly handles optional validation by checking if the value implements thestaticValidatorinterface before callingStaticValidate.
94-139: Comprehensive validation coverage.The
Validatemethod appropriately validates all configuration sections (Actions, List, Entitlements, Grants, etc.) using thestaticValidatorinterface and wraps errors with contextual information.pkg/connector/connector.go (1)
79-93: LGTM!The validation flow correctly validates each syncer before performing the database health check. The type assertion pattern safely checks if syncers implement the
Validatemethod without requiring all syncers to implement it.pkg/bsql/validate.go (6)
34-40: LGTM!The
StaticValidateimplementations forListQueryandEntitlementsQuerycorrectly delegate tovalidateVarsInQuery.
42-66: LGTM!The
EntitlementMapping.StaticValidatemethod properly validates both Grant and Revoke queries when provisioning is configured, using the provisioning-level vars.
68-70: LGTM!The
GrantsQuery.StaticValidatecorrectly delegates tovalidateVarsInQuery.
72-90: LGTM!The
AccountProvisioning.StaticValidatemethod appropriately validates both Create and Validate query configurations.
92-103: LGTM!The
CredentialRotation.StaticValidatecorrectly validates Update queries.
105-116: LGTM!The
ActionConfig.StaticValidatemethod correctly aggregates variables from bothVarsandArgumentsbefore validation, ensuring that argument names are treated as available variables in the query.
pkg/bsql/config.go
Outdated
| DSN string `yaml:"dsn" json:"dsn" validate:"required"` | ||
|
|
||
| // Structured connection fields (optional, override DSN components when set) | ||
|
|
||
| // Scheme is the database type (e.g., "postgres", "mysql", "sqlserver", "oracle", "hdb") | ||
| Scheme string `yaml:"scheme" json:"scheme"` | ||
| Scheme string `yaml:"scheme" json:"scheme" validate:"required"` | ||
|
|
||
| // Host is the database server hostname or IP address (may include port for some databases) | ||
| Host string `yaml:"host" json:"host"` | ||
| Host string `yaml:"host" json:"host" validate:"required"` | ||
|
|
||
| // Port is the database server port number | ||
| Port string `yaml:"port" json:"port"` | ||
| Port string `yaml:"port" json:"port" validate:"required"` | ||
|
|
||
| // Database is the name of the database to connect to | ||
| Database string `yaml:"database" json:"database"` | ||
| Database string `yaml:"database" json:"database" validate:"required"` | ||
|
|
||
| // User is the database username used for authentication | ||
| User string `yaml:"user" json:"user"` | ||
| User string `yaml:"user" json:"user" validate:"required"` | ||
|
|
||
| // Password is the database password used for authentication | ||
| Password string `yaml:"password" json:"password"` | ||
| Password string `yaml:"password" json:"password" validate:"required"` | ||
|
|
There was a problem hiding this comment.
Validation tags conflict with documented behavior.
All DatabaseConfig fields are marked validate:"required", but the documentation (lines 42-48) explicitly states:
- DSN is "optional if using structured fields"
- Structured fields are "optional, override DSN components when set"
Making all fields required prevents users from using either DSN-only or structured-field-only configurations as documented.
Consider one of these approaches:
- Remove the
validate:"required"tags and implement custom validation logic that checks for either DSN or complete structured fields - Update the documentation to reflect that all fields are actually required
- Use conditional validation to require either DSN OR all structured fields
Example custom validation approach:
func (d *DatabaseConfig) Validate() error {
if d.DSN == "" && (d.Scheme == "" || d.Host == "" || d.Port == "" || d.Database == "" || d.User == "" || d.Password == "") {
return fmt.Errorf("either DSN or all structured fields (scheme, host, port, database, user, password) must be provided")
}
return nil
}🤖 Prompt for AI Agents
In pkg/bsql/config.go around lines 49 to 70, the struct fields are all marked
`validate:"required"` which conflicts with the documented behavior that DSN or
the structured fields may be used independently; remove the
`validate:"required"` tags from these fields and implement a `Validate() error`
method on DatabaseConfig that enforces the rule "either DSN is provided or all
structured fields (scheme, host, port, database, user, password) are present"
(return a clear error message otherwise), and ensure callers that load this
config invoke that Validate() method after unmarshalling.
| func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { | ||
| if query == "" { | ||
| return fmt.Errorf("list query is required") | ||
| } | ||
|
|
||
| usedVars, err := s.queryVars(query) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse list query for variables: %w", err) | ||
| } | ||
|
|
||
| if vars == nil { | ||
| vars = make(map[string]string) | ||
| } | ||
|
|
||
| for _, v := range usedVars { | ||
| if _, ok := vars[v]; !ok { | ||
| if v == "limit" || v == "offset" || v == "cursor" { | ||
| continue | ||
| } | ||
| return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Error messages are too specific.
The validateVarsInQuery function is used for validating all query types (list, entitlements, grants, provisioning, etc.), but the error messages specifically mention "list query" (lines 10, 15, and 27), which is misleading when validating other query types.
Make the error messages generic:
func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error {
if query == "" {
- return fmt.Errorf("list query is required")
+ return fmt.Errorf("query is required")
}
usedVars, err := s.queryVars(query)
if err != nil {
- return fmt.Errorf("failed to parse list query for variables: %w", err)
+ return fmt.Errorf("failed to parse query for variables: %w", err)
}
if vars == nil {
vars = make(map[string]string)
}
for _, v := range usedVars {
if _, ok := vars[v]; !ok {
if v == "limit" || v == "offset" || v == "cursor" {
continue
}
- return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v)
+ return fmt.Errorf("query uses variable '%s' which is not defined in vars", v)
}
}
return 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 validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { | |
| if query == "" { | |
| return fmt.Errorf("list query is required") | |
| } | |
| usedVars, err := s.queryVars(query) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse list query for variables: %w", err) | |
| } | |
| if vars == nil { | |
| vars = make(map[string]string) | |
| } | |
| for _, v := range usedVars { | |
| if _, ok := vars[v]; !ok { | |
| if v == "limit" || v == "offset" || v == "cursor" { | |
| continue | |
| } | |
| return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) | |
| } | |
| } | |
| return nil | |
| } | |
| func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { | |
| if query == "" { | |
| return fmt.Errorf("query is required") | |
| } | |
| usedVars, err := s.queryVars(query) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse query for variables: %w", err) | |
| } | |
| if vars == nil { | |
| vars = make(map[string]string) | |
| } | |
| for _, v := range usedVars { | |
| if _, ok := vars[v]; !ok { | |
| if v == "limit" || v == "offset" || v == "cursor" { | |
| continue | |
| } | |
| return fmt.Errorf("query uses variable '%s' which is not defined in vars", v) | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In pkg/bsql/validate.go around lines 8 to 32, the error messages reference "list
query" but this function validates any query type; change the three messages to
be generic (e.g., "query is required", "failed to parse query for variables:
%w", and "query uses variable '%s' which is not defined in vars") so they no
longer mention "list"; keep the same formatting and error wrapping semantics and
do not alter the validation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/bsql/validate.go (1)
9-33: MakevalidateVarsInQueryerror messages query-type agnostic.This helper is used for multiple query types, but the messages currently say "list query", which is misleading for other callers; make the wording generic while keeping behavior identical.
func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { if query == "" { - return fmt.Errorf("list query is required") + return fmt.Errorf("query is required") } usedVars, err := s.queryVars(query) if err != nil { - return fmt.Errorf("failed to parse list query for variables: %w", err) + return fmt.Errorf("failed to parse query for variables: %w", err) } @@ for _, v := range usedVars { if _, ok := vars[v]; !ok { if v == "limit" || v == "offset" || v == "cursor" { continue } - return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) + return fmt.Errorf("query uses variable '%s' which is not defined in vars", v) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/bsql/validate.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/validate.go
🪛 GitHub Actions: ci
pkg/bsql/validate.go
[error] 73-73: golangci-lint: unnecessary leading newline (whitespace)
🪛 GitHub Check: go-lint
pkg/bsql/validate.go
[failure] 73-73:
unnecessary leading newline (whitespace)
🔇 Additional comments (6)
pkg/bsql/validate.go (6)
35-37:ListQuery.StaticValidatecleanly reuses shared validation.The method is a thin, clear delegation to
validateVarsInQuery, which keeps list-query validation logic centralized and consistent.
39-41:EntitlementsQuery.StaticValidatecorrectly delegates to common validator.Same pattern as
ListQuery, so entitlements queries get the same variable checking semantics via the shared helper.
43-67:EntitlementMapping.StaticValidatesafely handles nil provisioning and validates grant/revoke queries.Early return on
l.Provisioning == nilavoids nil dereferences, and the loops overGrant.QueriesandRevoke.Queriescorrectly reusel.Provisioning.Varsfor variable validation.
69-71:GrantsQuery.StaticValidatewiring looks good.Delegating directly to
validateVarsInQuerymatches the pattern used for list and entitlements queries and keeps behavior aligned.
122-133:CredentialRotation.StaticValidatecorrectly guards onUpdateand validates queries.The method safely no-ops when
l.Updateis nil and validates each update query via the shared helper when present.
135-146:ActionConfig.StaticValidatecorrectly mergesVarsandArgumentsbefore validation.Building
availableVarsfrom both maps and then callingvalidateVarsInQueryensures action queries see the full set of allowed variables, and nil maps are handled safely.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/bsql/validate.go (2)
9-33: Error messages remain too specific.The error messages on lines 11, 16, and 28 still reference "list query" even though this function validates all query types. This was previously flagged and remains unresolved.
Apply this diff to fix the error messages:
func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { if query == "" { - return fmt.Errorf("list query is required") + return fmt.Errorf("query is required") } usedVars, err := s.queryVars(query) if err != nil { - return fmt.Errorf("failed to parse list query for variables: %w", err) + return fmt.Errorf("failed to parse query for variables: %w", err) } if vars == nil { vars = make(map[string]string) } for _, v := range usedVars { if _, ok := vars[v]; !ok { if v == limitKey || v == offsetKey || v == cursorKey { continue } - return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) + return fmt.Errorf("query uses variable '%s' which is not defined in vars", v) } } return nil }
73-74: Remove unnecessary leading newline.The blank line after the function signature violates the golangci-lint whitespace rule. This was previously flagged and remains unresolved.
Apply this diff:
func (l *AccountProvisioning) StaticValidate(ctx context.Context, s *SQLSyncer) error { - if l.Credentials == nil {Based on pipeline failures.
🧹 Nitpick comments (1)
pkg/bsql/query_test.go (1)
68-79: Consider migrating to testify/require for consistency with coding guidelines.While the test logic is sound, the coding guidelines specify that tests should use testify/require. The current test uses plain testing assertions (
t.Errorf). This applies to the entire test file, not just the new test case.Example migration pattern:
import ( "github.com/stretchr/testify/require" ) // In test body: require.NoError(t, err) require.Equal(t, tt.want, got)As per coding guidelines, tests should be table-driven using testify/require.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/bsql/query.go(4 hunks)pkg/bsql/query_test.go(1 hunks)pkg/bsql/validate.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/bsql/query.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/query_test.gopkg/bsql/validate.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/bsql/query_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-27T00:53:43.641Z
Learnt from: CR
Repo: ConductorOne/baton-sql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T00:53:43.641Z
Learning: Applies to **/*_test.go : Tests should be table-driven using testify/require; name tests `TestStructName_methodName`
Applied to files:
pkg/bsql/query_test.go
🧬 Code graph analysis (1)
pkg/bsql/query_test.go (1)
pkg/database/database.go (1)
MySQL(27-27)
🪛 GitHub Check: go-lint
pkg/bsql/validate.go
[failure] 73-73:
unnecessary leading newline (whitespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (1)
pkg/bsql/query_test.go (1)
361-377: LGTM! Excellent security test.This test case correctly validates that SQL injection attempts via unquoted table names are neutralized through identifier sanitization. The expected output (
example_tableDROPTABLEusers) confirms that all special characters (;, spaces,--) are stripped, preventing the injection attack while preserving alphanumeric characters.
pkg/bsql/validate.go
Outdated
| func (l *ListQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| return validateVarsInQuery(s, l.Query, l.Vars) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add godoc comments for exported methods.
All seven exported StaticValidate methods lack documentation. Each should have a godoc comment describing what it validates.
As per coding guidelines, comments for exported items must be complete sentences ending with periods.
Example for ListQuery.StaticValidate:
+// StaticValidate verifies that all variables used in the query are defined in Vars.
func (l *ListQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error {
return validateVarsInQuery(s, l.Query, l.Vars)
}Apply similar documentation to the remaining methods:
EntitlementsQuery.StaticValidate(line 39)EntitlementMapping.StaticValidate(line 43)GrantsQuery.StaticValidate(line 69)AccountProvisioning.StaticValidate(line 73)CredentialRotation.StaticValidate(line 122)ActionConfig.StaticValidate(line 135)
Also applies to: 39-41, 43-67, 69-71, 73-120, 122-133, 135-146
🤖 Prompt for AI Agents
In pkg/bsql/validate.go around lines 35-37, 39-41, 43-67, 69-71, 73-120,
122-133, and 135-146, each exported StaticValidate method is missing godoc; add
a one-sentence godoc comment immediately above each method (e.g.,
"ListQuery.StaticValidate validates that the query and provided variables are
consistent and returns an error if not.") that describes what the method
validates and ends with a period; ensure the comment names the receiver/type and
uses a full sentence for each of: ListQuery.StaticValidate,
EntitlementsQuery.StaticValidate, EntitlementMapping.StaticValidate,
GrantsQuery.StaticValidate, AccountProvisioning.StaticValidate,
CredentialRotation.StaticValidate, and ActionConfig.StaticValidate.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func (l *EntitlementsQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| return validateVarsInQuery(s, l.Query, l.Vars) |
There was a problem hiding this comment.
Missing validation of EntitlementsQuery.Map field. The EntitlementsQuery.StaticValidate method only validates the query variables but doesn't validate the Map field, which is a slice of EntitlementMapping objects. Each EntitlementMapping has provisioning queries that should also be validated.
Consider adding validation for the Map field:
func (l *EntitlementsQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error {
if err := validateVarsInQuery(s, l.Query, l.Vars); err != nil {
return err
}
for i, mapping := range l.Map {
if mapping != nil {
if err := mapping.StaticValidate(ctx, s); err != nil {
return fmt.Errorf("map[%d]: %w", i, err)
}
}
}
return nil
}| return validateVarsInQuery(s, l.Query, l.Vars) | |
| if err := validateVarsInQuery(s, l.Query, l.Vars); err != nil { | |
| return err | |
| } | |
| for i, mapping := range l.Map { | |
| if mapping != nil { | |
| if err := mapping.StaticValidate(ctx, s); err != nil { | |
| return fmt.Errorf("map[%d]: %w", i, err) | |
| } | |
| } | |
| } | |
| return nil |
pkg/bsql/validate.go
Outdated
| func (l *EntitlementsQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| return validateVarsInQuery(s, l.Query, l.Vars) | ||
| } | ||
|
|
||
| func (l *EntitlementMapping) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| if l.Provisioning == nil { | ||
| return nil | ||
| } | ||
|
|
||
| if l.Provisioning.Grant != nil { | ||
| for _, query := range l.Provisioning.Grant.Queries { | ||
| err := validateVarsInQuery(s, query, l.Provisioning.Vars) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if l.Provisioning.Revoke != nil { | ||
| for _, query := range l.Provisioning.Revoke.Queries { | ||
| err := validateVarsInQuery(s, query, l.Provisioning.Vars) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (l *GrantsQuery) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| return validateVarsInQuery(s, l.Query, l.Vars) | ||
| } | ||
|
|
||
| func (l *AccountProvisioning) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
|
|
||
| if l.Credentials == nil { | ||
| return errors.New("no credentials defined") | ||
| } | ||
|
|
||
| if l.Credentials.EncryptedPassword == nil && | ||
| l.Credentials.RandomPassword == nil && | ||
| l.Credentials.NoPassword == nil { | ||
| return errors.New("no credential method defined") | ||
| } | ||
|
|
||
| if l.Credentials.RandomPassword != nil { | ||
| if l.Credentials.RandomPassword.MaxLength <= 0 { | ||
| return errors.New("random password max_length must be greater than zero") | ||
| } | ||
|
|
||
| if l.Credentials.RandomPassword.MinLength <= 0 { | ||
| return errors.New("random password min_length must be greater than zero") | ||
| } | ||
|
|
||
| if l.Credentials.RandomPassword.MinLength > l.Credentials.RandomPassword.MaxLength { | ||
| return errors.New("random password min_length cannot be greater than max_length") | ||
| } | ||
| } | ||
|
|
||
| if l.Create == nil { | ||
| return errors.New("no create functions defined") | ||
| } | ||
|
|
||
| for _, query := range l.Create.Queries { | ||
| err := validateVarsInQuery(s, query, l.Create.Vars) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if l.Validate == nil { | ||
| return errors.New("no validate functions defined") | ||
| } | ||
|
|
||
| err := validateVarsInQuery(s, l.Validate.Query, l.Validate.Vars) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (l *CredentialRotation) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| if l.Update != nil { | ||
| for _, query := range l.Update.Queries { | ||
| err := validateVarsInQuery(s, query, l.Update.Vars) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (l *ActionConfig) StaticValidate(ctx context.Context, s *SQLSyncer) error { | ||
| availableVars := make(map[string]string) | ||
| for k, v := range l.Vars { | ||
| availableVars[k] = v | ||
| } | ||
|
|
||
| for k, config := range l.Arguments { | ||
| availableVars[k] = config.Name | ||
| } | ||
|
|
||
| return validateVarsInQuery(s, l.Query, availableVars) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for newly added validation methods. The test file only covers ListQuery validation, but the new validation logic includes several other important validators:
EntitlementsQuery.StaticValidateEntitlementMapping.StaticValidateGrantsQuery.StaticValidateAccountProvisioning.StaticValidate(with complex credential and password length validation)CredentialRotation.StaticValidateActionConfig.StaticValidate
Consider adding test cases for these validators to ensure they properly catch configuration errors, especially the AccountProvisioning validator which has multiple validation rules.
pkg/bsql/validate.go
Outdated
| } | ||
|
|
||
| for k, config := range l.Arguments { | ||
| availableVars[k] = config.Name |
There was a problem hiding this comment.
[nitpick] Semantically incorrect value assignment in ActionConfig.StaticValidate. At line 142, the code assigns config.Name (the argument's display name) as the value in availableVars, but should use config.Type for consistency with how Vars maps are defined elsewhere (e.g., ListQuery.Vars at line 109 of config.go is map[string]string where values are types).
While this doesn't affect functionality (since validateVarsInQuery only checks key existence), it's semantically incorrect:
for k, config := range l.Arguments {
availableVars[k] = config.Type // Use Type instead of Name
}| availableVars[k] = config.Name | |
| availableVars[k] = config.Type |
pkg/bsql/config.go
Outdated
|
|
||
| // Query is the SQL statement used to fetch dynamic entitlements. | ||
| Query string `yaml:"query" json:"query"` | ||
| Query string `yaml:"query" json:"query" validate:"required"` |
There was a problem hiding this comment.
[nitpick] Added validate:"required" struct tags appear to be unused. These tags are typically used with validation libraries like go-playground/validator, but there's no evidence of such a library being used in the codebase to process these tags. The actual validation is done through the StaticValidate methods, not through struct tag validation.
If these tags are not intended to be used by a validation library, they should be removed to avoid confusion. If they are intended for future use, that should be documented.
| Query string `yaml:"query" json:"query" validate:"required"` | |
| Query string `yaml:"query" json:"query"` |
pkg/bsql/validate.go
Outdated
|
|
||
| func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { | ||
| if query == "" { | ||
| return fmt.Errorf("list query is required") |
There was a problem hiding this comment.
The error message "list query is required" is misleading. This function validateVarsInQuery is used for various types of queries (list, entitlements, grants, provisioning, etc.), not just list queries. The error message should be more generic.
Consider changing to:
return fmt.Errorf("query is required")| return fmt.Errorf("list query is required") | |
| return fmt.Errorf("query is required") |
pkg/bsql/validate.go
Outdated
|
|
||
| usedVars, err := s.queryVars(query) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse list query for variables: %w", err) |
There was a problem hiding this comment.
The error message "failed to parse list query for variables" is misleading. This function is reused for various types of queries (list, entitlements, grants, provisioning, etc.), not just list queries. The error message should be more generic.
Consider changing to:
return fmt.Errorf("failed to parse query for variables: %w", err)| return fmt.Errorf("failed to parse list query for variables: %w", err) | |
| return fmt.Errorf("failed to parse query for variables: %w", err) |
pkg/bsql/validate.go
Outdated
| if v == limitKey || v == offsetKey || v == cursorKey { | ||
| continue | ||
| } | ||
| return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) |
There was a problem hiding this comment.
The error message "list query uses variable" is misleading. This function is reused for various types of queries (list, entitlements, grants, provisioning, etc.), not just list queries. The error message should be more generic.
Consider changing to:
return fmt.Errorf("query uses variable '%s' which is not defined in vars", v)| return fmt.Errorf("list query uses variable '%s' which is not defined in vars", v) | |
| return fmt.Errorf("query uses variable '%s' which is not defined in vars", v) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/bsql/config.go
Outdated
| // Supports environment variable expansion via ${VAR_NAME} syntax. | ||
| // Example: "postgres://${DB_HOST}:${DB_PORT}/${DB_DATABASE}?sslmode=disable" | ||
| DSN string `yaml:"dsn" json:"dsn"` | ||
| DSN string `yaml:"dsn" json:"dsn" validate:"required"` |
There was a problem hiding this comment.
DSN is required only if the other fields aren't, and vice-versa.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/bsql/sql_syncer.go (1)
73-84: Consider simplifying the method body.The error handling can be condensed since you're just returning the error directly.
🔎 Proposed simplification
func (s *SQLSyncer) validateInternal(ctx context.Context, validator staticValidator) error { if validator == nil { return nil } - - err := validator.staticValidate(ctx, s) - if err != nil { - return err - } - - return nil + return validator.staticValidate(ctx, s) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/bsql/sql_syncer.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/sql_syncer.go
🧬 Code graph analysis (1)
pkg/bsql/sql_syncer.go (1)
pkg/bsql/config.go (2)
AccountProvisioning(358-367)CredentialRotation(427-432)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (4)
pkg/bsql/sql_syncer.go (4)
3-12: LGTM!Import organization follows the coding guidelines: standard library first (alphabetized), then third-party packages.
86-92: LGTM!The nil check for
s.resourceTypeproperly handles action syncers created withresourceType: nil, addressing the previously identified potential panic.
94-112: LGTM!The validation logic for
Actions,List, andEntitlementsis correct. The action key is properly included in error messages for better debugging.
130-143: LGTM!The validation for
AccountProvisioningandCredentialRotationcorrectly delegates tovalidateInternalwith appropriate nil guards.
Description
Useful links:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.