Replace Group.members and User.members with memberships

Author: seanh

h/models/group.py

@@ -5,9 +5,11 @@
 import slugify
 import sqlalchemy as sa
 from sqlalchemy.dialects.postgresql import JSONB
+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
@@ -49,14 +51,19 @@ class GroupMembership(Base):
     __table_args__ = (sa.UniqueConstraint("user_id", "group_id"),)
 
     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")
+
     roles = sa.Column(
         JSONB,
         sa.CheckConstraint(
@@ -159,12 +166,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

@@ -126,9 +126,7 @@ def _create(self, name, userid, type_flags, scopes, **kwargs):
         # self.publish() or `return group`.
         self.db.flush()
 
-        self.db.add(
-            GroupMembership(user_id=creator.id, group_id=group.id, roles=["owner"])
-        )
+        group.memberships.append(GroupMembership(user=group.creator, roles=["owner"]))
         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,9 @@ 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.extend([user, factories.User()])
+        group.memberships.extend(
+            [GroupMembership(user=user), GroupMembership(user=factories.User())]
+        )
         db_session.commit()
 
         res = app.get(
@@ -251,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)
@@ -38,8 +36,9 @@ def test_it_overrides_authority_param_with_user_authority(
         self, app, factories, db_session, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        group1 = factories.Group(authority=user.authority)
-        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
+        group1 = factories.Group(
+            authority=user.authority, memberships=[GroupMembership(user=user)]
+        )
         db_session.commit()
 
         res = app.get("/api/groups?authority=whatever.com", headers=token_auth_header)
@@ -94,8 +93,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(creator=user)
-        db_session.add(GroupMembership(group_id=group.id, user_id=user.id))
+        group = factories.Group(creator=user, memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get(
@@ -109,7 +107,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):
@@ -147,8 +149,9 @@ def groups(factories):
 
 @pytest.fixture
 def user(groups, db_session, factories):
-    user = factories.User()
-    user.groups = groups
+    user = factories.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)
@@ -111,9 +112,15 @@ def test_search_url(self, pyramid_request, matched_route, matchdict, search_url)
         assert result["search_url"] == search_url
 
     @pytest.fixture
-    def user(self, factories):
+    def user(self, db_session, factories):
         user = factories.User()
-        user.groups = [factories.Group(creator=user), factories.Group()]
+
+        with db_session.no_autoflush:
+            user.memberships = [
+                GroupMembership(group=group)
+                for group in [factories.Group(creator=user), factories.Group()]
+            ]
+
         return user
 
     @pytest.fixture(autouse=True)