-
Notifications
You must be signed in to change notification settings - Fork 38
fix(groups): remove Group->user edge #2304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the direct Group<->User edge relationship in favor of using only the GroupMembership join table to handle group-user associations. This change addresses an issue where the direct edge couldn't properly handle the DeletedAt condition, causing the List repository method to return deleted items. By removing the edge and using explicit join table relationships, the conditions can be properly specified.
- Replaced direct Group<->User edge with explicit GroupMembership relationships
- Updated filtering logic to use GroupMembership predicates with DeletedAt conditions
- Modified group creation flow to explicitly create GroupMembership records
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
app/controlplane/pkg/data/group.go | Updated query methods to use GroupMembership relationships and added proper DeletedAt filtering |
app/controlplane/pkg/data/ent/schema/user.go | Removed direct Group edge, replaced with GroupMembership edge |
app/controlplane/pkg/data/ent/schema/group.go | Removed direct User edge, replaced with GroupMembership edge |
app/controlplane/pkg/data/ent/*.go | Generated code changes removing Group<->User edges and replacing with GroupMembership relationships |
app/controlplane/pkg/biz/group.go | Added UserID field to ListGroupOpts for filtering |
Comments suppressed due to low confidence (1)
app/controlplane/pkg/data/group.go:252
- [nitpick] The variable name 'grUerr' is unclear and inconsistent with typical Go naming conventions. Consider renaming it to 'createErr' or 'err' to better reflect its purpose.
if _, grUerr := tx.GroupMembership.Create().
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ptal at my comments
app/controlplane/pkg/data/group.go
Outdated
groupmembership.UserIDEQ(*opts.UserID), | ||
). | ||
// Set user as group maintainer | ||
if _, grUerr := tx.GroupMembership.Create(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why of this difference? Update vs Create?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous approach used the Create query to directly create the related membership items, and then editing it to update the maintainer role. This approach is not possible after removing the users
edge.
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
This PR removes the Group<->User edge, since there is no way to navigate the edge with DeletedAt=nil condition. By removing the edge, using the join table is required and we can specify the condition clearly.
This issue was making the
List
repository method to return deleted items, causing additional exceptions.Additionally, I've also added the ability to search groups by user ID.