Replace `Group.members` and `User.members` with `memberships` #9047

seanh · 2024-10-29T19:21:36Z

Now that GroupMembership's are going to have roles via the (not-yet-merged) GroupMembership.roles column it's no longer sufficient for code to speak in terms of "members", for example a group having a list of users as group.members. Code instead needs to speak of "memberships" so that it can refer to the role of the membership.

For example when adding a user to a group, this won't work because there's no way to specify the membership role:

group.members.append(user)

Instead code now needs to do this:

group.memberships.append(GroupMembership(group=group, user=user, roles=roles))

And similarly when reading a group's memberships, we need to read a list of GroupMembership's with their roles rather than reading a list of User's. We need Group.memberships not Group.members.

So this PR replaces the SQLAlchemy relationships Group.members and User.groups with new Group.memberships and User.memberships relationships and updates lots of code and tests to work with memberships instead of working with members and groups directly.

The approach that we're taking here where we have three ORM classes User, Group and GroupMembership for the association table is called the "association object" pattern and has its own section in the SQLAlchemy docs. It's the pattern that you're supposed to use when the association class has its own extra attributes like the upcoming GroupMembership.roles attribute.

Testing

GroupCreateService was changed so go to http://localhost:5000/admin/features and enable the group_type feature flag then go to http://localhost:5000/groups/new and test creating each of the three types of group. The group's creator should be added to the group as a member.
GroupMembersService.member_join() was changed so test visiting a private group's page as a user who isn't a member of the group and joining the group.
GroupMembersService.member_join() was changed so test visiting a private group's page and clicking the Leave this group link.

seanh · 2024-10-30T10:57:05Z

h/models/group.py

@@ -40,13 +41,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")


Adding GroupMembership.user and GroupMembership.group relationships as well. This allows you to do GroupMembership(user=user, group=group) instead of having to do GroupMembership(user_id=user.id, group_id=group.id).

seanh · 2024-10-30T10:57:39Z

h/models/group.py

-        secondary="user_group",
-        backref=sa.orm.backref("groups", order_by="Group.name"),
-    )
+    memberships = sa.orm.relationship("GroupMembership", back_populates="group")


Replacing the Group.members relationship with Group.memberships, so any code or tests that was setting or mutating Group.members now has to be updated to use Group.memberships instead.

seanh · 2024-10-30T11:02:43Z

h/models/user.py

@@ -290,6 +290,8 @@ def activate(self):

    tokens = sa.orm.relationship("Token", back_populates="user")

+    memberships = sa.orm.relationship("GroupMembership", back_populates="user")


It's not easy to see because the User.groups relationship wasn't defined here in the User class itself (it was defined only in the Group class and added to the User class via backref) but the old User.groups relationship has been removed and here I'm adding User.memberships to replace it. So any code or tests that was writing to or mutating User.groups now has to be updated to use User.memberships instead.

(Here I'm doing relationships in the same way as the modern examples in the SQLAlchemy docs where if you want the relationship to work bidirectionally you have to put it in both classes, which also seems better and more explicit.)

seanh · 2024-10-30T17:25:23Z

h/services/group_create.py

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


I don't have to pass group=group to GroupMembership(), SQLAlchemy will figure out the group automatically because I'm appending the GroupMembership to group.memberships.

This does not trigger a warning from SQLAlchemy because group.memberships is evaluated (and the autoflush triggered) before the GroupMembership object is initialized.

seanh · 2024-10-30T17:30:39Z

h/services/group_members.py

+        for membership in matching_memberships:
+            self.db.delete(membership)
+            group.memberships.remove(membership)
+            user.memberships.remove(membership)


If you just do group.memberships.remove(membership) that sets the GroupMembership's group_id to None but doesn't remove it from the DB session, and then you end up with a psycopg2.errors.NotNullViolation on the user_group.group_id column. So it's necessary to also call self.db.delete(membership).

The user.memberships.remove(membership) is also needed because removing the membership from the group's memberships doesn't seem to automatically remove it from the user's memberships and you end up with some functional tests crashing when some code later in the request cycle is reading user.memberships and trying to read user.memberships[i].group.name and crashing because one of the memberships in user.memberships has group=None.

seanh · 2024-10-30T17:33:15Z

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)


This has to be removed because Group.members is now a read-only property. I'm not replacing this with a memberships attribute in the factory: this would be pointless because models.Group.memberships defaults to [] anyway. The members attribute here was never necessary in the first place

