feat: validate conf libraries contain spock#411
Conversation
📝 WalkthroughWalkthroughA new helper Changesspock Library Validation
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/internal/api/apiv1/validate.go`:
- Line 302: The validateConfLibraries call for PostgreSQL configuration
validation is using validation.NewPath("postgresql_conf") which creates a fresh
path without node context, causing validation errors to lose the node-scoped
context. Replace validation.NewPath("postgresql_conf") with
path.Append("postgresql_conf") to maintain the proper node context (nodes[i]...)
in the validation error paths when PostgreSQL configuration validation occurs at
the node level.
- Around line 45-49: The key lookup in validateConfLibraries uses case-sensitive
comparison for "shared_preload_libraries", but PostgreSQL GUC names are
case-insensitive, allowing mixed-case variants to bypass the spock safeguard.
Update the key lookup logic to handle case-insensitivity similar to how
validateAuthFileGUCs already does it, ensuring that the validation properly
catches any case variation of the shared_preload_libraries key before returning
nil defaults.
🪄 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: CHILL
Plan: Pro
Run ID: 2ced2b17-b9d9-4176-9216-277eb18a17b6
📒 Files selected for processing (1)
server/internal/api/apiv1/validate.go
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
This implementation is looking good! Could you please also add some tests for this function to server/internal/api/apiv1/validate_test.go. I'm thinking maybe:
- Add a
TestValidateConfLibrariesto test this all the cases of this function individually - Just to validate that it's being called in the right place, add an invalid
PostgresqlConfand expected error message to theinvalidtest cases in bothTestValidateDatabaseSpecandTestValidateNode
| } | ||
|
|
||
| func validateConfLibraries(conf map[string]any, path validation.Path) []error { | ||
| for key, val := range conf { |
There was a problem hiding this comment.
Could you please add a comment here noting that we can't do a simple lookup because parameter names are case-insensitive? Your implementation is good, and I want to make sure we don't accidentally change it in the future.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/internal/api/apiv1/validate_test.go (2)
498-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-string
shared_preload_librariesbranch in helper tests.
validateConfLibrariesalso returns an error whenshared_preload_librariesis present but not a string. Adding that case here will protect the full helper contract.Proposed test-table addition
func TestValidateConfLibraries(t *testing.T) { for _, tc := range []struct { name string conf map[string]any expected []string }{ @@ { + name: "invalid conf non-string shared_preload_libraries", + conf: map[string]any{"shared_preload_libraries": 123}, + expected: []string{ + `"shared_preload_libraries" must be a string`, + }, + }, + { name: "invalid conf missing spock", conf: map[string]any{"shared_preload_libraries": "snowflake"}, expected: []string{ `"spock" must be included in shared_preload_libraries`, }, },🤖 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 `@server/internal/api/apiv1/validate_test.go` around lines 498 - 539, The TestValidateConfLibraries test table does not cover the case where shared_preload_libraries is present but not a string value. Add a new test case to the table that sets shared_preload_libraries to a non-string type (such as an integer or boolean) and include the expected error message that validateConfLibraries returns when encountering a non-string value for this configuration parameter. This will ensure full coverage of the validateConfLibraries function's error handling logic.
1410-1426: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a spec-level create-path case in
TestValidateDatabaseSpec.This new case covers node-level
postgresql_conf, butvalidateDatabaseSpecalso validatesspec.PostgresqlConfdirectly. A spec-level missing-spockcase here would complete create-path coverage.Proposed test-table addition
{ + name: "invalid spec-level lib conf", + spec: &api.DatabaseSpec{ + PostgresqlConf: map[string]any{"shared_preload_libraries": "snowflake"}, + Nodes: []*api.DatabaseNodeSpec{ + { + Name: "n1", + HostIds: []api.Identifier{api.Identifier("host-1")}, + }, + }, + }, + expected: []string{ + `"spock" must be included in shared_preload_libraries`, + }, + }, + { name: "invalid lib conf", spec: &api.DatabaseSpec{ Nodes: []*api.DatabaseNodeSpec{ { Name: "n1",🤖 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 `@server/internal/api/apiv1/validate_test.go` around lines 1410 - 1426, The current test case at the node-level PostgresqlConf is incomplete because validateDatabaseSpec also validates spec-level PostgresqlConf directly. Add a new test case to the TestValidateDatabaseSpec test table with a DatabaseSpec that has PostgresqlConf (at the spec level, not nested under Nodes) containing shared_preload_libraries without "spock". The test case should expect the same validation error message as the node-level case, ensuring spec-level PostgresqlConf validation is covered in the create-path tests.
🤖 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.
Nitpick comments:
In `@server/internal/api/apiv1/validate_test.go`:
- Around line 498-539: The TestValidateConfLibraries test table does not cover
the case where shared_preload_libraries is present but not a string value. Add a
new test case to the table that sets shared_preload_libraries to a non-string
type (such as an integer or boolean) and include the expected error message that
validateConfLibraries returns when encountering a non-string value for this
configuration parameter. This will ensure full coverage of the
validateConfLibraries function's error handling logic.
- Around line 1410-1426: The current test case at the node-level PostgresqlConf
is incomplete because validateDatabaseSpec also validates spec-level
PostgresqlConf directly. Add a new test case to the TestValidateDatabaseSpec
test table with a DatabaseSpec that has PostgresqlConf (at the spec level, not
nested under Nodes) containing shared_preload_libraries without "spock". The
test case should expect the same validation error message as the node-level
case, ensuring spec-level PostgresqlConf validation is covered in the
create-path tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 103e3110-f41b-4565-b8ab-09f4cba0c15b
📒 Files selected for processing (2)
server/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/api/apiv1/validate.go
2 participants
Summary
Prevent users from disabling the spock extension via postgresql_conf.shared_preload_libraries. The control plane has a hard dependency on spock, so removing it would silently break replication.
Changes
Add validateConfLibraries to reject any postgresql_conf that explicitly sets shared_preload_libraries without including spock
Wire into validateDatabaseSpec (spec-level conf) and validateNode (node-level conf) for the create path
Wire into validateDatabaseUpdate for the update path, at both spec and node level
Testing
TestValidateConfLibraries: unit tests covering absent key, valid configs, uppercase param name, and missing spock
Invalid postgresql_conf cases added to TestValidateDatabaseSpec and TestValidateNode to confirm the check is called in the right places
TestValidateDatabaseUpdate_LibraryConf: confirms the check runs on update requests at both spec and node level