Repository navigation
OSAC-1284: Update Event and Database Migration for ProjectMembership - #787
Conversation
|
@CrystalChun: This pull request references OSAC-1284 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds migration 67 creating ChangesProject Membership Schema, DAO Errors, and Events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/database/migrations/66_create_project_memberships_tables.up.sql`:
- Around line 24-58: `project_memberships` currently has no uniqueness guarantee
for the membership subject tuple, so duplicate `(tenant, project, user)` rows
can be inserted via the `data` JSON. Add the repo’s materialized helper-table
plus trigger pattern around `project_memberships` to materialize `(tenant,
data->'spec'->>'project', data->'spec'->>'user')` and enforce a unique
constraint there, then wire the trigger logic so inserts/updates reject
duplicates. Also add a migration test that verifies the duplicate-insert path
fails, using the existing `project_memberships` migration/test conventions.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5275ef10-eb20-4dbb-a212-f3cf75b888ed
⛔ Files ignored due to path filters (2)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
internal/database/migrations.sha256internal/database/migrations/66_create_project_memberships_tables.up.sqlinternal/database/migrations/66_create_project_memberships_tables_test.goproto/private/osac/private/v1/event_type.proto
37c4d25 to
b10b457
Compare
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Review Summary
This PR establishes the database foundation for ProjectMembership - overall follows the established patterns well (materialized helper table + trigger from migration 66). However, there is a critical error code mismatch that will cause the wrong error type at runtime.
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟡 Important | 1 |
| 💡 Suggestion | 1 |
💡 AI attribution trailer
The HEAD commit uses Co-Authored-By for AI attribution. Per Red Hat convention, use Assisted-by: Claude Code <noreply@anthropic.com> instead.
What looks good
- Migration pattern matches the established
tenant_domainspattern from migration 66 - Thorough test coverage: table creation, insert/query, indexes, FK enforcement, cross-tenant isolation, column metadata, defaults, and duplicate rejection
- Proto change is minimal and correctly scoped
- The backfill line (
update project_memberships set data = data) is a smart way to trigger materialization for existing rows
| exception when unique_violation then | ||
| raise exception using | ||
| errcode = 'Z0004', | ||
| message = format('user ''%s'' already has a membership in project ''%s'' within tenant ''%s''', v_user, v_project, new.tenant); |
There was a problem hiding this comment.
🔴 Error code mismatch: trigger raises Z0004 but DAO handles Z0005
This trigger raises errcode = 'Z0004' (errNotUniqueCode), but the new DAO changes in dao_errors.go define a separate errDuplicateMembershipCode = "Z0005" with handlers in generic_dao_create.go and generic_dao_update.go.
Since the trigger raises Z0004, the existing errNotUniqueCode handler catches it and returns ErrNotUnique (the raw trigger message). The new Z0005 handler is dead code and ErrAlreadyExists is never returned for duplicate memberships.
Fix: Either change the trigger to raise Z0005 to match the DAO handler, or remove the new error code and rely on the existing Z0004/ErrNotUnique path (which is what tenant_domains uses).
There was a problem hiding this comment.
Thanks for catching this! Updated it to use the existing error code
| 'tenant_domains' | ||
| ) | ||
| ) and | ||
| c.relname not in ('notifications', 'schema_migrations', 'project_membership_subjects') |
There was a problem hiding this comment.
🟡 Redundant NOT IN clause
This adds a second NOT IN that duplicates notifications and schema_migrations from the clause above. Just add 'project_membership_subjects' to the existing list:
c.relname not in (
'notifications',
'schema_migrations',
'tenant_domains',
'project_membership_subjects'
)There was a problem hiding this comment.
Modified as suggested. Thanks for catching this!
| tenant text not null, | ||
| project text not null, | ||
| username text not null, | ||
| membership_id text not null references project_memberships(id) on delete cascade, |
There was a problem hiding this comment.
In other places we don't use the ..._id suffix. Can we rename this to just membership to make it more consistent?
There was a problem hiding this comment.
Yes thank you! Removed this suffix
| create table project_membership_subjects ( | ||
| tenant text not null, | ||
| project text not null, | ||
| username text not null, |
There was a problem hiding this comment.
I think this should be just user, as it is a reference to the user. It may be the user name, or the user identifier, but that shouldn't affect the name of this column. That way we can decide what is the best unique identifier of a user without changing the name of this column.
There was a problem hiding this comment.
That makes sense! Thanks Juan, updated it to be user
| errNotUniqueCode = "Z0004" | ||
| // errDuplicateMembershipCode is the SQLSTATE error code returned by the 'materialize_project_membership_subjects' | ||
| // trigger when an insert or update attempts to create a duplicate (tenant, project, user) tuple. | ||
| errDuplicateMembershipCode = "Z0005" |
There was a problem hiding this comment.
I think we can reuse errNotUniqueCode for this.
There was a problem hiding this comment.
Removed this and reused the not unique code. Thank you!
| exception when unique_violation then | ||
| raise exception using | ||
| errcode = 'Z0004', | ||
| message = format('user ''%s'' already has a membership in project ''%s'' within tenant ''%s''', v_user, v_project, new.tenant); |
There was a problem hiding this comment.
Can we remove the within tenant ... part? Regular users will only see a tenant, so we don't need to tell them what tenant it is.
What would be really helpful is to include in the message the name of the existing membership, something like user 'my-user' is already a member of project 'my-project' via membership 'my-membership'. Is that doable?
There was a problem hiding this comment.
That makes sense for the tenant, removed that part and modified the message to include project and membership.
Thank you!
| case errDuplicateMembershipCode: | ||
| return &ErrAlreadyExists{ | ||
| ID: id, | ||
| } |
There was a problem hiding this comment.
Aren't we loosing the nice error message that we prepared in the trigger? Can we check if if the database has populated the Message field of the error and copy it to a new Reason field of ErrAlreadyExists? We can then change the String method of ErrAlreadyExists to return that, and the gRPC server will already translate that into the appropriate gRPC error.
Also, can you add a unit test to verify this behavior, including that it generates the expected error message?
There was a problem hiding this comment.
Thank you for the suggestion! I've attempted to do this, but I'm not sure it's fully correct. How does it look?
4bf4acc to
6652a58
Compare
| } | ||
| case errNotUniqueCode: | ||
| // Project membership uniqueness violations should return ErrAlreadyExists with the custom message | ||
| if strings.Contains(pgErr.Message, "is already a member of project") { |
There was a problem hiding this comment.
I think we shouldn't check here for a substring of the message: it is brittle. As this will only happen when we explicilty set the Z.... error code I think we can assume that the message will also be something that we set explicitly. So we can just check if pgErr.Message is set, and if it is then set Reason on the error. Otherwise we set Id.
Add ProjectMembership as a valid payload type in the Event message so controllers can receive create/update/delete events for project memberships. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Create migration 67 adding project_memberships and archived_project_memberships tables with standard DAO schema and indexes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…memberships Add materialized helper table pattern to prevent duplicate project membership assignments. Uses trigger-based constraint enforcement with custom error code Z0004. Changes: - Add errDuplicateMembershipCode constant (Z0004) - Translate Z0004 to ErrAlreadyExists in create and update paths - Exclude project_membership_subjects helper table from schema validation - Add tests verifying duplicate rejection and valid multi-user scenarios Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/database/migrations/67_create_project_memberships_tables.up.sql`:
- Around line 52-55: The migration currently adds only a non-unique name index,
so update the project_memberships table definition to add the tenant-scoped
unique constraint/index for (tenant, name) using a name that includes
_unique_name_ so it matches the DAO logic in generic_dao_create and
generic_dao_update. Keep the existing non-unique indexes if needed, but ensure
the new unique constraint is the one used for name collision detection. Also
extend the project_memberships test coverage to include a same-tenant
duplicate-name negative case in addition to the existing different-tenant
behavior.
- Around line 89-100: The duplicate-membership error in the membership-check
block can emit an empty identifier because project_memberships.name may be
blank. Update the logic in the section that selects existing_membership_name
from project_membership_subjects/project_memberships so it falls back to the
membership ID when name is empty, using coalesce(nullif(pm.name, ''), pm.id) or
equivalent before the raise exception message is built.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 208223fb-c4ec-4365-9263-dbe2403c15b2
⛔ Files ignored due to path filters (2)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
internal/database/dao/dao_errors.gointernal/database/dao/generic_dao_create.gointernal/database/dao/generic_dao_update.gointernal/database/database_tool.gointernal/database/migrations.sha256internal/database/migrations/67_create_project_memberships_tables.up.sqlinternal/database/migrations/67_create_project_memberships_tables_test.goproto/private/osac/private/v1/event_type.proto
💤 Files with no reviewable changes (1)
- proto/private/osac/private/v1/event_type.proto
| create index project_memberships_by_name on project_memberships (name); | ||
| create index project_memberships_by_creator on project_memberships (creator); | ||
| create index project_memberships_by_tenant on project_memberships (tenant); | ||
| create index project_memberships_by_label on project_memberships using gin (labels); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the tenant-scoped name uniqueness constraint.
This migration only creates a non-unique project_memberships_by_name index, so two rows with the same name can still be inserted into the same tenant. That breaks the existing DAO contract in internal/database/dao/generic_dao_create.go and internal/database/dao/generic_dao_update.go, which looks for a _unique_name_ constraint to classify name collisions, and it makes the "same name in different tenants" test assert only half of the intended behavior. Please add the usual unique (tenant, name) constraint/index with a name containing _unique_name_, plus a same-tenant negative test.
🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 52-52: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
[error] 53-53: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
[error] 54-54: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
[error] 55-55: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
🤖 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 `@internal/database/migrations/67_create_project_memberships_tables.up.sql`
around lines 52 - 55, The migration currently adds only a non-unique name index,
so update the project_memberships table definition to add the tenant-scoped
unique constraint/index for (tenant, name) using a name that includes
_unique_name_ so it matches the DAO logic in generic_dao_create and
generic_dao_update. Keep the existing non-unique indexes if needed, but ensure
the new unique constraint is the one used for name collision detection. Also
extend the project_memberships test coverage to include a same-tenant
duplicate-name negative case in addition to the existing different-tenant
behavior.
| declare | ||
| existing_membership_name text; | ||
| begin | ||
| select pm.name into existing_membership_name | ||
| from project_membership_subjects pms | ||
| join project_memberships pm on pm.id = pms.membership | ||
| where pms.tenant = new.tenant and pms.project = v_project and pms."user" = v_user; | ||
|
|
||
| raise exception using | ||
| errcode = 'Z0004', | ||
| message = format('user ''%s'' is already a member of project ''%s'' via membership ''%s''', | ||
| v_user, v_project, existing_membership_name); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back to the membership ID when the name is empty.
project_memberships.name defaults to '', so this path can legitimately raise via membership '' for duplicates created without a name. Use coalesce(nullif(pm.name, ''), pm.id) (or equivalent) so the error always includes a usable identifier.
Suggested fix
- select pm.name into existing_membership_name
+ select coalesce(nullif(pm.name, ''), pm.id) into existing_membership_name
from project_membership_subjects pms
join project_memberships pm on pm.id = pms.membership
where pms.tenant = new.tenant and pms.project = v_project and pms."user" = v_user;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare | |
| existing_membership_name text; | |
| begin | |
| select pm.name into existing_membership_name | |
| from project_membership_subjects pms | |
| join project_memberships pm on pm.id = pms.membership | |
| where pms.tenant = new.tenant and pms.project = v_project and pms."user" = v_user; | |
| raise exception using | |
| errcode = 'Z0004', | |
| message = format('user ''%s'' is already a member of project ''%s'' via membership ''%s''', | |
| v_user, v_project, existing_membership_name); | |
| declare | |
| existing_membership_name text; | |
| begin | |
| select coalesce(nullif(pm.name, ''), pm.id) into existing_membership_name | |
| from project_membership_subjects pms | |
| join project_memberships pm on pm.id = pms.membership | |
| where pms.tenant = new.tenant and pms.project = v_project and pms."user" = v_user; | |
| raise exception using | |
| errcode = 'Z0004', | |
| message = format('user ''%s'' is already a member of project ''%s'' via membership ''%s''', | |
| v_user, v_project, existing_membership_name); |
🤖 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 `@internal/database/migrations/67_create_project_memberships_tables.up.sql`
around lines 89 - 100, The duplicate-membership error in the membership-check
block can emit an empty identifier because project_memberships.name may be
blank. Update the logic in the section that selects existing_membership_name
from project_membership_subjects/project_memberships so it falls back to the
membership ID when name is empty, using coalesce(nullif(pm.name, ''), pm.id) or
equivalent before the raise exception message is built.
|
Juan has already approved, merging. |
…sac-project#787) * OSAC-1284: Add ProjectMembership to Event payload Add ProjectMembership as a valid payload type in the Event message so controllers can receive create/update/delete events for project memberships. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * OSAC-1284: Add project_memberships database tables Create migration 67 adding project_memberships and archived_project_memberships tables with standard DAO schema and indexes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * OSAC-1284: Enforce unique (tenant, project, user) tuples for project memberships Add materialized helper table pattern to prevent duplicate project membership assignments. Uses trigger-based constraint enforcement with custom error code Z0004. Changes: - Add errDuplicateMembershipCode constant (Z0004) - Translate Z0004 to ErrAlreadyExists in create and update paths - Exclude project_membership_subjects helper table from schema validation - Add tests verifying duplicate rejection and valid multi-user scenarios Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…sac-project#787) * OSAC-1284: Add ProjectMembership to Event payload Add ProjectMembership as a valid payload type in the Event message so controllers can receive create/update/delete events for project memberships. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * OSAC-1284: Add project_memberships database tables Create migration 67 adding project_memberships and archived_project_memberships tables with standard DAO schema and indexes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * OSAC-1284: Enforce unique (tenant, project, user) tuples for project memberships Add materialized helper table pattern to prevent duplicate project membership assignments. Uses trigger-based constraint enforcement with custom error code Z0004. Changes: - Add errDuplicateMembershipCode constant (Z0004) - Translate Z0004 to ErrAlreadyExists in create and update paths - Exclude project_membership_subjects helper table from schema validation - Add tests verifying duplicate rejection and valid multi-user scenarios Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
4 participants
Description
Relevant changes:
ProjectMembershipas a payload type67 with project_membershipsandarchived_project_membershipstables following the standard DAO schema.ProjectMembership enables assigning and unassigning users from projects.
Testing
/cc @jhernand
Summary by CodeRabbit
New Features
Bug Fixes