seanh · 2024-10-30T17:35:16Z

tests/functional/api/groups/read_test.py

+        group1 = factories.Group(memberships=[GroupMembership(user=user)])
+        group2 = factories.Group(memberships=[GroupMembership(user=user)])


Now that we have the Group.memberships relationship I can do this and have the memberships be added to the DB session automatically instead of having to do separate db_session.add(GroupMembership(...)) calls.

seanh · 2024-10-30T17:41:10Z

tests/unit/h/services/group_list_test.py

-        # Remove `private_group.creator` from `private_group.members`.
-        # The creator is still attached to `private_group` as `private_group.creator`.
-        private_group.members = [
-            user for user in private_group.members if user is not private_group.creator
-        ]


Oops, this should have been removed in the previous PR. factories.Group() no longer automatically adds the creator as a member, so this line no longer does anything.

seanh · 2024-10-30T17:46:48Z

tests/unit/h/services/group_members_test.py

@@ -88,7 +85,7 @@ def test_it_does_not_remove_existing_members(
 class TestUpdateMembers:
    def test_it_adds_users_in_userids(self, factories, group_members_service):
        group = factories.OpenGroup()  # no members at outset
-        new_members = [factories.User(), factories.User()]
+        new_members = (factories.User(), factories.User())


new_members will be compared to group.members in an assert below and group.members is now a tuple so new_members also needs to be a tuple now or the assert fails

seanh · 2024-10-30T17:52:04Z

tests/unit/h/services/group_members_test.py

@@ -154,8 +154,7 @@ def test_it_does_not_add_duplicate_members(self, factories, group_members_servic
            group, [new_member.userid, new_member.userid]
        )

-        assert group.members == [new_member]
-        assert len(group.members) == 1


Removed the pointless second assert

seanh · 2024-10-30T17:53:10Z

tests/unit/h/services/group_members_test.py


        group_members_service.update_members(group, [])

-        assert not group.members  # including the creator


The comment is no longer applicable now that the factories no longer add creators as group members automatically.

seanh · 2024-10-30T20:16:59Z

tests/functional/api/groups/members_test.py

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


This can alternatively be written:

db_session.add(GroupMembership(user=user, group=group))

But it's slightly longer and requires you to have the db_session 🤷‍♂️

When adding multiple memberships the difference is even larger:

group.memberships.extend([GroupMembership(user=user), GroupMembership(user=another_user)]) for user in (user, another_user): db_session.add(GroupMembership(user=user, group=group))

Also when creating a new group and adding a membership at the same time:

db_session.add(Group(memberships=[GroupMembership(user=user)])) group = Group() db_session.add_all([group, GroupMembership(user=user, group=group)])

seanh · 2024-10-31T11:19:23Z

h/models/group.py

+    @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)


There's a lot of code (and a lot of tests) that read Group.members and this avoids having to do [membership.user for membership in group.memberships] all the time.

An alternative approach would be SQLAlchemy's association proxy extension which would also allow the Group.member relationship to be writeable (in particular see the section Simplifying Association Objects, but at a glance this seems more trouble than it's worth.

seanh · 2024-10-31T11:45:28Z

h/services/group_members.py

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


Again avoiding a warning from SQLAlchemy for the same reason.

Change `factories.Group()` to *not* automatically add the group's creator as a member of the group. Future commits need to replace the `group.members` relation with a `group.memberships` relation (which is a list of `GroupMembership`'s rather than a list of `User`'s). See <#9047>. This is necessary because `GroupMembership`'s will in future have additional attributes (e.g. `roles`) and to add a user to a group with a particular role it'll be necessary to append a `GroupMembership` with that role to `group.memberships`, it's not enough to append a `User` to `group.members` because the role is an attribute of the membership not an attribute of the user, so we need to actually create a `GroupMembership` with the desired role and append that. With this change it'll no longer be possible for `factories.Group`'s `add_creator_as_member()` to add the creator as a member. For example this kind of thing won't work: @factory.post_generation def add_creator_as_member( # pylint:disable=no-self-argument obj, _create, _extracted, **_kwargs ): if ( obj.creator and obj.creator not in obj.members ): obj.memberships.append( models.GroupMembership( group=obj, user=obj.creator, role="owner ) ) The problem is that the `GroupMembership` that's been appended will not have been added to the DB session, which causes this SQLAlchemy error: https://docs.sqlalchemy.org/en/20/errors.html#object-is-being-merged-into-a-session-along-the-backref-cascade Or alternatively you get a `NotNullViolation`, depending. Nor can `factories.Group.add_creator_as_member()` simply add the `GroupMembership` to the DB session: it doesn't have access to the DB session (and this wouldn't necessarily get around `NotNullViolation`'s anyway). Removing this feels like a good direction to me because `add_creator_as_member()` seems too clever for a test factory, and my experience with test factories is that having them do extra things like this automatically usually ends up creating problems and it's better to keep the factories simpler and just make certain tests do more work. There looks to have been a bunch of tests that were implicitly or explicitly relying on the fact that the factory adds the group's creator as a member, even when this concept is irrelevant to the test at hand. So I think removing this is a good thing. The current behavior is also potentially confusing when you do something like `factories.Group(members=[...])` and then it auto-generates a user to be the group's `creator` and adds them to the group's members even though that user wasn't in the members list that was passed in.

