Replace Group.members and User.members with memberships

h/models/group.py

@@ -4,9 +4,11 @@
 
 import slugify
 import sqlalchemy as sa
+from sqlalchemy.orm import relationship
 
 from h import pubid
 from h.db import Base, mixins
+from h.models.user import User
 from h.util.group import split_groupid
 
 GROUP_NAME_MIN_LENGTH = 3
@@ -40,13 +42,16 @@ class GroupMembership(Base):
 
     id = sa.Column("id", sa.Integer, autoincrement=True, primary_key=True)
     user_id = sa.Column("user_id", sa.Integer, sa.ForeignKey("user.id"), nullable=False)
+    user = relationship("User", back_populates="memberships")
+
     group_id = sa.Column(
         "group_id",
         sa.Integer,
         sa.ForeignKey("group.id", ondelete="cascade"),
         nullable=False,
         index=True,
     )
+    group = relationship("Group", back_populates="memberships")
 
 
 class Group(Base, mixins.Timestamps):
@@ -138,12 +143,31 @@ def groupid(self, value):
             self.authority_provided_id = groupid_parts["authority_provided_id"]
             self.authority = groupid_parts["authority"]
 
-    # Group membership
-    members = sa.orm.relationship(
-        "User",
-        secondary="user_group",
-        backref=sa.orm.backref("groups", order_by="Group.name"),
-    )
+    memberships = sa.orm.relationship("GroupMembership", back_populates="group")
+
+    @property
+    def members(self) -> tuple[User, ...]:
+        """
+        Return a tuple of this group's members.
+
+        This is a convenience property for when you want to access a group's
+        members (User objects) rather than its memberships (GroupMembership
+        objects).
+
+        This is not an SQLAlchemy relationship! SQLAlchemy emits a warning if
+        you try to have both Group.memberships and a Group.members
+        relationships at the same time because it can result in reads returning
+        conflicting data and in writes causing integrity errors or unexpected
+        inserts or deletes. See:
+
+        https://docs.sqlalchemy.org/en/20/orm/basic_relationships.html#combining-association-object-with-many-to-many-access-patterns
+
+        Since this is just a normal Python property setting or mutating it
+        (e.g. `group.members = [...]` or `group.members.append(...)`) wouldn't
+        be registered with SQLAlchemy and the changes wouldn't be saved to the
+        DB. So this is a read-only property that returns an immutable tuple.
+        """
+        return tuple(membership.user for membership in self.memberships)
 
     scopes = sa.orm.relationship(
         "GroupScope", backref="group", cascade="all, delete-orphan"

h/models/user.py

@@ -1,5 +1,6 @@
 import datetime
 import re
+from typing import TYPE_CHECKING
 
 import sqlalchemy as sa
 from sqlalchemy.ext.declarative import declared_attr
@@ -9,6 +10,10 @@
 from h.exceptions import InvalidUserId
 from h.util.user import format_userid, split_user
 
+if TYPE_CHECKING:
+    from models.group import Group  # pragma: nocover
+
+
 USERNAME_MIN_LENGTH = 3
 USERNAME_MAX_LENGTH = 30
 USERNAME_PATTERN = "(?i)^[A-Z0-9._]+$"
@@ -290,6 +295,32 @@ def activate(self):
 
     tokens = sa.orm.relationship("Token", back_populates="user")
 
+    memberships = sa.orm.relationship("GroupMembership", back_populates="user")
+
+    @property
+    def groups(self) -> tuple["Group", ...]:
+        """
+        Return a tuple of this user's groups.
+
+        This is a convenience property for when you want to access a user's
+        groups (Group objects) rather than its memberships (GroupMembership
+        objects).
+
+        This is not an SQLAlchemy relationship! SQLAlchemy emits a warning if
+        you try to have both User.memberships and a User.groups relationships
+        at the same time because it can result in reads returning conflicting
+        data and in writes causing integrity errors or unexpected inserts or
+        deletes. See:
+
+        https://docs.sqlalchemy.org/en/20/orm/basic_relationships.html#combining-association-object-with-many-to-many-access-patterns
+
+        Since this is just a normal Python property setting or mutating it
+        (e.g. `user.groups = [...]` or `user.groups.append(...)`) wouldn't
+        be registered with SQLAlchemy and the changes wouldn't be saved to the
+        DB. So this is a read-only property that returns an immutable tuple.
+        """
+        return tuple(membership.group for membership in self.memberships)
+
     @sa.orm.validates("email")
     def validate_email(self, _key, email):
         if email is None:

h/services/group_create.py

@@ -1,7 +1,7 @@
 from functools import partial
 
 from h import session
-from h.models import Group, GroupScope
+from h.models import Group, GroupMembership, GroupScope
 from h.models.group import GROUP_TYPE_FLAGS
 
 
@@ -126,7 +126,7 @@ def _create(self, name, userid, type_flags, scopes, **kwargs):
         # self.publish() or `return group`.
         self.db.flush()
 
-        group.members.append(group.creator)
+        group.memberships.append(GroupMembership(user=group.creator))
         self.publish("group-join", group.pubid, group.creator.userid)
 
         return group

h/services/group_members.py

@@ -1,6 +1,7 @@
 from functools import partial
 
 from h import session
+from h.models import GroupMembership
 
 
 class GroupMembersService:
@@ -60,18 +61,25 @@ def member_join(self, group, userid):
         if user in group.members:
             return
 
-        group.members.append(user)
+        group.memberships.append(GroupMembership(user=user))
 
         self.publish("group-join", group.pubid, userid)
 
     def member_leave(self, group, userid):
         """Remove `userid` from the member list of `group`."""
         user = self.user_fetcher(userid)
 
-        if user not in group.members:
+        matching_memberships = [
+            membership for membership in group.memberships if membership.user == user
+        ]
+
+        if not matching_memberships:
             return
 
-        group.members.remove(user)
+        for membership in matching_memberships:
+            self.db.delete(membership)
+            group.memberships.remove(membership)
+            user.memberships.remove(membership)
 
         self.publish("group-leave", group.pubid, userid)
 

tests/common/factories/group.py

@@ -20,7 +20,6 @@ class Meta:
     joinable_by = JoinableBy.authority
     readable_by = ReadableBy.members
     writeable_by = WriteableBy.members
-    members = factory.LazyFunction(list)
     authority_provided_id = Faker("hexify", text="^" * 30)
     enforce_scope = True
 

tests/functional/api/bulk/annotation_test.py

@@ -2,6 +2,8 @@
 
 import pytest
 
+from h.models import GroupMembership
+
 
 @pytest.mark.usefixtures("with_clean_db")
 class TestBulkAnnotation:
@@ -23,7 +25,7 @@ def test_it_accepts_a_valid_request(self, make_request, factories):
         group = factories.Group(
             authority="lms.hypothes.is",
             authority_provided_id="1234567890",
-            members=[user],
+            memberships=[GroupMembership(user=user)],
         )
         annotation_slim = factories.AnnotationSlim(
             user=user,

tests/functional/api/groups/members_test.py

@@ -10,9 +10,14 @@ class TestReadMembers:
     def test_it_returns_list_of_members_for_restricted_group_without_authn(
         self, app, factories, db_session
     ):
-        group = factories.RestrictedGroup()
-        group.members = [factories.User(), factories.User(), factories.User()]
+        group = factories.RestrictedGroup(
+            memberships=[
+                GroupMembership(user=user)
+                for user in factories.User.create_batch(size=3)
+            ]
+        )
         db_session.commit()
+
         res = app.get("/api/groups/{pubid}/members".format(pubid=group.pubid))
 
         assert res.status_code == 200
@@ -22,7 +27,7 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group(
         self, app, factories, db_session, group, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        group.members.append(user)
+        group.memberships.append(GroupMembership(user=user))
         db_session.commit()
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
@@ -238,7 +243,7 @@ def auth_client_header(auth_client):
 @pytest.fixture
 def group(db_session, factories):
     group = factories.Group()
-    db_session.add(GroupMembership(group_id=group.id, user_id=group.creator.id))
+    group.memberships.append(GroupMembership(user=group.creator))
     db_session.commit()
     return group
 
@@ -253,7 +258,7 @@ def third_party_group(db_session, factories):
 @pytest.fixture
 def group_member(group, db_session, factories):
     user = factories.User()
-    group.members.append(user)
+    group.memberships.append(GroupMembership(user=user))
     db_session.commit()
     return user
 

tests/functional/api/groups/read_test.py

@@ -21,10 +21,8 @@ def test_it_returns_private_groups_along_with_world_groups(
         self, app, factories, db_session, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        group1 = factories.Group()
-        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
-        group2 = factories.Group()
-        db_session.add(GroupMembership(group_id=group2.id, user_id=user.id))
+        group1 = factories.Group(memberships=[GroupMembership(user=user)])
+        group2 = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get("/api/groups", headers=token_auth_header)
@@ -39,8 +37,7 @@ def test_it_overrides_authority_param_with_user_authority(
     ):
         user, _ = user_with_token
         # This group will be created with the user's authority
-        group1 = factories.Group()
-        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
+        group1 = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get("/api/groups?authority=whatever.com", headers=token_auth_header)
@@ -95,8 +92,7 @@ def test_it_returns_http_200_for_private_group_with_creator_authentication(
         self, app, user_with_token, token_auth_header, factories, db_session
     ):
         user, _ = user_with_token
-        group = factories.Group()
-        db_session.add(GroupMembership(group_id=group.id, user_id=user.id))
+        group = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get(
@@ -110,7 +106,7 @@ def test_it_returns_http_200_for_private_group_with_member_authentication(
     ):
         user, _ = user_with_token
         group = factories.Group()
-        group.members.append(user)
+        group.memberships.append(GroupMembership(user=user))
         db_session.commit()
 
         res = app.get(

tests/functional/api/profile_test.py

@@ -1,5 +1,7 @@
 import pytest
 
+from h.models import GroupMembership
+
 
 class TestGetProfile:
     def test_it_returns_profile_with_single_group_when_not_authd(self, app):
@@ -148,7 +150,7 @@ def groups(factories):
 @pytest.fixture
 def user(groups, db_session, factories):
     user = factories.User()
-    user.groups = groups
+    user.memberships = [GroupMembership(group=group) for group in groups]
     db_session.commit()
     return user
 

tests/unit/h/jinja2_extensions/navbar_data_test.py

@@ -4,6 +4,7 @@
 from h_matchers import Any
 
 from h.jinja_extensions.navbar_data import navbar_data
+from h.models import GroupMembership
 
 
 class TestNavbarData:
@@ -53,9 +54,9 @@ def test_it(self, pyramid_request, user):
     def test_it_with_a_group_with_no_creator(
         self, pyramid_request, user, factories, group_list_service
     ):
-        user.groups = group_list_service.associated_groups.return_value = [
-            factories.Group(creator=None)
-        ]
+        group = factories.Group(creator=None)
+        group_list_service.associated_groups.return_value = [group]
+        user.memberships = [GroupMembership(group=group, user=user)]
         pyramid_request.user = user
 
         result = navbar_data(pyramid_request)
@@ -113,7 +114,14 @@ def test_search_url(self, pyramid_request, matched_route, matchdict, search_url)
     @pytest.fixture
     def user(self, factories):
         user = factories.User()
-        user.groups = [factories.Group(creator=user), factories.Group()]
+
+        user.memberships.extend(
+            [
+                GroupMembership(group=group)
+                for group in [factories.Group(creator=user), factories.Group()]
+            ]
+        )
+
         return user
 
     @pytest.fixture(autouse=True)

tests/unit/h/models/group_test.py

@@ -218,6 +218,38 @@ def test_non_public_group():
     assert not group.is_public
 
 
+def test_members(factories):
+    users = factories.User.build_batch(size=2)
+    group = factories.Group.build(
+        memberships=[
+            models.GroupMembership(user=users[0]),
+            models.GroupMembership(user=users[1]),
+        ]
+    )
+
+    assert group.members == tuple(users)
+
+
+def test_members_is_readonly(factories):
+    group = factories.Group.build()
+    new_members = (factories.User.build(),)
+
+    with pytest.raises(
+        AttributeError, match="^property 'members' of 'Group' object has no setter$"
+    ):
+        group.members = new_members
+
+
+def test_members_is_immutable(factories):
+    group = factories.Group.build()
+    new_member = factories.User.build()
+
+    with pytest.raises(
+        AttributeError, match="^'tuple' object has no attribute 'append'$"
+    ):
+        group.members.append(new_member)
+
+
 @pytest.fixture()
 def organization(factories):
     return factories.Organization()