Add GroupMembership.roles column to model
#9029
Conversation
| class GroupMembershipRoles(enum.StrEnum): | ||
| """The valid role strings that're allowed in the GroupMembership.roles column.""" | ||
|
|
||
| MEMBER = "member" | ||
| MODERATOR = "moderator" | ||
| ADMIN = "admin" | ||
| OWNER = "owner" |
There was a problem hiding this comment.
Pulled these four strings out into an enum because they'll inevitably need to be referenced from elsewhere in the code later
| sa.CheckConstraint( | ||
| " OR ".join( | ||
| f"""(roles = '["{role}"]'::jsonb)""" for role in GroupMembershipRoles | ||
| ), | ||
| name="validate_role_strings", | ||
| ), |
There was a problem hiding this comment.
This both ensures that roles has length 1 and that the one string is "member", "moderator", "admin" or "owner".
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 "{authority}:{role}"-formatted roles and consider only the core "{role}" ones, and enforce that there must be exactly one of those and it must be "member", "moderator", "admin" or "owner". That might be difficult to write as an SQL expression, so at that point perhaps we'll just remove this constraint and replace it with an SQLAlchemy @validates method (though I think those only work for code that's using the ORM).
There was a problem hiding this comment.
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.
In future if we want to allow LMS to insert custom roles the constraint will have to ignore all
Maybe we are overthinking this, we could use another array "custom_roles" or similar.
| sa.CheckConstraint( | ||
| " OR ".join( | ||
| f"""(roles = '["{role}"]'::jsonb)""" for role in GroupMembershipRoles | ||
| ), | ||
| name="validate_role_strings", | ||
| ), |
There was a problem hiding this comment.
In future if we want to allow LMS to insert custom roles the constraint will have to ignore all
Maybe we are overthinking this, we could use another array "custom_roles" or similar.
| JSONB, | ||
| sa.CheckConstraint( | ||
| " OR ".join( | ||
| f"""(roles = '["{role}"]'::jsonb)""" for role in GroupMembershipRoles |
There was a problem hiding this comment.
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.
No description provided.