feat: add creator-scoped config item properties#1930
Closed
adityathebe wants to merge 7 commits into
Closed
Conversation
Contributor
WalkthroughAdds an “owned properties” representation for ConfigItem properties, a pair of SQL functions to merge or delete creator-scoped properties, Go model types and helpers to persist/scan them, RBAC entries for the new RPCs, and tests/fixtures updated to use the new OwnedProperties shape. Updates cloning and benchmarks to the new type. ChangesOwned-properties feature (single cohesive DAG)
Sequence DiagramsequenceDiagram
participant Client
participant App as Go Model
participant DB as Postgres
Client->>App: call UpdateConfigItemProperties(configID, creatorType, createdBy, props)
App->>DB: SELECT changed, properties FROM update_config_item_properties(configID, creatorType, createdBy, props::jsonb)
DB-->>App: returns (changed, merged_properties_json)
App->>App: unmarshal merged_properties_json -> OwnedProperties
App-->>Client: return {Changed, Properties}
Possibly related PRs
🚥 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)
✨ Simplify code
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: ✅ No significant performance changes detectedFull benchstat output |
Benchstat (RLS)Base: ✅ No significant performance changes detectedFull benchstat output |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
adityathebe
force-pushed
the
feat/config-property-provenance
branch
from
1e7ea8c to
cc86af5
Compare
![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: 1
🧹 Nitpick comments (2)
functions/config_item_properties.sql (1)
20-54: Compute merged JSON once to reduce duplication and drift risk.Line 23-54 repeats the same merge expression in both
SETandWHERE. Consider factoring it into one CTE value and reusing it for both update and distinct-check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/config_item_properties.sql` around lines 20 - 54, Extract the repeated JSON merge expression into a single CTE (e.g., name it merged_incoming or merged_props) computed from stamped and the filtered existing properties, then reference that CTE in the UPDATE: use merged_props for the SET properties value and for the IS DISTINCT FROM check; update the current CTE chain so that stamped -> merged_props -> updated (which performs UPDATE config_items ci SET properties = (SELECT merged FROM merged_props) WHERE ci.id = p_config_id AND ci.properties IS DISTINCT FROM (SELECT merged FROM merged_props)) to remove duplication and ensure a single source of truth for the merged JSON used by updated.tests/config_item_properties_function_test.go (1)
104-114: Prefer dummy fixture resources before creating new config items in tests.Consider reusing
tests/fixtures/dummy/all.goresources first, then tailoring properties as needed, to keep fixture setup consistent across suites.Based on learnings: Use resources from the dummy dataset in
tests/fixtures/dummy/all.goin tests before creating new test resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/config_item_properties_function_test.go` around lines 104 - 114, seedConfigItemWithProperties currently creates new ConfigItem records directly; instead, reuse the dummy fixtures from tests/fixtures/dummy/all.go and then only mutate or extend their properties for this test. Modify seedConfigItemWithProperties (and callers) to first obtain an existing dummy resource (e.g., the exported dummy variables in tests/fixtures/dummy/all.go) to derive Type, ExternalID or base Config, then merge/override its Properties with the provided properties and persist that updated record via DefaultContext.DB().Create/Save; reference the seedConfigItemWithProperties function and DefaultContext.DB() when making the change so the test uses fixture resources rather than creating brand-new standalone items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models/config_item_properties.go`:
- Around line 25-31: The SQL function call using tx.Raw(...).Scan(&result) can
yield zero rows when configID doesn't exist, which Scan doesn't treat as an
error; after executing the query (the db returned by tx.Raw(...).Scan(&result)),
check the GORM RowsAffected value and if it is 0 return a descriptive error
(e.g., NotFound/invalid configID) instead of the zero-value
UpdateConfigItemPropertiesResult; ensure you still handle and return any non-nil
err from Scan. Use the existing symbols tx.Raw(...).Scan(&result), the result
variable and the UpdateConfigItemPropertiesResult return type to locate where to
add the RowsAffected check and error return.
---
Nitpick comments:
In `@functions/config_item_properties.sql`:
- Around line 20-54: Extract the repeated JSON merge expression into a single
CTE (e.g., name it merged_incoming or merged_props) computed from stamped and
the filtered existing properties, then reference that CTE in the UPDATE: use
merged_props for the SET properties value and for the IS DISTINCT FROM check;
update the current CTE chain so that stamped -> merged_props -> updated (which
performs UPDATE config_items ci SET properties = (SELECT merged FROM
merged_props) WHERE ci.id = p_config_id AND ci.properties IS DISTINCT FROM
(SELECT merged FROM merged_props)) to remove duplication and ensure a single
source of truth for the merged JSON used by updated.
In `@tests/config_item_properties_function_test.go`:
- Around line 104-114: seedConfigItemWithProperties currently creates new
ConfigItem records directly; instead, reuse the dummy fixtures from
tests/fixtures/dummy/all.go and then only mutate or extend their properties for
this test. Modify seedConfigItemWithProperties (and callers) to first obtain an
existing dummy resource (e.g., the exported dummy variables in
tests/fixtures/dummy/all.go) to derive Type, ExternalID or base Config, then
merge/override its Properties with the provided properties and persist that
updated record via DefaultContext.DB().Create/Save; reference the
seedConfigItemWithProperties function and DefaultContext.DB() when making the
change so the test uses fixture resources rather than creating brand-new
standalone items.
🪄 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: d948e48d-4931-497b-bd35-ca10ec2a5bdc
📒 Files selected for processing (5)
functions/config_item_properties.sqlmodels/config_item_properties.gorbac/objects.gotests/config_item_properties_function_test.gotypes/properties.go
adityathebe
force-pushed
the
feat/config-property-provenance
branch
2 times, most recently
from
127ef94 to
9876999
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/config_item_properties.sql`:
- Around line 120-144: computed currently reads ci.properties without locking,
allowing race conditions where two concurrent transactions overwrite each
other's changes; modify the computed CTE to select the target config_items row
using a SELECT ... FOR UPDATE (or FOR NO KEY UPDATE) against config_items
filtered by p_config_id so the row is locked before deriving new_properties,
keeping references to computed, updated, ci.properties, and p_config_id to
locate and update the CTE logic; ensure the lock is taken in the same
transaction before the jsonb aggregation so updated only runs after the locked
snapshot is used to compute and compare new_properties.
In `@models/owned_property.go`:
- Around line 96-123: The Scan implementation for OwnedProperties only accepts
[]byte but GormDBDataType advertises TEXT/NVARCHAR which database drivers often
return as string, so update the OwnedProperties.Scan method to also accept
string (and nullable string variants if used) by adding a type switch case for
string that converts it to []byte (or directly json.Unmarshal from the string)
before calling json.Unmarshal; reference the OwnedProperties.Scan method and
GormDBDataType to locate where to change this behavior and ensure Scan returns
the same zero-value on nil as it currently does.
🪄 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: 40b554c4-fd84-48ae-a450-8e285b95e529
📒 Files selected for processing (10)
functions/config_item_properties.sqlmodels/clone.gomodels/config.gomodels/owned_property.goquery/template_test.gorbac/objects.gotests/config_item_properties_function_test.gotests/fixtures/dummy/config.gotests/fixtures/dummy/config_demo_cluster.gotypes/resource_selector_test.go
✅ Files skipped from review due to trivial changes (1)
- rbac/objects.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/config_item_properties_function_test.go
Gavel resultsGavel exited with code . |
adityathebe
force-pushed
the
feat/config-property-provenance
branch
2 times, most recently
from
f27e903 to
9f6a3f8
Compare
Config item properties can be produced by scrapers and people, but previous writes had no ownership metadata or scoped replacement semantics. Add property creator provenance fields and a Postgres function that atomically replaces only the properties owned by a specific creator. The function preserves other creators and legacy unowned properties, returns the merged JSON for cache updates, and supports empty input as removal of the creator-owned slice. Expose the SQL function through a model helper and catalog RPC RBAC mapping so config-db can route scraper and manual property updates through the same path.
UpdateConfigItemProperties previously used Raw().Scan() without checking whether the SQL function returned any rows. When the config item ID did not exist, GORM left the result at its zero value and returned nil, making the missing item look like an unchanged update. Capture the scan result, return scan errors as before, and return an explicit not-found error when RowsAffected is zero. Add coverage for the missing config item case.
Concurrent updates for different property creators could derive their replacement JSON from the same stale config_items.properties snapshot. The later writer could then overwrite the earlier creator's slice, breaking the preservation guarantee. Lock the target config_items row before computing new_properties so concurrent calls serialize on the latest row state. Also allow OwnedProperties.Scan to accept string values returned by text-backed drivers such as sqlite or sqlserver.
Add a PostgREST-accessible SQL function for deleting a single config item property owned by a specific creator.\n\nThe delete RPC removes only the matching owned property by name while preserving other owners and legacy unowned properties, so UI deletes do not replace the user's entire property list.
adityathebe
force-pushed
the
feat/config-property-provenance
branch
from
9f6a3f8 to
4f6025d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
functions/config_item_properties.sql (1)
174-213: ⚡ Quick winDuplicate subquery computation in
delete_config_item_property.The same
jsonb_agg(prop ...)filter is computed twice — once in theSETclause and once in theWHERE IS DISTINCT FROMguard. Refactoring to the samelocked → computed → updatedCTE pattern used inupdate_config_item_propertiescomputes the result once, addsFOR UPDATEfor style consistency, and makes the no-op guard a simple column comparison.♻️ Proposed refactor
- RETURN QUERY - WITH updated AS ( - UPDATE config_items ci - SET properties = - COALESCE( - ( - SELECT jsonb_agg(prop ORDER BY ord) - FROM jsonb_array_elements(COALESCE(ci.properties, '[]'::jsonb)) - WITH ORDINALITY AS existing(prop, ord) - WHERE ( - prop->>'creator_type' = p_creator_type - AND prop->>'created_by' = p_created_by::text - AND prop->>'name' = p_property_name - ) IS NOT TRUE - ), - '[]'::jsonb - ) - WHERE ci.id = p_config_id - AND ci.properties IS DISTINCT FROM - COALESCE( - ( - SELECT jsonb_agg(prop ORDER BY ord) - FROM jsonb_array_elements(COALESCE(ci.properties, '[]'::jsonb)) - WITH ORDINALITY AS existing(prop, ord) - WHERE ( - prop->>'creator_type' = p_creator_type - AND prop->>'created_by' = p_created_by::text - AND prop->>'name' = p_property_name - ) IS NOT TRUE - ), - '[]'::jsonb - ) - RETURNING true AS changed, ci.properties - ) - SELECT updated.changed, updated.properties - FROM updated - UNION ALL - SELECT false AS changed, ci.properties - FROM config_items ci - WHERE ci.id = p_config_id - AND NOT EXISTS (SELECT 1 FROM updated); + RETURN QUERY + WITH locked AS ( + SELECT + ci.id, + COALESCE(ci.properties, '[]'::jsonb) AS current_properties + FROM config_items ci + WHERE ci.id = p_config_id + FOR UPDATE + ), computed AS ( + SELECT + locked.id, + locked.current_properties, + COALESCE( + ( + SELECT jsonb_agg(prop ORDER BY ord) + FROM jsonb_array_elements(locked.current_properties) + WITH ORDINALITY AS existing(prop, ord) + WHERE ( + prop->>'creator_type' = p_creator_type + AND prop->>'created_by' = p_created_by::text + AND prop->>'name' = p_property_name + ) IS NOT TRUE + ), + '[]'::jsonb + ) AS new_properties + FROM locked + ), updated AS ( + UPDATE config_items ci + SET properties = computed.new_properties + FROM computed + WHERE ci.id = computed.id + AND computed.current_properties IS DISTINCT FROM computed.new_properties + RETURNING true AS changed, ci.properties + ) + SELECT updated.changed, updated.properties + FROM updated + UNION ALL + SELECT false AS changed, computed.current_properties AS properties + FROM computed + WHERE NOT EXISTS (SELECT 1 FROM updated);🤖 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 `@functions/config_item_properties.sql` around lines 174 - 213, The delete_config_item_property logic duplicates the same jsonb_agg subquery in both the SET and the WHERE IS DISTINCT FROM check; refactor by adding a locking/compute CTE (e.g., compute_new AS (SELECT ci.id, ci.properties AS old_properties, (SELECT jsonb_agg(prop ORDER BY ord) FROM jsonb_array_elements(COALESCE(ci.properties, '[]'::jsonb)) WITH ORDINALITY AS existing(prop, ord) WHERE NOT (prop->>'creator_type' = p_creator_type AND prop->>'created_by' = p_created_by::text AND prop->>'name' = p_property_name)) AS new_properties FROM config_items ci WHERE ci.id = p_config_id FOR UPDATE)) then use an UPDATE ... FROM compute_new to set properties = COALESCE(compute_new.new_properties, '[]'::jsonb) only when compute_new.old_properties IS DISTINCT FROM COALESCE(compute_new.new_properties, '[]'::jsonb), RETURNING changed and properties in the updated CTE, and keep the UNION ALL fallback selecting from config_items when NOT EXISTS updated; reference compute_new, updated, p_config_id, p_creator_type, p_created_by, and p_property_name to implement this single-computation pattern.tests/config_item_properties_function_test.go (1)
166-182: ⚡ Quick winMissing test coverage for
delete_config_item_propertywith a non-existent config item.
callDeleteConfigItemPropertydoesn't guard against 0 rows being returned: if the SQL function returns nothing (config item missing),row.Propertiesis an empty string andjson.Unmarshal([]byte(""), &props)emitsunexpected end of JSON input, which is harder to diagnose than the correspondingconfig item not foundmessage incallUpdateConfigItemPropertiesErr. Consider adding parity test coverage for the missing-ID case, mirroring the existing"returns an error when the config item does not exist"spec.🤖 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_item_properties_function_test.go` around lines 166 - 182, The helper callDeleteConfigItemProperty can unmarshal an empty string when the SQL function returns no rows; add a guard after the DB scan to detect an empty row.Properties and fail with the same "config item not found" style used elsewhere (e.g., in callUpdateConfigItemPropertiesErr) before attempting json.Unmarshal. Update the callDeleteConfigItemProperty function to check if row.Properties == "" (or len(row.Properties) == 0) and call Expect(...).To/Fail with a clear "config item not found" message so tests get the same readable error for missing-ID cases.
🤖 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 `@functions/config_item_properties.sql`:
- Around line 174-213: The delete_config_item_property logic duplicates the same
jsonb_agg subquery in both the SET and the WHERE IS DISTINCT FROM check;
refactor by adding a locking/compute CTE (e.g., compute_new AS (SELECT ci.id,
ci.properties AS old_properties, (SELECT jsonb_agg(prop ORDER BY ord) FROM
jsonb_array_elements(COALESCE(ci.properties, '[]'::jsonb)) WITH ORDINALITY AS
existing(prop, ord) WHERE NOT (prop->>'creator_type' = p_creator_type AND
prop->>'created_by' = p_created_by::text AND prop->>'name' = p_property_name))
AS new_properties FROM config_items ci WHERE ci.id = p_config_id FOR UPDATE))
then use an UPDATE ... FROM compute_new to set properties =
COALESCE(compute_new.new_properties, '[]'::jsonb) only when
compute_new.old_properties IS DISTINCT FROM COALESCE(compute_new.new_properties,
'[]'::jsonb), RETURNING changed and properties in the updated CTE, and keep the
UNION ALL fallback selecting from config_items when NOT EXISTS updated;
reference compute_new, updated, p_config_id, p_creator_type, p_created_by, and
p_property_name to implement this single-computation pattern.
In `@tests/config_item_properties_function_test.go`:
- Around line 166-182: The helper callDeleteConfigItemProperty can unmarshal an
empty string when the SQL function returns no rows; add a guard after the DB
scan to detect an empty row.Properties and fail with the same "config item not
found" style used elsewhere (e.g., in callUpdateConfigItemPropertiesErr) before
attempting json.Unmarshal. Update the callDeleteConfigItemProperty function to
check if row.Properties == "" (or len(row.Properties) == 0) and call
Expect(...).To/Fail with a clear "config item not found" message so tests get
the same readable error for missing-ID cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8da30016-bacd-4c8e-bb39-2492a89ec21f
📒 Files selected for processing (11)
bench/insertion_test.gofunctions/config_item_properties.sqlmodels/clone.gomodels/config.gomodels/owned_property.goquery/template_test.gorbac/objects.gotests/config_item_properties_function_test.gotests/fixtures/dummy/config.gotests/fixtures/dummy/config_demo_cluster.gotypes/resource_selector_test.go
✅ Files skipped from review due to trivial changes (2)
- query/template_test.go
- models/owned_property.go
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/fixtures/dummy/config_demo_cluster.go
- models/config.go
- types/resource_selector_test.go
- rbac/objects.go
- models/clone.go
- tests/fixtures/dummy/config.go
Member
Author
|
abandoning this approach. follow up here: #1955 |
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
1 participant
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.
Config item properties can now carry creator provenance for scrapers and people.
Add a Postgres function that atomically replaces only the property slice owned by a specific creator, preserving user-created, other-scraper, and legacy unowned properties. Expose a Go helper for callers, register the RPC for catalog RBAC, and extend property models with created_by / creator_type.
This also supports empty incoming properties as creator now owns zero properties, allowing stale scraper-owned properties to be removed without clobbering manual properties.
Summary by CodeRabbit
New Features
Tests