-
Notifications
You must be signed in to change notification settings - Fork 427
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
Add GroupMembership.roles
column to model
#9029
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
import slugify | ||
import sqlalchemy as sa | ||
from sqlalchemy.dialects.postgresql import JSONB | ||
|
||
from h import pubid | ||
from h.db import Base, mixins | ||
|
@@ -33,6 +34,15 @@ class WriteableBy(enum.Enum): | |
members = "members" | ||
|
||
|
||
class GroupMembershipRoles(enum.StrEnum): | ||
"""The valid role strings that're allowed in the GroupMembership.roles column.""" | ||
|
||
MEMBER = "member" | ||
MODERATOR = "moderator" | ||
ADMIN = "admin" | ||
OWNER = "owner" | ||
|
||
|
||
class GroupMembership(Base): | ||
__tablename__ = "user_group" | ||
|
||
|
@@ -47,6 +57,17 @@ class GroupMembership(Base): | |
nullable=False, | ||
index=True, | ||
) | ||
roles = sa.Column( | ||
JSONB, | ||
sa.CheckConstraint( | ||
" OR ".join( | ||
f"""(roles = '["{role}"]'::jsonb)""" for role in GroupMembershipRoles | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think using a loop here is not the best choice, it gives the impression that things will just work but they won't. Listing them all will make it more obvious that a migration is needed as the model definition of the constraint needed a change. |
||
), | ||
name="validate_role_strings", | ||
), | ||
Comment on lines
+62
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This both ensures that This'll be validated at the API level as well, but I like having the constraint at the DB level so that no one accessing the DB directly, or Python code other than the API, or a migration, etc can enter an invalid value. Doing this as an SQL-level constraint is slightly inconvenient because if we want to widen the constraint in future we'll have to do a migration. But it'll be an easy migration, and this ensures that nothing can insert an invalid value (not even some non-ORM code e.g. in a migration or something). In future if we want to allow LMS to insert custom roles the constraint will have to ignore all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P.S. I'm not sure if SQLAlchemy has any other way to do an enum for the strings inside of a JSON array, I think probably not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe we are overthinking this, we could use another array "custom_roles" or similar. |
||
server_default=sa.text("""'["member"]'::jsonb"""), | ||
nullable=False, | ||
) | ||
|
||
|
||
class Group(Base, mixins.Timestamps): | ||
|
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.
Pulled these four strings out into an enum because they'll inevitably need to be referenced from elsewhere in the code later