-
Notifications
You must be signed in to change notification settings - Fork 1
Add validation for queries #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
8dedbc9
b5d5860
3b04ca4
6597e7f
282b6f7
dbc7a21
f5aea0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,10 @@ import ( | |||||
| v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" | ||||||
| ) | ||||||
|
|
||||||
| type staticValidator interface { | ||||||
| StaticValidate(ctx context.Context, s *SQLSyncer) error | ||||||
| } | ||||||
|
|
||||||
| // Config represents the overall connector configuration. | ||||||
| type Config struct { | ||||||
| // AppName is the application name that identifies the connector. | ||||||
|
|
@@ -42,27 +46,27 @@ type DatabaseConfig struct { | |||||
| // DSN is the Database Source Name connection string (optional if using structured fields). | ||||||
| // 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"` | ||||||
|
|
||||||
| // 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"` | ||||||
|
|
||||||
|
||||||
| // Params contains additional connection parameters (e.g., {"sslmode": "disable", "timeout": "30s"}) | ||||||
| Params map[string]string `yaml:"params" json:"params"` | ||||||
|
|
@@ -105,7 +109,7 @@ type ListQuery struct { | |||||
| Vars map[string]string `yaml:"vars,omitempty" json:"vars,omitempty"` | ||||||
|
|
||||||
| // Query is the SQL statement used to fetch a list of resources. | ||||||
| Query string `yaml:"query" json:"query"` | ||||||
| Query string `yaml:"query" json:"query" validate:"required"` | ||||||
|
|
||||||
| // Pagination defines the pagination strategy and settings for the list query. | ||||||
| Pagination *Pagination `yaml:"pagination" json:"pagination"` | ||||||
|
|
@@ -241,7 +245,7 @@ type EntitlementsQuery struct { | |||||
| Vars map[string]string `yaml:"vars,omitempty" json:"vars,omitempty"` | ||||||
|
|
||||||
| // Query is the SQL statement used to fetch dynamic entitlements. | ||||||
| Query string `yaml:"query" json:"query"` | ||||||
| Query string `yaml:"query" json:"query" validate:"required"` | ||||||
|
||||||
| Query string `yaml:"query" json:"query" validate:"required"` | |
| Query string `yaml:"query" json:"query"` |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ package bsql | |||||||||||||||
| import ( | ||||||||||||||||
| "context" | ||||||||||||||||
| "database/sql" | ||||||||||||||||
| "fmt" | ||||||||||||||||
|
|
||||||||||||||||
| v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" | ||||||||||||||||
| "github.com/conductorone/baton-sdk/pkg/connectorbuilder" | ||||||||||||||||
|
|
@@ -68,3 +69,71 @@ func NewActionSyncer(ctx context.Context, db *sql.DB, dbEngine database.DbEngine | |||||||||||||||
| fullConfig: fullConfig, | ||||||||||||||||
| }, nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func (s *SQLSyncer) validateInternal(ctx context.Context, anyV any) error { | ||||||||||||||||
| if anyV == nil { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if v, ok := anyV.(staticValidator); ok { | ||||||||||||||||
| err := v.StaticValidate(ctx, s) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func (s *SQLSyncer) validateFormatErr(field string, err error) error { | ||||||||||||||||
| rsTypeId := s.resourceType.Id | ||||||||||||||||
MarcusGoldschmidt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||
|
|
||||||||||||||||
| return fmt.Errorf("validation error for resource type %q, field %q: %w", rsTypeId, 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) |
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
| } |
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,146 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package bsql | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func validateVarsInQuery(s *SQLSyncer, query string, vars map[string]string) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if query == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("list query is required") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("list query is required") | |
| return fmt.Errorf("query is required") |
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 |
Outdated
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package bsql | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| 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) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
Comment on lines
9
to
51
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DSN is required only if the other fields aren't, and vice-versa.