feat: config properties table#1955
Open
adityathebe wants to merge 3 commits into
Open
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdded a new ChangesConfig Properties Feature
🚥 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 (RLS)Base: 📊 4 minor regression(s) (all within 5% threshold)
✅ 3 improvement(s)
Full benchstat output |
Benchstat (Other)Base: ✅ No significant performance changes detectedFull benchstat output |
adityathebe
force-pushed
the
feat/config-properties-normalized
branch
from
1683ff4 to
ccf81b6
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: 3
🧹 Nitpick comments (1)
views/006_config_views.sql (1)
156-199:config_properties_jsonis a regular view, not materialized — consider the full-scan cost forconfigs.
configsselects all rows fromconfig_items_with_properties, which in turn groups and aggregates the entireconfig_propertiestable viaconfig_properties_json. For a single config-detail lookup this is fine (the planner can push theconfig_idpredicate), but for list queries (e.g.,SELECT * FROM configs) the aggregation runs across allconfig_propertiesrows on every request.If
config_propertiesgrows large (high-volume scrapers writing many properties per item), consider materializingconfig_properties_jsonwith a refresh trigger/job on INSERT/UPDATE/DELETE onconfig_properties.🤖 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/006_config_views.sql` around lines 156 - 199, The view config_items_with_properties joins to config_properties_json (a regular view) which causes full aggregation of config_properties on list queries (e.g., configs SELECT *), so replace config_properties_json with a MATERIALIZED VIEW and add a refresh strategy: create a materialized view for config_properties_json, populate it initially, and implement a refresh mechanism (either triggers or a background refresh job) that refreshes the matview on INSERT/UPDATE/DELETE to config_properties; ensure refreshes are transactional or use CONCURRENTLY if needed and update any consumers that assume the view name/columns (config_items_with_properties, configs) to point to the matview.
🤖 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 `@models/config.go`:
- Around line 203-232: The NewConfigProperty function currently only uses the
first element of types.Property.Links and silently ignores additional links;
update the code by adding a clear doc comment above NewConfigProperty (and
optionally on the ConfigProperty type) that states the function and table schema
only store a single link (LinkURL/LinkLabel/LinkIcon) and any extra links in
types.Property will be dropped, referencing NewConfigProperty, ConfigProperty,
and types.Property so callers are aware; if you prefer API behavior change
instead, outline in the comment that a follow-up task will be required to
support multiple links and where to modify (NewConfigProperty and the DB
schema).
In `@views/006_config_views.sql`:
- Around line 138-148: The JSON for the "links" array omits the inlined Text
fields from types.Link; update the jsonb_build_object inside the CASE that
creates "links" (the block referencing link_url, link_label, link_icon) to also
include the inlined fields 'type', 'tooltip', and 'text' (e.g., map to columns
such as link_type, link_tooltip, link_text or the actual column names used) so
the jsonb_build_object produces {'type', 'url', 'label', 'icon', 'tooltip',
'text'} and then jsonb_strip_nulls/jsonb_build_array as before to avoid data
loss when serializing/deserializing types.Link.
In `@views/9998_rls_enable.sql`:
- Around line 181-194: The new RLS policy config_properties_auth won't be
applied when accessed through views that run as their owner, so add ALTER VIEW
... SECURITY_INVOKER for the views that currently default to security_definer:
set security_invoker = true on config_items_with_properties and
config_properties_json (place these ALTER VIEW statements next to the existing
security-invoker block that already sets configs and config_detail) so those
views execute as the calling PostgREST role and allow the config_properties_auth
policy to be enforced.
---
Nitpick comments:
In `@views/006_config_views.sql`:
- Around line 156-199: The view config_items_with_properties joins to
config_properties_json (a regular view) which causes full aggregation of
config_properties on list queries (e.g., configs SELECT *), so replace
config_properties_json with a MATERIALIZED VIEW and add a refresh strategy:
create a materialized view for config_properties_json, populate it initially,
and implement a refresh mechanism (either triggers or a background refresh job)
that refreshes the matview on INSERT/UPDATE/DELETE to config_properties; ensure
refreshes are transactional or use CONCURRENTLY if needed and update any
consumers that assume the view name/columns (config_items_with_properties,
configs) to point to the matview.
🪄 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: 2f0e55b2-c034-4627-92b6-9bb0edf21f91
📒 Files selected for processing (7)
models/config.gorbac/objects.goschema/config.hclviews/006_config_views.sqlviews/038_config_access.sqlviews/9998_rls_enable.sqlviews/9999_rls_disable.sql
adityathebe
force-pushed
the
feat/config-properties-normalized
branch
from
ccf81b6 to
080590b
Compare
adityathebe
force-pushed
the
feat/config-properties-normalized
branch
from
080590b to
2f51bc8
Compare
The config access migration recreates config_access_summary, but some databases can still have the old external_group_summary view depending on it. Drop the stale dependent view before dropping config_access_summary so the migration can proceed without using CASCADE.
Add a normalized config_properties table for properties created by scrapers or users, with foreign keys, creator validation, and uniqueness per creator/name. Expose typed properties through JSON compatibility views and merge them into configs and config_detail while keeping existing scraper-written config_items.properties intact.
Set security_invoker on the new config property compatibility views so configs and config_detail continue to enforce config_items RLS through nested view reads.
adityathebe
force-pushed
the
feat/config-properties-normalized
branch
from
2f51bc8 to
0c52d93
Compare
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.
Summary by CodeRabbit