Summary

Fix issues publishing when there are multiple groups with ACLs.

Review Request #13399 — Created Nov. 3, 2023 and submitted Nov. 3, 2023, 9:22 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

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.	8c0c210a45cb041e4070fa47cf30b82c151df87b

Summary

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

Issues

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 …	chipx86	Nov. 3, 2023, 2:41 p.m.
Let's avoid doing any review request logic if the IDs are empty.	chipx86	Nov. 3, 2023, 2:43 p.m.
The for .. should be on the next line.	chipx86	Nov. 3, 2023, 2:43 p.m.
Let's avoid doing any review logic if the IDs are empty.	chipx86	Nov. 3, 2023, 2:44 p.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	Nov. 3, 2023, 3:01 p.m.
Small performance improvement for tests: Using .add() for these will save a query (.set() will do a lookup, possible delete, …	chipx86	Nov. 3, 2023, 3:57 p.m.
Sorry, these can be added in one go, making it one query: group1.users.add(doc, grumpy)	chipx86	Nov. 3, 2023, 7:44 p.m.
Rather than a cast, let's assert isinstance that we have a User. That will type-narrow and would catch if for …	chipx86	Nov. 3, 2023, 3:57 p.m.

flake8 passed.

JSHint passed.

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.

chipx86 Nov. 3, 2023, 2:31 p.m.

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.

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

The issue has been resolved. Show all issues

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.

reviewboard/reviews/views/batch.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's avoid doing any review request logic if the IDs are empty.
```
reviewboard/reviews/views/batch.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The for .. should be on the next line.
```
reviewboard/reviews/views/batch.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's avoid doing any review logic if the IDs are empty.
```

Ship it!

```
Ship It!
```

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

	reviewboard/reviews/tests/test_batch_view.py
	reviewboard/reviews/views/batch.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/views/batch.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```

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.	2501c1bb85da91a1de2be5e730eb102e74224bc5
	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.	c852304c3bdf99cb1ed33a96a521187cbdeab754

Diff:

Revision 3 (+314 -178)

Show changes

	reviewboard/reviews/tests/test_batch_view.py
	reviewboard/reviews/views/batch.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Awesome. Two small things and then I'm happy.

reviewboard/reviews/tests/test_batch_view.py (Diff revision 3)

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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.

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.	c852304c3bdf99cb1ed33a96a521187cbdeab754
	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

Diff:

Revision 4 (+318 -176)

Show changes

	reviewboard/reviews/tests/test_batch_view.py
	reviewboard/reviews/views/batch.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/reviews/tests/test_batch_view.py (Diff revisions 3 - 4)

The issue has been resolved. Show all issues

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

group1.users.add(doc, grumpy)

Status:: Completed
Change Summary:: Pushed to release-6.x (ab25430)