Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
415 changes: 236 additions & 179 deletions internal/api/osac/private/v1/event_type.pb.go

Large diffs are not rendered by default.

414 changes: 235 additions & 179 deletions internal/api/osac/private/v1/event_type_protoopaque.pb.go

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions internal/database/dao/dao_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,17 @@ type ErrAlreadyExists struct {
// Name is set when the violation is caused by a name uniqueness constraint rather than a primary key
// collision. When set, error messages should reference the name instead of the identifier.
Name string

// Reason is set when a custom error message is provided by a database trigger (e.g., for uniqueness
// violations with additional context). When set, this message is returned instead of the default message.
Reason string
}

// Error returns the error message.
func (e *ErrAlreadyExists) Error() string {
if e.Reason != "" {
return e.Reason
}
if e.Name != "" {
return fmt.Sprintf("object with name '%s' already exists", e.Name)
}
Expand Down
12 changes: 10 additions & 2 deletions internal/database/dao/generic_dao_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,16 @@ func (r *CreateRequest[O]) translateError(ctx context.Context, id, name, tenant
return &ErrReference{}
}
case errNotUniqueCode:
return &ErrNotUnique{
Reason: pgErr.Message,
// When the trigger provides a custom message, use it as Reason.
// If no message is provided, fall back to ID.
if pgErr.Message != "" {
return &ErrAlreadyExists{
ID: id,
Reason: pgErr.Message,
}
}
return &ErrAlreadyExists{
ID: id,
}
}
return err
Expand Down
12 changes: 10 additions & 2 deletions internal/database/dao/generic_dao_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,16 @@ func (r *UpdateRequest[O]) translateError(ctx context.Context, id, name, tenant
Fields: fields,
}
case errNotUniqueCode:
return &ErrNotUnique{
Reason: pgErr.Message,
// When the trigger provides a custom message, use it as Reason.
// If no message is provided, fall back to ID.
if pgErr.Message != "" {
return &ErrAlreadyExists{
ID: id,
Reason: pgErr.Message,
}
}
return &ErrAlreadyExists{
ID: id,
}
default:
return err
Expand Down
3 changes: 2 additions & 1 deletion internal/database/database_tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,8 @@ func (t *tool) listObjectTables(ctx context.Context, pool *pgxpool.Pool) (result
c.relname not in (
'notifications',
'schema_migrations',
'tenant_domains'
'tenant_domains',
'project_membership_subjects'
)
order by
c.relname
Expand Down
2 changes: 1 addition & 1 deletion internal/database/migrations.sha256
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3edebc9c801b9aff1c40e7ce405dccb9642cc494817dd16da29695e10ff4ed28
763636618c139b4e2822f2d82c41adee6d00090d23d003548785af4a2fd17aa7
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
--
-- Copyright (c) 2026 Red Hat Inc.
--
-- Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
--
-- http://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
-- an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
-- specific language governing permissions and limitations under the License.
--

-- Create the project_memberships tables:
--
-- This migration establishes the database schema for ProjectMembership objects following the generic schema pattern.
-- ProjectMemberships define which users have access to a project and what their role is (viewer or manager).
--
-- The data column stores:
-- - spec: ProjectMembershipSpec (project, role, user)
-- - status: ProjectMembershipStatus (state, message)
-- as JSONB.
--
create table project_memberships (
id text not null primary key,
name text not null default '',
creation_timestamp timestamp with time zone not null default now(),
deletion_timestamp timestamp with time zone not null default 'epoch',
finalizers text[] not null default '{}',
creator text not null default '',
tenant text not null default '',
labels jsonb not null default '{}'::jsonb,
annotations jsonb not null default '{}'::jsonb,
data jsonb not null,
version integer not null default 0
);

create table archived_project_memberships (
id text not null,
name text not null default '',
creation_timestamp timestamp with time zone not null,
deletion_timestamp timestamp with time zone not null,
archival_timestamp timestamp with time zone not null default now(),
creator text not null default '',
tenant text not null default '',
labels jsonb not null default '{}'::jsonb,
annotations jsonb not null default '{}'::jsonb,
data jsonb not null,
version integer not null default 0
);

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);
Comment on lines +52 to +55

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.


-- Project memberships must belong to a specific organization (tenant).
alter table project_memberships add constraint project_memberships_tenant_fk foreign key (tenant) references tenants (id);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

-- Helper table to enforce uniqueness of (tenant, project, user) tuples.
-- A user can only have one membership per project within a tenant.
create table project_membership_subjects (
tenant text not null,
project text not null,
"user" text not null,
membership text not null references project_memberships(id) on delete cascade,
primary key (tenant, project, "user")
);
create index project_membership_subjects_by_membership on project_membership_subjects (membership);

-- Trigger function that materializes the (tenant, project, user) tuple from the JSONB data column.
create function materialize_project_membership_subjects() returns trigger as $$
declare
v_project text;
v_user text;
begin
-- Delete stale rows for this membership:
delete from project_membership_subjects where membership = new.id;

-- Extract project and user from the spec:
v_project := new.data->'spec'->>'project';
v_user := new.data->'spec'->>'user';

-- Insert the new tuple, catching duplicates:
begin
insert into project_membership_subjects (tenant, project, "user", membership)
values (new.tenant, v_project, v_user, new.id);
exception when unique_violation then
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);
Comment on lines +89 to +100

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

end;
end;

return new;
end;
$$ language plpgsql;

create trigger materialize_project_membership_subjects
after insert or update on project_memberships
for each row
execute function materialize_project_membership_subjects();

-- Backfill existing rows to populate the helper table:
update project_memberships set data = data;
Loading
Loading