Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion h/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
from h.models.feature import Feature
from h.models.feature_cohort import FeatureCohort, FeatureCohortUser
from h.models.flag import Flag
from h.models.group import Group, GroupMembership
from h.models.group import Group, GroupMembership, GroupMembershipRoles
from h.models.group_scope import GroupScope
from h.models.job import Job
from h.models.organization import Organization
Expand Down
21 changes: 21 additions & 0 deletions h/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Comment on lines +37 to +43
Copy link
Contributor Author

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



class GroupMembership(Base):
__tablename__ = "user_group"

Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

server_default=sa.text("""'["member"]'::jsonb"""),
nullable=False,
)


class Group(Base, mixins.Timestamps):
Expand Down
59 changes: 59 additions & 0 deletions tests/unit/h/models/group_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from sqlalchemy.exc import IntegrityError

from h import models
from h.models.group import (
Expand Down Expand Up @@ -218,6 +219,64 @@ def test_non_public_group():
assert not group.is_public


class TestGroupMembership:
def test_defaults(self, db_session, user, group):
membership = models.GroupMembership(user_id=user.id, group_id=group.id)
db_session.add(membership)

db_session.flush()
assert membership.id
assert membership.user_id == user.id
assert membership.group_id == group.id
assert membership.roles == ["member"]

@pytest.mark.parametrize(
"roles",
(
["member"],
["moderator"],
["admin"],
["owner"],
),
)
def test_custom_roles(self, db_session, user, group, roles):
membership = models.GroupMembership(
user_id=user.id, group_id=group.id, roles=roles
)
db_session.add(membership)

db_session.flush()
assert membership.roles == roles

@pytest.mark.parametrize(
"roles",
(
["unknown_role"],
["moderator", "admin"], # Two valid roles, only one role is allowed.
[], # Every membership must have at least one role.
),
)
def test_invalid_roles(self, db_session, user, group, roles):
membership = models.GroupMembership(
user_id=user.id, group_id=group.id, roles=roles
)
db_session.add(membership)

with pytest.raises(
IntegrityError,
match='new row for relation "user_group" violates check constraint "ck__user_group__validate_role_strings"',
):
db_session.flush()

@pytest.fixture
def user(self, factories):
return factories.User()

@pytest.fixture
def group(self, factories):
return factories.Group()


@pytest.fixture()
def organization(factories):
return factories.Organization()
Loading