Pr/config access#1945
Closed
moshloop wants to merge 3 commits into
Closed
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (22)
WalkthroughThe ChangesDocumentation Targets Reorganization
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Benchstat (Other)Base: 📊 1 minor regression(s) (all within 5% threshold)
✅ 1 improvement(s)
Full benchstat output |
Benchstat (RLS)Base: 📊 9 minor regression(s) (all within 5% threshold)
✅ 4 improvement(s)
Full benchstat output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
views/006_config_views.sql (1)
1171-1184:⚠️ Potential issue | 🟠 MajorAdd explicit
scraper_idto the INSERT statement or use a specific ON CONFLICT target.The concern is valid. The
config_relationshipstable has a 4-column unique constraint(related_id, config_id, relation, scraper_id)withscraper_iddefaulting to00000000-0000-0000-0000-000000000000. Without explicitly includingscraper_idin the INSERT,ON CONFLICT DO NOTHINGwill only detect conflicts when all 4 columns match. This means a duplicate Alias relationship can exist with the same(config_id, related_id, relation)but a differentscraper_id, violating intended uniqueness.The same pattern exists in two other INSERT statements at lines 1081–1083 and 1093–1095, which also omit
scraper_id. Additionally, the DELETE statements at lines 1088–1089 only filter on(config_id, related_id, relation)and missscraper_id, potentially leaving orphaned rows.Fix: Either (1) add
scraper_idto all three INSERT column lists, or (2) specify an explicitON CONFLICT (config_id, related_id, relation) DO NOTHINGtarget that excludesscraper_idif relationships are intentionally global across scrapers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/006_config_views.sql` around lines 1171 - 1184, The INSERT into config_relationships omits scraper_id so ON CONFLICT DO NOTHING doesn't match the table's unique key (related_id, config_id, relation, scraper_id); update the three INSERT statements that write Alias/other relations to include scraper_id in the INSERT column list and in the SELECT (e.g., use v_config_item.scraper_id or the appropriate source/default UUID) so the inserted rows include the correct scraper_id, and also update the corresponding DELETE statements to include scraper_id in their WHERE predicates (or alternatively, if relationships are intended to be global across scrapers, change the ON CONFLICT clause to an explicit ON CONFLICT (config_id, related_id, relation) DO NOTHING). Ensure changes apply to the INSERTs/DELETEs that reference config_relationships and use v_config_item and ci as shown.
🧹 Nitpick comments (1)
Makefile (1)
14-35: 💤 Low valueConsider updating AGENTS.md to reflect the ginkgo → gavel transition.
The switch from
ginkgotogavelfor unit tests appears intentional (E2E tests still useginkgoat line 39). However, the repository learning fromAGENTS.mdstates:"Always use ginkgo to run tests, never run
go testdirectly"This documentation should be updated to clarify that:
- Unit tests now use
gavel test- E2E tests continue to use
ginkgoBased on learnings, the documented guidance may need to be synchronized with this change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 14 - 35, Update AGENTS.md to reflect the test runner change: note that unit tests now use "gavel test" (include the equivalent flags used in the Makefile such as --timeout and --test-timeout and the ignore patterns or link to the Makefile target), and explicitly state that E2E tests still use "ginkgo" (keep the existing E2E guidance). Mention the policy change that contributors should run unit tests with the Makefile "make test" / "make test-concurrent" which invoke gavel, and keep the existing admonition about not running "go test" directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PROPERTIES.md`:
- Around line 9-10: Update the hardcoded absolute file URL in the PROPERTIES.md
link to a repository-relative path so other developers can access it; replace
the link target that currently points to /Users/moshe/go/src/... with a relative
reference like PROPERTIES.schema.json or ./PROPERTIES.schema.json in the line
containing [PROPERTIES.schema.json] to ensure the schema is resolved from the
repo rather than a local filesystem path.
In `@query/config_tree.go`:
- Around line 225-237: getExistingConfigsByIDs currently calls
ConfigItemFromCache for each id which causes a DB fallback per cache miss and
makes the operation O(n) round-trips; instead implement a batch fetch: try
resolving items from cache in bulk (or collect IDs that miss) and then run a
single DB query like WHERE id IN ? AND deleted_at IS NULL to load all missing
ConfigItem rows, merge results preserving the "skip missing rows" behavior, and
return the combined slice; update references in getExistingConfigsByIDs and the
cache-resolve logic (ConfigItemFromCache usage) to perform bulk resolution
rather than per-id queries.
In `@tests/config_access_test.go`:
- Around line 107-131: The test reads rows from the external_group_summary view
into summaries and builds byID; however missing rows make zero-value entries
pass the count asserts for deletedGroup.ID. Before asserting
MembersCount/PermissionsCount, assert the view actually returned the seeded
group by adding an existence check such as Expect(summaries).To(HaveLen(4)) or
Expect(byID).To(HaveKey(deletedGroup.ID)); locate the query that fills summaries
and the byID map in the test and insert the HaveLen/HaveKey assertion
immediately after building byID and before the existing count Expectations.
In `@tests/setup/common.go`:
- Around line 187-190: The current code constructs PgUrl by string replacement
which can corrupt credentials or query params; instead parse the Postgres DSN
with net/url (e.g., url.Parse), set the URL's Path to "/"+dbName and then
reserialize (u.String()) so only the path component changes, and use that value
for PgUrl before calling execPostgres (referencing dbName, PgUrl, execPostgres,
postgresDBUrl); apply the same change to the other occurrence around lines
278-282 so both places update only the URL path.
In `@views/045_merge_external_entities.sql`:
- Around line 291-295: The INSERT into external_user_groups can create duplicate
(external_user_id, external_group_id, scraper_id) rows across scrapers and thus
overcount in external_group_summary; modify the SELECT that feeds the INSERT
(the JOIN between external_user_groups eug and _eu_merges mp using mp.loser_id
-> mp.winner_id) to collapse duplicates before inserting by selecting distinct
tuples (mp.winner_id, eug.external_group_id, eug.scraper_id, eug.created_at) or
by grouping, so only unique scraper-scoped memberships are inserted; apply the
same dedup change to the other mirrored insert at lines 494-498 and/or
alternatively update external_group_summary to COUNT(DISTINCT external_user_id)
per external_group_id if you prefer to de-duplicate at summary time.
---
Outside diff comments:
In `@views/006_config_views.sql`:
- Around line 1171-1184: The INSERT into config_relationships omits scraper_id
so ON CONFLICT DO NOTHING doesn't match the table's unique key (related_id,
config_id, relation, scraper_id); update the three INSERT statements that write
Alias/other relations to include scraper_id in the INSERT column list and in the
SELECT (e.g., use v_config_item.scraper_id or the appropriate source/default
UUID) so the inserted rows include the correct scraper_id, and also update the
corresponding DELETE statements to include scraper_id in their WHERE predicates
(or alternatively, if relationships are intended to be global across scrapers,
change the ON CONFLICT clause to an explicit ON CONFLICT (config_id, related_id,
relation) DO NOTHING). Ensure changes apply to the INSERTs/DELETEs that
reference config_relationships and use v_config_item and ci as shown.
---
Nitpick comments:
In `@Makefile`:
- Around line 14-35: Update AGENTS.md to reflect the test runner change: note
that unit tests now use "gavel test" (include the equivalent flags used in the
Makefile such as --timeout and --test-timeout and the ignore patterns or link to
the Makefile target), and explicitly state that E2E tests still use "ginkgo"
(keep the existing E2E guidance). Mention the policy change that contributors
should run unit tests with the Makefile "make test" / "make test-concurrent"
which invoke gavel, and keep the existing admonition about not running "go test"
directly.
🪄 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: 49a5a897-06cd-4e5d-8969-6fdfdf05dff1
📒 Files selected for processing (23)
MakefilePROPERTIES.mdPROPERTIES.schema.jsonTaskfile.yamlmodels/common.gomodels/config.gomodels/config_access.goquery/config_tree.goquery/config_tree_test.goquery/resource_selector.gorbac/objects.goschema/config.hclschema/config_access.hcltests/config_access_test.gotests/config_relationship_test.gotests/e2e-blobs/containers.gotests/fixtures/dummy/all.gotests/setup/common.gotypes/client_options.goviews/006_config_views.sqlviews/038_config_access.sqlviews/045_merge_external_entities.sqlviews/9998_rls_enable.sql
Comment on lines
+225
to
+237
| func getExistingConfigsByIDs(ctx context.Context, ids []uuid.UUID) ([]models.ConfigItem, error) { | ||
| configs := make([]models.ConfigItem, 0, len(ids)) | ||
| for _, id := range ids { | ||
| config, err := ConfigItemFromCache(ctx, id.String()) | ||
| if err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| continue | ||
| } | ||
| return nil, err | ||
| } | ||
| configs = append(configs, config) | ||
| } | ||
| return configs, nil |
Contributor
There was a problem hiding this comment.
This reintroduces one cache/DB lookup per config ID.
ConfigItemFromCache falls back to a SQL query on every cache miss, so this helper turns one parent/child expansion into O(n) round-trips. On larger trees that will dominate the request. Keep the “skip missing rows” behavior, but fetch the ID set in one WHERE id IN ? AND deleted_at IS NULL query or bulk-resolve cache misses instead.
Batch-fetch variant
func getExistingConfigsByIDs(ctx context.Context, ids []uuid.UUID) ([]models.ConfigItem, error) {
- configs := make([]models.ConfigItem, 0, len(ids))
- for _, id := range ids {
- config, err := ConfigItemFromCache(ctx, id.String())
- if err != nil {
- if errors.Is(err, gorm.ErrRecordNotFound) {
- continue
- }
- return nil, err
- }
- configs = append(configs, config)
- }
- return configs, nil
+ if len(ids) == 0 {
+ return nil, nil
+ }
+ var configs []models.ConfigItem
+ if err := ctx.DB().
+ Where("id IN ?", lo.Uniq(ids)).
+ Where("deleted_at IS NULL").
+ Find(&configs).Error; err != nil {
+ return nil, err
+ }
+ return configs, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@query/config_tree.go` around lines 225 - 237, getExistingConfigsByIDs
currently calls ConfigItemFromCache for each id which causes a DB fallback per
cache miss and makes the operation O(n) round-trips; instead implement a batch
fetch: try resolving items from cache in bulk (or collect IDs that miss) and
then run a single DB query like WHERE id IN ? AND deleted_at IS NULL to load all
missing ConfigItem rows, merge results preserving the "skip missing rows"
behavior, and return the combined slice; update references in
getExistingConfigsByIDs and the cache-resolve logic (ConfigItemFromCache usage)
to perform bulk resolution rather than per-id queries.
Comment on lines
+291
to
+295
| INSERT INTO external_user_groups (external_user_id, external_group_id, scraper_id, created_at) | ||
| SELECT mp.winner_id, eug.external_group_id, eug.scraper_id, eug.created_at | ||
| FROM external_user_groups eug JOIN _eu_merges mp ON eug.external_user_id = mp.loser_id | ||
| WHERE eug.deleted_at IS NULL | ||
| ON CONFLICT (external_user_id, external_group_id) DO NOTHING; | ||
| ON CONFLICT (external_user_id, external_group_id, scraper_id) DO NOTHING; |
Contributor
There was a problem hiding this comment.
Deduplicate member counts across scrapers before preserving scraper-scoped memberships.
With scraper_id now part of the membership key, the same logical user↔group link can exist once per scraper. external_group_summary still does COUNT(*) grouped only by external_group_id in views/038_config_access.sql:225-231, so these inserts will overstate members_count as soon as two scrapers report the same membership. Either collapse duplicates here or make the summary count distinct members per group instead.
Also applies to: 494-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/045_merge_external_entities.sql` around lines 291 - 295, The INSERT
into external_user_groups can create duplicate (external_user_id,
external_group_id, scraper_id) rows across scrapers and thus overcount in
external_group_summary; modify the SELECT that feeds the INSERT (the JOIN
between external_user_groups eug and _eu_merges mp using mp.loser_id ->
mp.winner_id) to collapse duplicates before inserting by selecting distinct
tuples (mp.winner_id, eug.external_group_id, eug.scraper_id, eug.created_at) or
by grouping, so only unique scraper-scoped memberships are inserted; apply the
same dedup change to the other mirrored insert at lines 494-498 and/or
alternatively update external_group_summary to COUNT(DISTINCT external_user_id)
per external_group_id if you prefer to de-duplicate at summary time.
adityathebe
reviewed
May 1, 2026
| return err | ||
| } | ||
|
|
||
| func randomDatabaseName(prefix string) string { |
adityathebe
force-pushed
the
pr/config-access
branch
from
b6a1892 to
edfdbbb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/setup/common.go (1)
187-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace DSN string rewrites with URL path mutation.
Line 188 and Line 282 still rewrite the full URL via string replacement, which can accidentally modify credentials/query fragments or the wrong path segment. Parse with
net/urland only setu.Path.Suggested fix
import ( "database/sql" "fmt" + neturl "net/url" "net/http" "os" "path" "strconv" "strings" @@ ) + +func withDatabaseName(rawURL, dbName string) (string, error) { + u, err := neturl.Parse(rawURL) + if err != nil { + return "", err + } + u.Path = "/" + dbName + return u.String(), nil +} @@ - PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1) + newURL, err := withDatabaseName(url, dbName) + if err != nil { + return context.Context{}, err + } + PgUrl = newURL @@ - config := api.NewConfig(strings.ReplaceAll(pgUrl, pgDbName, newName)) + newURL, err := withDatabaseName(pgUrl, newName) + if err != nil { + return nil, nil, err + } + config := api.NewConfig(newURL)Also applies to: 282-282
🤖 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 `@tests/setup/common.go` around lines 187 - 188, The code currently builds dbName and then rewrites PgUrl by string replacement of url (using dbName and PgUrl variables), which can corrupt credentials or query strings; instead parse the DSN with net/url (parse the existing url variable into u via url.Parse), set u.Path = "/"+dbName (or use path.Join with the existing Path if needed), then assign PgUrl = u.String(); apply the same change to the other occurrence that rewrites the URL so only the path is mutated and all other URL components (userinfo, host, query) are preserved.
🧹 Nitpick comments (1)
Makefile (1)
18-35: ⚡ Quick winFactor shared test ignore flags into one variable.
testandtest-concurrentduplicate the same ignore set; centralizing it reduces drift and addresses body-length churn.♻️ Proposed refactor
+TEST_IGNORES := \ + --ignore ./bench \ + --ignore ./hack \ + --ignore ./specs \ + --ignore ./tests/e2e \ + --ignore ./tests/e2e-blobs + test: gavel gavel test --timeout 30m --test-timeout 15m \ - --ignore ./bench \ - --ignore ./hack \ - --ignore ./specs \ - --ignore ./tests/e2e \ - --ignore ./tests/e2e-blobs \ + $(TEST_IGNORES) \ ./... test-concurrent: gavel gavel test --timeout 30m --test-timeout 15m \ --nodes 4 \ - --ignore ./bench \ - --ignore ./hack \ - --ignore ./specs \ - --ignore ./tests/e2e \ - --ignore ./tests/e2e-blobs \ + $(TEST_IGNORES) \ ./...🤖 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 `@Makefile` around lines 18 - 35, The duplicated ignore list in the Makefile targets test and test-concurrent should be moved into a single Makefile variable (e.g., TEST_IGNORES) and referenced in both targets to avoid drift; update the test target and the test-concurrent target (which also adds --nodes 4) to replace the repeated --ignore ... lines with the TEST_IGNORES variable, keeping the existing flags like --timeout and --test-timeout intact and preserving the trailing ./... argument.
🤖 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 `@Makefile`:
- Around line 95-99: The Make recipe for the target labeled "PROPERTIES.md:
captain" runs the pipeline "claude -p ... | captain" without preserving upstream
failures; change the shell invocation to enable pipefail so the recipe fails
when claude fails—either set the makefile shell flags (e.g., ensure SHELL is
bash and add .SHELLFLAGS = -eu -o pipefail -c) or wrap the single recipe line
with bash -o pipefail -c 'claude -p ... | captain' so the exit status reflects
failures from both claude and captain.
- Around line 14-17: Replace the floating `@latest` for gavel with a pinned
version variable: add a GAVEL_VERSION variable (e.g., GAVEL_VERSION := vX.Y.Z)
at the top of the Makefile and update the gavel target to use go install
github.com/flanksource/gavel/cmd/gavel@$(GAVEL_VERSION); ensure the variable is
documented alongside existing tool version variables (like
CONTROLLER_TOOLS_VERSION) so CI is reproducible and easy to update.
In `@tests/setup/common.go`:
- Line 190: The CREATE/DROP DATABASE SQL is building identifiers by direct
string interpolation (see execPostgres usage with postgresDBUrl and dbName),
which is brittle and risky; instead validate the dbName against a safe pattern
(e.g., only letters, digits, underscore) and quote/escape it as a SQL identifier
before formatting the statement. Add a small helper (e.g., quoteIdentifier or
validateAndQuoteDBName) used where you call execPostgres for CREATE DATABASE and
DROP DATABASE to reject invalid names and return the quoted identifier (escape
any internal double-quotes by doubling and wrap in double quotes) and use that
quoted value when building the SQL string. Ensure all places that interpolate
dbName (the CREATE DATABASE and DROP DATABASE calls) use this helper.
---
Duplicate comments:
In `@tests/setup/common.go`:
- Around line 187-188: The code currently builds dbName and then rewrites PgUrl
by string replacement of url (using dbName and PgUrl variables), which can
corrupt credentials or query strings; instead parse the DSN with net/url (parse
the existing url variable into u via url.Parse), set u.Path = "/"+dbName (or use
path.Join with the existing Path if needed), then assign PgUrl = u.String();
apply the same change to the other occurrence that rewrites the URL so only the
path is mutated and all other URL components (userinfo, host, query) are
preserved.
---
Nitpick comments:
In `@Makefile`:
- Around line 18-35: The duplicated ignore list in the Makefile targets test and
test-concurrent should be moved into a single Makefile variable (e.g.,
TEST_IGNORES) and referenced in both targets to avoid drift; update the test
target and the test-concurrent target (which also adds --nodes 4) to replace the
repeated --ignore ... lines with the TEST_IGNORES variable, keeping the existing
flags like --timeout and --test-timeout intact and preserving the trailing ./...
argument.
🪄 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: 4595ddc7-d351-4165-9a33-ab4af4713077
📒 Files selected for processing (23)
MakefilePROPERTIES.mdPROPERTIES.schema.jsonTaskfile.yamlmodels/common.gomodels/config.gomodels/config_access.goquery/config_tree.goquery/config_tree_test.goquery/resource_selector.gorbac/objects.goschema/config.hclschema/config_access.hcltests/config_access_test.gotests/config_relationship_test.gotests/e2e-blobs/containers.gotests/fixtures/dummy/all.gotests/setup/common.gotypes/client_options.goviews/006_config_views.sqlviews/038_config_access.sqlviews/045_merge_external_entities.sqlviews/9998_rls_enable.sql
✅ Files skipped from review due to trivial changes (5)
- views/9998_rls_enable.sql
- tests/e2e-blobs/containers.go
- views/045_merge_external_entities.sql
- PROPERTIES.schema.json
- query/config_tree_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- types/client_options.go
- schema/config_access.hcl
- schema/config.hcl
- models/config.go
- Taskfile.yaml
- query/config_tree.go
- tests/config_access_test.go
- views/038_config_access.sql
| dbName = fmt.Sprintf("duty_gingko%d", port) | ||
| PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1) | ||
| _ = execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) | ||
| if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName); err != nil { |
Contributor
There was a problem hiding this comment.
Avoid raw identifier interpolation in CREATE/DROP DATABASE SQL.
Line 190, Line 195, Line 278, and Line 299 interpolate DB names directly into SQL. Even in test code, this is brittle (identifier syntax) and opens injection risk if names ever become unsanitized. Validate and quote identifiers before formatting SQL.
Suggested fix
import (
"database/sql"
"fmt"
"net/http"
"os"
"path"
+ "regexp"
"strconv"
"strings"
@@
)
+
+var validDBIdentifier = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
+
+func quoteDBIdentifier(name string) (string, error) {
+ if !validDBIdentifier.MatchString(name) {
+ return "", fmt.Errorf("invalid database identifier: %q", name)
+ }
+ return `"` + name + `"`, nil
+}
@@
- _ = execPostgres(postgresDBUrl, "DROP DATABASE "+dbName)
- if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName); err != nil {
+ qdb, err := quoteDBIdentifier(dbName)
+ if err != nil {
+ return context.Context{}, err
+ }
+ _ = execPostgres(postgresDBUrl, "DROP DATABASE "+qdb)
+ if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+qdb); err != nil {
return context.Context{}, fmt.Errorf("cannot create %s: %v", dbName, err)
}
@@
- if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", dbName)); err != nil {
+ if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", qdb)); err != nil {
logger.Errorf("execPostgres: %v", err)
}
@@
- if err := ctx.DB().Exec(fmt.Sprintf("CREATE DATABASE %s", newName)).Error; err != nil {
+ qNewName, err := quoteDBIdentifier(newName)
+ if err != nil {
+ return nil, nil, err
+ }
+ if err := ctx.DB().Exec(fmt.Sprintf("CREATE DATABASE %s", qNewName)).Error; err != nil {
return nil, nil, err
}
@@
- if err := ctx.DB().Exec(fmt.Sprintf("DROP DATABASE %s (FORCE)", newName)).Error; err != nil {
+ if err := ctx.DB().Exec(fmt.Sprintf("DROP DATABASE %s (FORCE)", qNewName)).Error; err != nil {
logger.Errorf("error cleaning up db: %v", err)
}Also applies to: 195-195, 278-278, 299-299
🤖 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 `@tests/setup/common.go` at line 190, The CREATE/DROP DATABASE SQL is building
identifiers by direct string interpolation (see execPostgres usage with
postgresDBUrl and dbName), which is brittle and risky; instead validate the
dbName against a safe pattern (e.g., only letters, digits, underscore) and
quote/escape it as a SQL identifier before formatting the statement. Add a small
helper (e.g., quoteIdentifier or validateAndQuoteDBName) used where you call
execPostgres for CREATE DATABASE and DROP DATABASE to reject invalid names and
return the quoted identifier (escape any internal double-quotes by doubling and
wrap in double quotes) and use that quoted value when building the SQL string.
Ensure all places that interpolate dbName (the CREATE DATABASE and DROP DATABASE
calls) use this helper.
Gavel resultsGavel exited with code . |
…r groups Introduce scraper_id as part of composite primary keys for ConfigRelationship and ExternalUserGroup to support multi-scraper ownership. This allows multiple scrapers to independently maintain relationship tuples without colliding, while uuid.Nil represents legacy scraper-agnostic relationships. Add Health.Badge() method to render health status as colored pills for dense table cells. Implement MergeConfigTrees() to collapse independently-built config trees into a unified forest by merging shared ancestors recursively. Enhance SearchResources to populate Health and Status fields for configs and components. Add RoleExternalIDs to ConfigAccessSummary. Create external_group_summary view with member and permission counts. Update SQL views to filter deleted relationships and add GROUP BY clauses for accurate counts. Simplify database setup helpers by removing unnecessary quoting and URL parsing utilities. BREAKING CHANGE: ConfigRelationship and ExternalUserGroup primary keys now include scraper_id; existing code must populate this field from scraper context.
Replace ginkgo-based test targets with gavel, a unified test runner that provides better output formatting and cross-platform compatibility. Update test and test-concurrent targets to use gavel with consistent timeout and ignore patterns. Add gavel installation target and captain target for properties documentation generation. Add PROPERTIES.md and PROPERTIES.schema.json documenting all runtime properties across duty and related services. Update Taskfile.yaml test task to use gavel with passthrough argument support.
adityathebe
force-pushed
the
pr/config-access
branch
from
edfdbbb to
10a4417
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
PROPERTIES.md (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace local absolute schema link with a repo-relative link
Line 10 points to a machine-local absolute path, which breaks for everyone else. Use a repository-relative path (
./PROPERTIES.schema.json).🤖 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 `@PROPERTIES.md` at line 10, The markdown link in PROPERTIES.md currently points to a machine-local absolute path "[PROPERTIES.schema.json](/Users/moshe/go/src/github.com/flanksource/duty/PROPERTIES.schema.json)"; change that link to a repository-relative path by replacing the absolute URL with "./PROPERTIES.schema.json" so the link works for all contributors.tests/setup/common.go (1)
189-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse validated/quoted SQL identifiers for database names
Line 189, Line 190, Line 195, Line 278, and Line 299 still build
CREATE/DROP DATABASESQL with direct name interpolation. Even in test utilities, this is brittle for identifier syntax and keeps an injection footgun. Please route all DB-name SQL through one helper that validates and quotes identifiers.Proposed minimal fix
+var validDBIdentifier = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) + +func quoteDBIdentifier(name string) (string, error) { + if !validDBIdentifier.MatchString(name) { + return "", fmt.Errorf("invalid database identifier: %q", name) + } + return `"` + name + `"`, nil +} @@ - _ = execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) - if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName); err != nil { + qdb, err := quoteDBIdentifier(dbName) + if err != nil { + return context.Context{}, err + } + _ = execPostgres(postgresDBUrl, "DROP DATABASE "+qdb) + if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+qdb); err != nil { @@ - if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", dbName)); err != nil { + if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", qdb)); err != nil { @@ - if err := ctx.DB().Exec(fmt.Sprintf("CREATE DATABASE %s", newName)).Error; err != nil { + qNewName, err := quoteDBIdentifier(newName) + if err != nil { + return nil, nil, err + } + if err := ctx.DB().Exec(fmt.Sprintf("CREATE DATABASE %s", qNewName)).Error; err != nil { @@ - if err := ctx.DB().Exec(fmt.Sprintf("DROP DATABASE %s (FORCE)", newName)).Error; err != nil { + if err := ctx.DB().Exec(fmt.Sprintf("DROP DATABASE %s (FORCE)", qNewName)).Error; err != nil {Also applies to: 195-195, 278-278, 299-299
🤖 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 `@tests/setup/common.go` around lines 189 - 190, Replace all direct string concatenation that builds "CREATE DATABASE "+dbName and "DROP DATABASE "+dbName with a single helper (e.g., validateAndQuoteIdentifier or quoteSQLIdentifier) that validates the identifier characters and returns a properly quoted identifier; update every call site that calls execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName) or execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) to use execPostgres(postgresDBUrl, "CREATE DATABASE "+quoteSQLIdentifier(dbName)) and similarly for DROP, and ensure the helper is used for all occurrences in tests/setup/common.go (references: execPostgres, dbName, CREATE DATABASE, DROP DATABASE).Makefile (1)
96-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve failures from the
claude | captainpipelineLine 96-98 currently allows a successful
captainexit to hide an upstreamclaudefailure. Run this recipe withpipefailso either side failing fails the target.Proposed fix
- claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \ - "/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \ - | captain + bash -o pipefail -c 'claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \ + "/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \ + | captain'🤖 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 `@Makefile` around lines 96 - 98, The Makefile recipe that runs the pipeline "claude -p ... | captain" hides upstream failures; update that recipe so the shell runs with pipefail enabled (e.g., prefix the recipe with set -o pipefail or invoke bash with -o pipefail) so any nonzero exit from the claude stage causes the target to fail; modify the Makefile entry containing the "claude -p --permission-mode acceptEdits ... | captain" command accordingly.tests/config_access_test.go (1)
119-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an existence assertion before zero-value count checks.
Line 130 and Line 131 can pass even if
deletedGroup.IDis absent, because map lookup returns a zero-value struct. Assert presence first (HaveKey) or exact row count.Proposed test hardening
byID := map[uuid.UUID]externalGroupSummary{} for _, summary := range summaries { byID[summary.ID] = summary } + Expect(byID).To(HaveKey(dummy.MissionControlAdminsGroup.ID)) + Expect(byID).To(HaveKey(dummy.MissionControlReadersGroup.ID)) + Expect(byID).To(HaveKey(dummy.MissionControlEmptyGroup.ID)) + Expect(byID).To(HaveKey(deletedGroup.ID))🤖 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 `@tests/config_access_test.go` around lines 119 - 131, The test currently indexes into byID and asserts counts for deletedGroup.ID which will succeed with zero-values if the key is missing; before checking MembersCount/PermissionsCount for deletedGroup.ID, add an explicit existence assertion such as Expect(byID).To(HaveKey(deletedGroup.ID)) (or Expect(summaries).To(ContainElement(...)) / assert exact row count) so the test fails when the summary is absent rather than silently passing on zero-values; update the checks around byID, deletedGroup.ID to include this HaveKey assertion.
🤖 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 `@tests/setup/common.go`:
- Line 282: Replace the blind strings.ReplaceAll used when constructing config
(the call site creating config := api.NewConfig(strings.ReplaceAll(pgUrl,
pgDbName, newName))) with proper URL parsing: parse pgUrl (using net/url),
update only the path component that holds the database name (e.g., swap the path
segment equal to pgDbName to newName or set u.Path = "/" + newName), then
reconstruct the URL (u.String()) and pass that into api.NewConfig so only the DB
path is changed and usernames/query values remain untouched.
In `@views/038_config_access.sql`:
- Around line 16-20: The CTE active_external_user_groups and all
joins/aggregations that currently match only on external_group_id must include
scraper_id to prevent cross-scraper mixing: update the CTE SELECT DISTINCT to
include external_user_id, external_group_id AND scraper_id (from
external_user_groups), add scraper_id to every JOIN predicate that currently
matches on external_group_id alone, and add scraper_id to all GROUP BY clauses
and aggregation keys (the queries that aggregate by group id must aggregate by
(external_group_id, scraper_id)). Ensure every reference to
external_user_groups/external_group_id in this view (including the blocks
derived from active_external_user_groups and the aggregations around group
membership/grants) is adjusted to carry and join on scraper_id.
---
Duplicate comments:
In `@Makefile`:
- Around line 96-98: The Makefile recipe that runs the pipeline "claude -p ... |
captain" hides upstream failures; update that recipe so the shell runs with
pipefail enabled (e.g., prefix the recipe with set -o pipefail or invoke bash
with -o pipefail) so any nonzero exit from the claude stage causes the target to
fail; modify the Makefile entry containing the "claude -p --permission-mode
acceptEdits ... | captain" command accordingly.
In `@PROPERTIES.md`:
- Line 10: The markdown link in PROPERTIES.md currently points to a
machine-local absolute path
"[PROPERTIES.schema.json](/Users/moshe/go/src/github.com/flanksource/duty/PROPERTIES.schema.json)";
change that link to a repository-relative path by replacing the absolute URL
with "./PROPERTIES.schema.json" so the link works for all contributors.
In `@tests/config_access_test.go`:
- Around line 119-131: The test currently indexes into byID and asserts counts
for deletedGroup.ID which will succeed with zero-values if the key is missing;
before checking MembersCount/PermissionsCount for deletedGroup.ID, add an
explicit existence assertion such as Expect(byID).To(HaveKey(deletedGroup.ID))
(or Expect(summaries).To(ContainElement(...)) / assert exact row count) so the
test fails when the summary is absent rather than silently passing on
zero-values; update the checks around byID, deletedGroup.ID to include this
HaveKey assertion.
In `@tests/setup/common.go`:
- Around line 189-190: Replace all direct string concatenation that builds
"CREATE DATABASE "+dbName and "DROP DATABASE "+dbName with a single helper
(e.g., validateAndQuoteIdentifier or quoteSQLIdentifier) that validates the
identifier characters and returns a properly quoted identifier; update every
call site that calls execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName) or
execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) to use
execPostgres(postgresDBUrl, "CREATE DATABASE "+quoteSQLIdentifier(dbName)) and
similarly for DROP, and ensure the helper is used for all occurrences in
tests/setup/common.go (references: execPostgres, dbName, CREATE DATABASE, DROP
DATABASE).
🪄 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: 76e9c536-d66a-43eb-b726-b3370de1568c
📒 Files selected for processing (23)
MakefilePROPERTIES.mdPROPERTIES.schema.jsonTaskfile.yamlmodels/common.gomodels/config.gomodels/config_access.goquery/config_tree.goquery/config_tree_test.goquery/resource_selector.gorbac/objects.goschema/config.hclschema/config_access.hcltests/config_access_test.gotests/config_relationship_test.gotests/e2e-blobs/containers.gotests/fixtures/dummy/all.gotests/setup/common.gotypes/client_options.goviews/006_config_views.sqlviews/038_config_access.sqlviews/045_merge_external_entities.sqlviews/9998_rls_enable.sql
✅ Files skipped from review due to trivial changes (3)
- views/9998_rls_enable.sql
- models/common.go
- tests/e2e-blobs/containers.go
🚧 Files skipped from review as they are similar to previous changes (11)
- views/045_merge_external_entities.sql
- models/config_access.go
- query/resource_selector.go
- schema/config.hcl
- rbac/objects.go
- schema/config_access.hcl
- query/config_tree.go
- views/006_config_views.sql
- query/config_tree_test.go
- models/config.go
- PROPERTIES.schema.json
| } | ||
|
|
||
| config := api.NewConfig(newPgURL) | ||
| config := api.NewConfig(strings.ReplaceAll(pgUrl, pgDbName, newName)) |
Contributor
There was a problem hiding this comment.
Avoid global string replacement when switching DB name in DSN
Line 282 uses strings.ReplaceAll(pgUrl, pgDbName, newName), which can modify unintended URL parts (e.g., username/query values containing the old DB name). Parse the URL and replace only the path/database segment.
Proposed fix
- config := api.NewConfig(strings.ReplaceAll(pgUrl, pgDbName, newName))
+ u, err := neturl.Parse(pgUrl)
+ if err != nil {
+ return nil, nil, err
+ }
+ u.Path = "/" + newName
+ config := api.NewConfig(u.String())🤖 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 `@tests/setup/common.go` at line 282, Replace the blind strings.ReplaceAll used
when constructing config (the call site creating config :=
api.NewConfig(strings.ReplaceAll(pgUrl, pgDbName, newName))) with proper URL
parsing: parse pgUrl (using net/url), update only the path component that holds
the database name (e.g., swap the path segment equal to pgDbName to newName or
set u.Path = "/" + newName), then reconstruct the URL (u.String()) and pass that
into api.NewConfig so only the DB path is changed and usernames/query values
remain untouched.
Comment on lines
+16
to
+20
| WITH active_external_user_groups AS ( | ||
| SELECT DISTINCT external_user_id, external_group_id | ||
| FROM external_user_groups | ||
| WHERE deleted_at IS NULL | ||
| ) |
Contributor
There was a problem hiding this comment.
Scope joins and aggregations by scraper_id to avoid cross-scraper access mixing.
Line 36 and Line 58 match only on external_group_id, and Line 230 / Line 238 aggregate only by group ID. With scraper-scoped membership/grant data, this can blend rows across scrapers and inflate/misattribute members/permissions.
Suggested direction
WITH active_external_user_groups AS (
- SELECT DISTINCT external_user_id, external_group_id
+ SELECT DISTINCT external_user_id, external_group_id, scraper_id
FROM external_user_groups
WHERE deleted_at IS NULL
)
...
- INNER JOIN active_external_user_groups ON config_access.external_group_id = active_external_user_groups.external_group_id
+ INNER JOIN active_external_user_groups
+ ON config_access.external_group_id = active_external_user_groups.external_group_id
+ AND config_access.scraper_id = active_external_user_groups.scraper_id
...
AND NOT EXISTS (
SELECT 1 FROM active_external_user_groups
- WHERE active_external_user_groups.external_group_id = config_access.external_group_id
+ WHERE active_external_user_groups.external_group_id = config_access.external_group_id
+ AND active_external_user_groups.scraper_id = config_access.scraper_id
) LEFT JOIN (
SELECT
external_user_groups.external_group_id,
+ external_user_groups.scraper_id,
COUNT(*) AS members_count
FROM external_user_groups
WHERE external_user_groups.deleted_at IS NULL
- GROUP BY external_user_groups.external_group_id
-) group_members ON group_members.external_group_id = external_groups.id
+ GROUP BY external_user_groups.external_group_id, external_user_groups.scraper_id
+) group_members
+ ON group_members.external_group_id = external_groups.id
+ AND group_members.scraper_id = external_groups.scraper_id LEFT JOIN (
SELECT
config_access_summary.external_group_id,
+ -- include scraper scope in source view or switch source to one that already has scraper_id
+ config_access_summary.scraper_id,
COUNT(*) AS permissions_count
FROM config_access_summary
WHERE config_access_summary.external_group_id IS NOT NULL
- GROUP BY config_access_summary.external_group_id
-) group_permissions ON group_permissions.external_group_id = external_groups.id;
+ GROUP BY config_access_summary.external_group_id, config_access_summary.scraper_id
+) group_permissions
+ ON group_permissions.external_group_id = external_groups.id
+ AND group_permissions.scraper_id = external_groups.scraper_id;Also applies to: 36-37, 57-59, 229-235, 237-243
🤖 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 `@views/038_config_access.sql` around lines 16 - 20, The CTE
active_external_user_groups and all joins/aggregations that currently match only
on external_group_id must include scraper_id to prevent cross-scraper mixing:
update the CTE SELECT DISTINCT to include external_user_id, external_group_id
AND scraper_id (from external_user_groups), add scraper_id to every JOIN
predicate that currently matches on external_group_id alone, and add scraper_id
to all GROUP BY clauses and aggregation keys (the queries that aggregate by
group id must aggregate by (external_group_id, scraper_id)). Ensure every
reference to external_user_groups/external_group_id in this view (including the
blocks derived from active_external_user_groups and the aggregations around
group membership/grants) is adjusted to carry and join on scraper_id.
Error: ../../../go/pkg/mod/github.com/flanksource/gavel@v0.0.35/testrunner/ui/assets.go:5:12: pattern dist/testui.js: no matching files found Error: ../../../go/pkg/mod/github.com/flanksource/gavel@v0.0.35/pr/ui/assets.go:5:12: pattern dist/prui.js: no matching files found make: *** [Makefile:16: gavel] Error 1 Error: Process completed with exit code 2.
adityathebe
force-pushed
the
pr/config-access
branch
from
0e74f12 to
ded47ac
Compare
yashmehrotra
force-pushed
the
pr/config-access
branch
from
ded47ac to
ac34658
Compare
adityathebe
force-pushed
the
pr/config-access
branch
from
ac34658 to
ded47ac
Compare
Member
|
https://github.com/flanksource/duty/pull/1946/changes was merged and it has all the changes from this PR |
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 by CodeRabbit