Fix issues publishing when there are multiple groups with ACLs.

Review Request #13399 — Created Nov. 3, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

We've had a few reports of users who had problems publishing reviews or
replies, and it seems like the cause is duplicate entries being returned
by our accessible object queries used by the batch endpoint. We'd then
do a length comparison to make sure that the length of the fetched
result was the same as the length of the requested set of objects to
publish, and fail with a very unhelpful message. This is most easily
triggered by having multiple invite-only review groups assigned to a
review request.

This change fixes the problem by using Django's in_bulk() method to
aggregate the results by PK. This handles deduplication without having
to add DISTINCT to the query, which can cause performance problems in
big databases on MySQL.

A new unit test has been added which failed with the reported error
before the change, and passes after.

  • Manually tested on both SQLite and MySQL. Saw that it failed before
    the change and succeeded afterward.
  • Ran unit tests.
Summary ID
Fix issues publishing when there are multiple groups with ACLs.
We've had a few reports of users who had problems publishing reviews or replies, and it seems like the cause is duplicate entries being returned by our accessible object queries used by the batch endpoint. We'd then do a length comparison to make sure that the length of the fetched result was the same as the length of the requested set of objects to publish, and fail with a very unhelpful message. This is most easily triggered by having multiple invite-only review groups assigned to a review request. This change fixes the problem by using Django's `in_bulk()` method to aggregate the results by PK. This handles deduplication without having to add DISTINCT to the query, which can cause performance problems in big databases on MySQL. A new unit test has been added which failed with the reported error before the change, and passes after. Testing Done: - Manually tested on both SQLite and MySQL. Saw that it failed before the change and succeeded afterward. - Ran unit tests.
8c0c210a45cb041e4070fa47cf30b82c151df87b
Description From Last Updated

This shouldn't be needed. A comparison between a lazy object and an object work fine, as the lazy object will …

chipx86chipx86

Let's avoid doing any review request logic if the IDs are empty.

chipx86chipx86

The for .. should be on the next line.

chipx86chipx86

Let's avoid doing any review logic if the IDs are empty.

chipx86chipx86

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

Small performance improvement for tests: Using .add() for these will save a query (.set() will do a lookup, possible delete, …

chipx86chipx86

Sorry, these can be added in one go, making it one query: group1.users.add(doc, grumpy)

chipx86chipx86

Rather than a cast, let's assert isinstance that we have a User. That will type-narrow and would catch if for …

chipx86chipx86
chipx86
  1. Awesome! This will be good to get out there.

    There's a couple things I think are worth tackling while here, for performance, avoiding side-effects, and keeping the logic tight in the common cases of empty ID lists.

    1. Just to dive into the side effects more, the resulting query should be a no-op (Django will optimize out a call), but it's still not free. We do queries for accessible IDs, Local Site state, etc. as aprt of it, and upcoming work will increase the amount of work that's performed when calling .accessible() methods. So these have expense to them.

  2. reviewboard/reviews/views/batch.py (Diff revision 1)
     
     
     
     

    This shouldn't be needed. A comparison between a lazy object and an object work fine, as the lazy object will resolve for that comparison. In this particular case for assertQueries(), the comparisons are definitely safe, as this has been a regular part of the queries I'm dealing with in my performance work. It looks like a mismatch in the output, and will appear as different values when seeing the mismatch output, but they won't be the source of the mismatch. In fact, I may just have assertQueries() resolve that for display to deal with the discrepancy anyway.

  3. reviewboard/reviews/views/batch.py (Diff revision 1)
     
     

    Let's avoid doing any review request logic if the IDs are empty.

  4. reviewboard/reviews/views/batch.py (Diff revision 1)
     
     
     

    The for .. should be on the next line.

  5. reviewboard/reviews/views/batch.py (Diff revision 1)
     
     

    Let's avoid doing any review logic if the IDs are empty.

  6. 
      
maubin
  1. Ship It!
  2. 
      
david
Review request changed

Change Summary:

Make requested changes.

Commits:

Summary ID
Fix issues publishing when there are multiple groups with ACLs.
We've had a few reports of users who had problems publishing reviews or replies, and it seems like the cause is duplicate entries being returned by our accessible object queries used by the batch endpoint. We'd then do a length comparison to make sure that the length of the fetched result was the same as the length of the requested set of objects to publish, and fail with a very unhelpful message. This is most easily triggered by having multiple invite-only review groups assigned to a review request. This change fixes the problem by using Django's `in_bulk()` method to aggregate the results by PK. This handles deduplication without having to add DISTINCT to the query, which can cause performance problems in big databases on MySQL. A new unit test has been added which failed with the reported error before the change, and passes after. Testing Done: - Manually tested on both SQLite and MySQL. Saw that it failed before the change and succeeded afterward. - Ran unit tests.
6a7d9e26606546e5fc6f7040954fe871a028b67a
Fix issues publishing when there are multiple groups with ACLs.
We've had a few reports of users who had problems publishing reviews or replies, and it seems like the cause is duplicate entries being returned by our accessible object queries used by the batch endpoint. We'd then do a length comparison to make sure that the length of the fetched result was the same as the length of the requested set of objects to publish, and fail with a very unhelpful message. This is most easily triggered by having multiple invite-only review groups assigned to a review request. This change fixes the problem by using Django's `in_bulk()` method to aggregate the results by PK. This handles deduplication without having to add DISTINCT to the query, which can cause performance problems in big databases on MySQL. A new unit test has been added which failed with the reported error before the change, and passes after. Testing Done: - Manually tested on both SQLite and MySQL. Saw that it failed before the change and succeeded afterward. - Ran unit tests.
2501c1bb85da91a1de2be5e730eb102e74224bc5

Diff:

Revision 2 (+314 -178)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Awesome. Two small things and then I'm happy.

  2. Small performance improvement for tests: Using .add() for these will save a query (.set() will do a lookup, possible delete, and then an .add()).

  3. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    Rather than a cast, let's assert isinstance that we have a User. That will type-narrow and would catch if for some reason we have something else.

  4. 
      
david
chipx86
  1. 
      
  2. reviewboard/reviews/tests/test_batch_view.py (Diff revisions 3 - 4)
     
     
     

    Sorry, these can be added in one go, making it one query:

    group1.users.add(doc, grumpy)
    
  3. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (ab25430)
Loading...