feat: require db_anon_role for PostgREST, remove pgedge_application_r…#361
Merged
moizpgedge merged 3 commits intomainfrom Apr 22, 2026
Conversation
…ead_only default `desiredAnonRole()` previously fell back to `pgedge_application_read_only` when no anon role was supplied, blocking removal of the deprecated built-in roles in PLAT-559. `db_anon_role` is now required at validation time. Omitting it returns a 400 with `db_anon_role is required`. All tests updated accordingly. PLAT-568
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request makes Changes
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 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
|
| 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. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/internal/database/postgrest_service_config.go (1)
11-16:⚠️ Potential issue | 🟡 MinorUpdate the optional-fields comments for
db_anon_role.Lines 13 and 37 still describe every field as optional/defaulted, but
db_anon_roleis now required by Lines 56-69.📝 Proposed comment update
// PostgRESTServiceConfig is the typed internal representation of PostgREST service // configuration. Parsed from ServiceSpec.Config map[string]any. All fields are -// optional; defaults are applied when absent. +// optional unless noted; defaults are applied when absent. type PostgRESTServiceConfig struct { DBSchemas string `json:"db_schemas"` // default: "public" - DBAnonRole string `json:"db_anon_role"` + DBAnonRole string `json:"db_anon_role"` // required DBPool int `json:"db_pool"` // default: 10, range: 1-30// ParsePostgRESTServiceConfig parses and validates a config map into a typed -// PostgRESTServiceConfig. All fields are optional with sensible defaults. +// PostgRESTServiceConfig. `db_anon_role` is required; other fields are optional +// with sensible defaults. func ParsePostgRESTServiceConfig(config map[string]any) (*PostgRESTServiceConfig, []error) {Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/postgrest_service_config.go` around lines 11 - 16, The struct comment incorrectly marks DBAnonRole as optional/defaulted — update the comment for PostgRESTServiceConfig.DBAnonRole to indicate it is required (no default) instead of optional, and adjust the surrounding struct-level comment to only list defaults for truly optional fields (e.g., leave DBSchemas as default "public" but remove or change the "All fields are optional; defaults are applied" phrasing); locate the DBAnonRole field in PostgRESTServiceConfig and change its inline comment to reflect "required: no default" (or similar clear wording) so readers and maintainers know this field must be provided.server/internal/api/apiv1/validate_test.go (1)
1475-1477:⚠️ Potential issue | 🟡 MinorKeep unrelated PostgREST validation tests isolated with a valid
db_anon_role.These cases assert db_pool, unknown-key, jwt_secret, connect_as, or port-conflict behavior, but they still produce an additional missing-
db_anon_roleerror. Since the tests only useErrorContains, that extra error is masked.🧪 Proposed fixture updates
{ name: "postgrest invalid db_pool range", svc: &api.ServiceSpec{ ServiceID: "my-postgrest", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, Config: map[string]any{ - "db_pool": float64(99), + "db_anon_role": "web_anon", + "db_pool": float64(99), }, },{ name: "postgrest unknown config key", svc: &api.ServiceSpec{ ServiceID: "my-postgrest", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, Config: map[string]any{ - "invalid_key": "value", + "db_anon_role": "web_anon", + "invalid_key": "value", }, },{ name: "postgrest jwt_secret too short", svc: &api.ServiceSpec{ ServiceID: "my-postgrest", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, Config: map[string]any{ - "jwt_secret": "tooshort", + "db_anon_role": "web_anon", + "jwt_secret": "tooshort", }, },{ name: "postgrest missing connect_as", svc: &api.ServiceSpec{ ServiceID: "my-postgrest", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, + Config: map[string]any{"db_anon_role": "web_anon"}, // ConnectAs intentionally left empty },{ name: "postgrest connect_as references unknown user", svc: &api.ServiceSpec{ ServiceID: "my-postgrest", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, ConnectAs: "nonexistent", + Config: map[string]any{"db_anon_role": "web_anon"}, },{ ServiceID: "postgrest-server", ServiceType: "postgrest", Version: "latest", HostIds: []api.Identifier{"host-1"}, ConnectAs: "app", Port: utils.PointerTo(8080), - Config: map[string]any{}, + Config: map[string]any{"db_anon_role": "web_anon"}, },Also applies to: 1490-1492, 1505-1507, 1514-1535, 2271-2278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate_test.go` around lines 1475 - 1477, The tests that validate specific PostgREST config errors are failing to isolate the intended error because the test fixtures' Config maps lack a valid db_anon_role, producing an extra missing-role error; update each problematic test's Config (e.g., the Config map used in the table-driven cases around the shown snippet) to include a valid db_anon_role entry (for example "db_anon_role": "anon") so the tests only surface the targeted validation error (apply the same change to the other similar cases referenced in this file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/internal/api/apiv1/validate_test.go`:
- Around line 1475-1477: The tests that validate specific PostgREST config
errors are failing to isolate the intended error because the test fixtures'
Config maps lack a valid db_anon_role, producing an extra missing-role error;
update each problematic test's Config (e.g., the Config map used in the
table-driven cases around the shown snippet) to include a valid db_anon_role
entry (for example "db_anon_role": "anon") so the tests only surface the
targeted validation error (apply the same change to the other similar cases
referenced in this file).
In `@server/internal/database/postgrest_service_config.go`:
- Around line 11-16: The struct comment incorrectly marks DBAnonRole as
optional/defaulted — update the comment for PostgRESTServiceConfig.DBAnonRole to
indicate it is required (no default) instead of optional, and adjust the
surrounding struct-level comment to only list defaults for truly optional fields
(e.g., leave DBSchemas as default "public" but remove or change the "All fields
are optional; defaults are applied" phrasing); locate the DBAnonRole field in
PostgRESTServiceConfig and change its inline comment to reflect "required: no
default" (or similar clear wording) so readers and maintainers know this field
must be provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f7ff90e-6a4c-47ac-aa1b-58cbd1a4ca64
📒 Files selected for processing (6)
server/internal/api/apiv1/validate_test.goserver/internal/database/postgrest_service_config.goserver/internal/database/postgrest_service_config_test.goserver/internal/orchestrator/swarm/postgrest_authenticator_resource.goserver/internal/orchestrator/swarm/postgrest_authenticator_resource_test.goserver/internal/orchestrator/swarm/postgrest_config_resource_test.go
Struct comment on PostgRESTServiceConfig incorrectly stated all fields are optional with defaults. Updated to note DBAnonRole is required with no default, and added an inline field comment to match. Three validate_test.go cases (db_pool range, unknown config key, jwt_secret too short) were missing db_anon_role in their Config maps, causing an extra validation error that obscured the intended assertion. Added db_anon_role to each fixture. PLAT-568
All PostgREST E2E tests were failing because db_anon_role is now required and the test fixtures did not supply it. - postgrestSpec() injects db_anon_role: "anon" when not provided - postgrestBaseSpec() adds an "anon" (NOLOGIN) user to DatabaseUsers - TestPostgRESTMultiHostDBURI and TestPostgRESTFailover get the same anon user added to their inline DatabaseSpec - TestPostgRESTAuthenticatorRole assertion updated from pgedge_application_read_only to anon PLAT-568
rshoemaker
approved these changes
Apr 22, 2026
jason-lynch
deleted the
Feat/PLAT-568/Remove-pgedge_application_read_only-default-from-PostgREST-anon-role
branch
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
Removes the
pgedge_application_read_onlyfallback from the PostgRESTauthenticator, unblocking PLAT-559 (removal of deprecated built-in roles).
Previously, if a user deployed a PostgREST service without supplying
db_anon_role, the control plane silently defaulted topgedge_application_read_onlyas the anon role and granted it to theconnect_asuser. This created a hidden dependency on the deprecated rolethat prevented PLAT-559 from cleaning it up.
Changes
desiredAnonRole()inpostgrest_authenticator_resource.gonow returnsr.DBAnonRoledirectly — no fallbackdb_anon_roleis now required inParsePostgRESTServiceConfig—omitting it returns
400: db_anon_role is requireddb_anon_roleor assert the newrequired-field error
Testing
db_anon_role→400 db_anon_role is required✓db_anon_role→400 db_anon_role must not be empty✓db_anon_role: "web_anon"— onlyweb_anonis granted,
pgedge_application_read_onlyis not granted ✓go test ./...) ✓