ENG-1311 Unify ConceptAccess and ContentAccess tables #712
+339
−179
Conversation
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
|
Updates to Preview Branch (eng-1311-unify-conceptaccess-and-contentaccess-tables) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
changed the base branch from
main
to
eng-1310-database-change-allow-concepts-to-exist-without-content
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughConsolidates concept- and content-specific access into a unified ResourceAccess table keyed by (account_uid, source_local_id, space_id); replaces can_view_specific_concept/content with can_view_specific_resource; adds is_last_local_reference and DB triggers/functions to maintain ResourceAccess and update RLS views/policies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB as Database
participant Trigger as on_delete_local_reference()
participant Checker as is_last_local_reference()
participant RA as ResourceAccess
Client->>DB: DELETE FROM "Content"/"Concept"/"Document" (row with source_local_id, space_id)
DB-->>Trigger: AFTER DELETE ROW fires
Trigger->>Checker: is_last_local_reference(space_id, source_local_id)
Checker-->>Trigger: boolean
alt last local reference
Trigger->>RA: DELETE FROM ResourceAccess WHERE source_local_id=... AND space_id=...
end
Trigger-->>DB: complete
sequenceDiagram
participant Client
participant DB as Database
participant Trigger as on_update_local_reference()
participant RA as ResourceAccess
Client->>DB: UPDATE "Content"/"Concept"/"Document" (change source_local_id or space_id)
DB-->>Trigger: AFTER UPDATE ROW fires
Trigger->>RA: DELETE/UPDATE ResourceAccess entries for OLD (source_local_id, space_id) as needed
RA-->>Trigger: rows removed/updated
Trigger-->>DB: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/database/supabase/schemas/content.sql (1)
648-680: Document access via ContentAccess won’t surface inmy_documentsview.Line 650 now allows document access via
can_view_specific_content, butpublic.my_documentsstill filters only onmy_space_ids(). This means users granted local-reference access (anddocument_of_contentlookups) won’t see documents unless they’re space members. Consider aligning the view predicate with the new policy.Based on learnings, the `my_*` views are the performance-critical path and should stay consistent with RLS behavior.♻️ Suggested view alignment (outside this range)
-FROM public."Document" WHERE space_id = any(public.my_space_ids()); +FROM public."Document" +WHERE ( + space_id = any(public.my_space_ids()) + OR public.can_view_specific_content(space_id, source_local_id) +);
🤖 Fix all issues with AI agents
In
`@packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql`:
- Around line 159-171: The trigger function on_delete_space_revoke_local_access
uses a non-existent column OLD.space_id causing failures on Space deletes;
update the DELETE statement in function on_delete_space_revoke_local_access to
use OLD.id (i.e., DELETE FROM public."ContentAccess" WHERE space_id = OLD.id) so
it correctly revokes ContentAccess rows for the deleted Space; no change needed
to the trigger declaration on_delete_space_revoke_access_trigger beyond this
fix.
- Around line 140-158: The trigger function on_update_local_reference uses
NULL-unsafe comparisons (OLD.space_id != NEW.space_id / OLD.source_local_id !=
NEW.source_local_id) so updates that set fields to NULL can yield NULL and skip
cleanup; change the conditional to use NULL-safe IS DISTINCT FROM (e.g. IF
(OLD.space_id IS DISTINCT FROM NEW.space_id OR OLD.source_local_id IS DISTINCT
FROM NEW.source_local_id) AND public.is_last_local_reference(OLD.space_id,
OLD.source_local_id) THEN ...) so the function reliably detects changes
including to/from NULL and still calls public.is_last_local_reference with the
OLD values before deleting from public."ContentAccess"; update the
on_update_local_reference function accordingly and keep the three CREATE TRIGGER
lines unchanged.
- Line 1: Before dropping the ConceptAccess table, migrate its grants into the
unified ContentAccess scheme by inserting rows into public."ContentAccess" using
ca.account_uid joined with public."Concept" c to obtain c.space_id and
c.source_local_id (only when source_local_id IS NOT NULL), and use ON CONFLICT
DO NOTHING to avoid duplicates; then drop public."ConceptAccess" CASCADE.
Reference the ConceptAccess rows as ca and join to Concept (c) on c.id =
ca.concept_id, mapping to ContentAccess(account_uid, space_id, source_local_id).
- Around line 9-21: Before adding the new primary key "ContentAccess_pkey" on
public."ContentAccess" (account_uid, source_local_id, space_id), deduplicate
existing rows that will collapse when you replace content_id with (space_id,
source_local_id): detect duplicates on (account_uid, space_id, source_local_id)
and remove all but one row per tuple in public."ContentAccess" (for example by
keeping the latest/first by ctid or timestamp), then proceed with the UPDATE
from public."Content" and drop content_id and add the NOT NULL constraints and
primary key; ensure the duplicate-check query and the dedupe DELETE target
public."ContentAccess" so the PK creation will not fail.
In `@packages/database/supabase/schemas/concept.sql`:
- Around line 471-483: The trigger function on_delete_space_revoke_local_access
is referencing a non-existent OLD.space_id on deletes from public."Space";
update the function to use OLD.id (the Space primary key) when deleting rows
from public."ContentAccess" (i.e., DELETE FROM public."ContentAccess" WHERE
space_id = OLD.id) so the delete succeeds; keep the function name
on_delete_space_revoke_local_access and the trigger
on_delete_space_revoke_access_trigger unchanged.
- Around line 452-469: The trigger function on_update_local_reference uses
NULL-unsafe comparisons (OLD.space_id != NEW.space_id and OLD.source_local_id !=
NEW.source_local_id) so updates that set those fields to NULL can yield NULL and
skip cleanup; change those comparisons to NULL-safe "IS DISTINCT FROM" (e.g., IF
(OLD.space_id IS DISTINCT FROM NEW.space_id OR OLD.source_local_id IS DISTINCT
FROM NEW.source_local_id) AND public.is_last_local_reference(OLD.space_id,
OLD.source_local_id) THEN ...) so the delete of public."ContentAccess" runs
correctly when values change to/from NULL; keep the rest of the function and
triggers (on_update_content_trigger, on_update_concept_trigger,
on_update_document_trigger) unchanged.
packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql
Show resolved
Hide resolved
packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql
Outdated
Show resolved
Hide resolved
packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql
Show resolved
Hide resolved
packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql
Outdated
Show resolved
Hide resolved
maparent
force-pushed
the
eng-1311-unify-conceptaccess-and-contentaccess-tables
branch
from
4913e56 to
c306427
Compare
maparent
force-pushed
the
eng-1310-database-change-allow-concepts-to-exist-without-content
branch
from
edd277a to
b0c533b
Compare
Base automatically changed from
eng-1310-database-change-allow-concepts-to-exist-without-content
to
main
January 19, 2026 04:22
maparent
force-pushed
the
eng-1311-unify-conceptaccess-and-contentaccess-tables
branch
from
c306427 to
4e8f0a4
Compare
Collaborator
Author
|
@CodeRabbit review last corrections. |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/database/supabase/schemas/concept.sql`:
- Around line 452-466: The trigger function on_update_local_reference is
declared STABLE but performs a DELETE (data modification); update its volatility
to VOLATILE (or remove the STABLE clause so it uses the default VOLATILE) in the
function declaration to reflect that it changes data (leave SECURITY DEFINER,
search_path, and body as-is); ensure the change is made on the CREATE OR REPLACE
FUNCTION on_update_local_reference() declaration so the trigger behaves
correctly.
- Around line 434-446: The trigger function on_delete_local_reference is
declared STABLE but performs a DELETE; change its volatility to VOLATILE (or
remove the STABLE clause so it defaults to VOLATILE) in the CREATE OR REPLACE
FUNCTION definition; keep the body intact (calls to
public.is_last_local_reference and DELETE FROM public."ContentAccess" remain
unchanged) and ensure SECURITY DEFINER, search_path, and LANGUAGE plpgsql
clauses are preserved.
- Around line 472-484: The function on_delete_space_revoke_local_access is
declared STABLE but performs a DELETE; update its volatility to VOLATILE (or
remove the STABLE clause so default VOLATILE is used) to reflect that it
modifies data, leaving RETURNS TRIGGER, SECURITY DEFINER, and the trigger on
public."Space" unchanged; ensure the function signature
on_delete_space_revoke_local_access() and the trigger declaration still match
after the change.
♻️ Duplicate comments (1)
packages/database/supabase/migrations/20260117210851_unify_concept_content_access_tables.sql (1)
136-186: Trigger functions have the sameSTABLEvolatility issue as in the schema file.The fixes for NULL-safe comparisons (
IS DISTINCT FROMat lines 161-162) andOLD.id(line 181) are correctly applied. TheSTABLEvolatility issue is reported in the schema file. Based on learnings, schema changes should be fixed in subsequent migrations or schema definition files.
🧹 Nitpick comments (1)
packages/database/supabase/schemas/concept.sql (1)
423-432: Consider addingLIMIT 1to all subqueries for consistency.The Content subquery has
LIMIT 1but Concept and Document don't. While functionally correct (EXISTS short-circuits), addingLIMIT 1to all three maintains consistency and makes the optimization intent explicit.♻️ Suggested consistency fix
CREATE OR REPLACE FUNCTION public.is_last_local_reference(space_id_ BIGINT, source_local_id_ VARCHAR) RETURNS boolean STABLE SET search_path = '' SECURITY DEFINER LANGUAGE sql AS $$ SELECT NOT EXISTS (SELECT id FROM public."Content" WHERE space_id=space_id_ AND source_local_id=source_local_id_ LIMIT 1) - AND NOT EXISTS (SELECT id FROM public."Concept" WHERE space_id=space_id_ AND source_local_id=source_local_id_) - AND NOT EXISTS (SELECT id FROM public."Document" WHERE space_id=space_id_ AND source_local_id=source_local_id_); + AND NOT EXISTS (SELECT id FROM public."Concept" WHERE space_id=space_id_ AND source_local_id=source_local_id_ LIMIT 1) + AND NOT EXISTS (SELECT id FROM public."Document" WHERE space_id=space_id_ AND source_local_id=source_local_id_ LIMIT 1); $$;
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Collaborator
Author
|
After discussing with Trang, we thought it would be less confusing to rename ContentAccess to ResourceAccess. |
Collaborator
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@mdroidian giving up on code rabbit. |
Contributor
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://linear.app/discourse-graphs/issue/ENG-1311/unify-conceptaccess-and-contentaccess-tables
Define ContentAccess in term of source_local_id.
It can then be used to gate access to both Content and Concepts.
We decided to rename it to ResourceAccess to express this.
Note that source_local_id is not a foreign key, so delete cascades have to be handled manually.
https://www.loom.com/share/4c29f549c2a549c8a9c50dea11f02564
Summary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.