test: verify db_owner wiring and CreateUserRole with built-in PG roles#345
Merged
rshoemaker merged 1 commit intomainfrom Apr 15, 2026
Merged
Conversation
Add TestSpec_NodeInstances_DBOwner to verify the db_owner → ALTER DATABASE OWNER TO chain: single owner propagation, multiple owner rejection, empty owner handling, and multi-node propagation. Add TestCreateUserRole_BuiltinPgRoles to verify that CreateUserRole correctly generates GRANT statements for built-in PostgreSQL roles like pg_read_all_data, pg_read_all_settings, and pg_read_all_stats.
📝 WalkthroughWalkthroughAdded comprehensive test coverage for database owner propagation in Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/database/spec_test.go (1)
527-567: Userequirefor precondition assertions to avoid panic-masked failures.At Lines 533, 543, 553, and 563-564,
assert.*is non-fatal; subsequentnodes[0],nodes[1], anderr.Error()can panic if preconditions fail. Preferrequire.*for those gates.Suggested change
import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" @@ t.Run("single db_owner is propagated", func(t *testing.T) { @@ nodes, err := spec.NodeInstances() - assert.NoError(t, err) + require.NoError(t, err) + require.Len(t, nodes, 1) assert.Equal(t, "app", nodes[0].DatabaseOwner) }) @@ t.Run("no db_owner results in empty owner", func(t *testing.T) { @@ nodes, err := spec.NodeInstances() - assert.NoError(t, err) + require.NoError(t, err) + require.Len(t, nodes, 1) assert.Empty(t, nodes[0].DatabaseOwner) }) @@ t.Run("multiple db_owners returns error", func(t *testing.T) { @@ _, err := spec.NodeInstances() - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "only one user can have db_owner=true") }) @@ t.Run("owner propagates to all nodes", func(t *testing.T) { @@ nodes, err := spec.NodeInstances() - assert.NoError(t, err) - assert.Len(t, nodes, 2) + require.NoError(t, err) + require.Len(t, nodes, 2) assert.Equal(t, "app", nodes[0].DatabaseOwner) assert.Equal(t, "app", nodes[1].DatabaseOwner) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec_test.go` around lines 527 - 567, The tests use non-fatal assert.* checks before indexing nodes or calling err.Error(), which can mask failures and cause panics; in the four subtests ("single db_owner is propagated", "no db_owner results in empty owner", "multiple db_owners returns error", "owner propagates to all nodes") replace the gating assertions with require.* so failures stop the test before accessing nodes/err. Concretely, use require.NoError(t, err) instead of assert.NoError, use require.Len/require.Empty/require.Equal where the test indexes nodes or reads values (e.g. nodes[0], nodes[1]) and use require.Error + require.Contains (or require.Contains on err.Error()) in the multiple-owner case; update calls around the NodeInstances(), minimalSpec(...) and nodes checks accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/database/spec_test.go`:
- Around line 527-567: The tests use non-fatal assert.* checks before indexing
nodes or calling err.Error(), which can mask failures and cause panics; in the
four subtests ("single db_owner is propagated", "no db_owner results in empty
owner", "multiple db_owners returns error", "owner propagates to all nodes")
replace the gating assertions with require.* so failures stop the test before
accessing nodes/err. Concretely, use require.NoError(t, err) instead of
assert.NoError, use require.Len/require.Empty/require.Equal where the test
indexes nodes or reads values (e.g. nodes[0], nodes[1]) and use require.Error +
require.Contains (or require.Contains on err.Error()) in the multiple-owner
case; update calls around the NodeInstances(), minimalSpec(...) and nodes checks
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7164268-f773-49ff-beac-51a2062f287c
📒 Files selected for processing (2)
server/internal/database/spec_test.goserver/internal/postgres/roles_test.go
mmols
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
db_ownerandCreateUserRolewith built-in PostgreSQL roles work as expected for theconnect_asdesignTest plan
TestSpec_NodeInstances_DBOwner— single owner propagated, no owner is empty, multiple owners rejected, owner propagates to all nodesTestCreateUserRole_BuiltinPgRoles—pg_read_all_data,pg_read_all_settings,pg_read_all_statsproduce correct GRANT statementsPLAT-550
PLAT-551