France-ioi / AlgoreaBackend

Backend for the new Algorea platform
MIT License
1 stars 2 forks source link

Pending group requests without corresponding membership changes logged #1123

Open zenovich opened 1 month ago

zenovich commented 1 month ago

In our dev DB (don't now about the prod) there is a pending invitation without corresponding group_membership_changes logged:

mysql> select count(*), (SELECT action FROM group_membership_changes where group_membership_changes.group_id=group_pending_requests.group_id AND group_membership_changes.member_id=group_pending_requests.member_id ORDER BY group_membership_changes.at DESC LIMIT 1) AS action, member_id, group_id from group_pending_requests where type='invitation' and (SELECT action FROM group_membership_changes where group_membership_changes.group_id=group_pending_requests.group_id AND group_membership_changes.member_id=group_pending_requests.member_id ORDER BY group_membership_changes.at DESC LIMIT 1)<>'invitation_created';
+----------+--------------------------------+--------------------+---------------------+
| count(*) | action                         | member_id          | group_id            |
+----------+--------------------------------+--------------------+---------------------+
|        1 | removed_due_to_approval_change | 879359034123878481 | 7917468119996777761 |
+----------+--------------------------------+--------------------+---------------------+
1 row in set (0.33 sec)

mysql> select * from group_membership_changes where member_id=879359034123878481 and group_id=7917468119996777761 order by at;
+---------------------+--------------------+---------------------+--------------------------------+--------------------+
| group_id            | member_id          | at                  | action                         | initiator_id       |
+---------------------+--------------------+---------------------+--------------------------------+--------------------+
| 7917468119996777761 | 879359034123878481 | 2024-04-12 11:09:44 | joined_by_code                 | 879359034123878481 |
| 7917468119996777761 | 879359034123878481 | 2024-04-23 12:31:21 | removed_due_to_approval_change | 670968966872011405 |
+---------------------+--------------------+---------------------+--------------------------------+--------------------+
2 rows in set (0.32 sec)

Also, there is a pending 'join_request' without a corresponding group membership change:

mysql> select (SELECT action FROM group_membership_changes where group_membership_changes.group_id=group_pending_requests.group_id AND group_membership_changes.member_id=group_pending_requests.member_id ORDER BY group_membership_changes.at DESC LIMIT 1) AS action, member_id, group_id from group_pending_requests where type='join_request' and (SELECT action FROM group_membership_changes where group_membership_changes.group_id=group_pending_requests.group_id AND group_membership_changes.member_id=group_pending_requests.member_id ORDER BY group_membership_changes.at DESC LIMIT 1)<>'join_request_created';
+--------------------+--------------------+--------------------+
| action             | member_id          | group_id           |
+--------------------+--------------------+--------------------+
| invitation_created | 150874283820426701 | 199217086610026392 |
+--------------------+--------------------+--------------------+
1 row in set (0.35 sec)

mysql> select * from group_membership_changes where group_id=199217086610026392 and member_id=150874283820426701 order by at;
+--------------------+--------------------+---------------------+--------------------+---------------------+
| group_id           | member_id          | at                  | action             | initiator_id        |
+--------------------+--------------------+---------------------+--------------------+---------------------+
| 199217086610026392 | 150874283820426701 | 2019-02-11 21:09:32 | invitation_created | 1909944900306129841 |
+--------------------+--------------------+---------------------+--------------------+---------------------+
1 row in set (0.29 sec)

mysql> select * from group_pending_requests where group_id=199217086610026392 and member_id=150874283820426701;
+--------------------+--------------------+--------------+---------------------+-----------------------------+--------------------------+----------------+
| group_id           | member_id          | type         | at                  | personal_info_view_approved | lock_membership_approved | watch_approved |
+--------------------+--------------------+--------------+---------------------+-----------------------------+--------------------------+----------------+
| 199217086610026392 | 150874283820426701 | join_request | 2019-02-11 21:09:32 |                           0 |                        0 |              0 |
+--------------------+--------------------+--------------+---------------------+-----------------------------+--------------------------+----------------+
1 row in set (0.34 sec)

This needs to be investigated as it affects the invitationsView and groupRequestsView services.

GeoffreyHuck commented 1 month ago

The first case have been fixed by https://github.com/France-ioi/AlgoreaBackend/pull/1098 , it shouldn't happen again. I noticed the transition system later on. Would it be possible to add database constraints to make sure group_membership_changes and group_pending_requests are consistent?

zenovich commented 1 month ago

@GeoffreyHuck, unfortunately, I don't think it's possible to add such constraints. group_membership_changes is a changes log, it's latest row for (group_id, member_id) pair is related to the row in group_pending_requests for the same pair with the same value in at. At the same time, there can be several rows in group_membership_changes with the same (group_id, member_id, at) triple. So, as it's impossible to use a SELECT statement in foreign key constraint definitions in MySQL, we cannot create such constraints.

GeoffreyHuck commented 1 month ago

Ok, then we should at least have a short documentation about this Transition system, how it works and how to use it.

And something that would prevent adding/modifying rows without using this Transition system. Because it's so easy to put the database in an inconsistent state. Maybe writing it down in the PR review process? Do you have any idea?

zenovich commented 1 month ago

For the documentation, I would start from reading this list of constants: https://pkg.go.dev/github.com/France-ioi/AlgoreaBackend/v2@v2.21.1/app/database#GroupGroupTransitionAction (probably you did that already) If it's not enough, let's decide what else we should document.

Adding a note about preventing modifications of group_pending_request/group_membership_changes in the PR review process would be really helpful. But, at the same time, we still add new rows into group_membership_changes and remove rows from group_pending_requests outside of the transition system: https://github.com/France-ioi/AlgoreaBackend/blob/master/app/api/groups/update_group.go#L342