seanh requested a review from marcospri October 29, 2024 19:21

seanh changed the title ~~Replace Group.members and User.members with memberships~~ Replace Group.members and User.members with memberships Oct 30, 2024

seanh commented Oct 30, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch 7 times, most recently from 5f535d9 to 14b99b3 Compare October 30, 2024 17:20

seanh commented Oct 30, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch from 14b99b3 to 7cadfc3 Compare October 30, 2024 17:50

seanh commented Oct 30, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch from 7cadfc3 to a07aaaf Compare October 31, 2024 11:17

seanh commented Oct 31, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch from a07aaaf to 557a779 Compare October 31, 2024 11:43

seanh commented Oct 31, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch 4 times, most recently from 4e42919 to 89cb5ba Compare October 31, 2024 11:55

seanh force-pushed the simplify-group-factory branch from 49cfac0 to a6c026e Compare October 31, 2024 14:23

seanh mentioned this pull request Oct 31, 2024

Simplify factories.Group: don't add creators as members #9045

Open

seanh force-pushed the simplify-group-factory branch 3 times, most recently from 126677a to 924d72e Compare October 31, 2024 16:01

seanh force-pushed the simplify-group-factory branch from 924d72e to 1c1742b Compare October 31, 2024 16:05

seanh force-pushed the simplify-group-factory branch from 1c1742b to 4daaebd Compare October 31, 2024 16:05

seanh force-pushed the simplify-group-factory branch from 4daaebd to 2821513 Compare October 31, 2024 16:10

seanh force-pushed the simplify-group-factory branch from 2821513 to a3d6471 Compare October 31, 2024 16:20

seanh force-pushed the simplify-group-factory branch from a3d6471 to 2131380 Compare October 31, 2024 16:23

seanh force-pushed the memberships-relationship branch 4 times, most recently from d244229 to 2324e08 Compare October 31, 2024 16:41

seanh marked this pull request as ready for review October 31, 2024 16:49

seanh force-pushed the simplify-group-factory branch from 2131380 to eca9ff7 Compare November 1, 2024 10:11

Replace Group.members and User.members with memberships

b3e3cfd

seanh force-pushed the memberships-relationship branch from 2324e08 to b3e3cfd Compare November 1, 2024 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Group.members` and `User.members` with `memberships` #9047

Replace `Group.members` and `User.members` with `memberships` #9047

seanh commented Oct 29, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 31, 2024 •

edited

Loading

seanh Oct 31, 2024

		@@ -290,6 +290,8 @@ def activate(self):

		tokens = sa.orm.relationship("Token", back_populates="user")

		memberships = sa.orm.relationship("GroupMembership", back_populates="user")

		group1 = factories.Group(memberships=[GroupMembership(user=user)])
		group2 = factories.Group(memberships=[GroupMembership(user=user)])


		group_members_service.update_members(group, [])

		assert not group.members # including the creator

Replace Group.members and User.members with memberships #9047

Are you sure you want to change the base?

Replace Group.members and User.members with memberships #9047

Conversation

seanh commented Oct 29, 2024 • edited Loading

Testing

seanh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `Group.members` and `User.members` with `memberships` #9047

Replace `Group.members` and `User.members` with `memberships` #9047

seanh commented Oct 29, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 30, 2024 •

edited

Loading

seanh Oct 31, 2024 •

edited

Loading