Summary

Cache the review request's draft on the instance.

Review Request #14376 — Created March 19, 2025 and updated 1 day, 1 hour ago

Information

Owner

chipx86*

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description*

ReviewRequest.get_draft() fetched the draft every time it was called.

This was somewhat acceptable once, but with the introduction of actions

and some fields, we ended up calling this a lot, leading to an excess

of database queries on the review request pages. This also meant that

all the callers got separate instances, which could in theory cause some

problems with state synchronization and data loss during mutation.
We now cache this on the ReviewRequest object, so that repeated

calls will return the same draft instance. We clear this when calling

ReviewRequest.clear_local_caches(), which includes when the draft is

deleted or published.
The codebase was audited to make sure this wouldn't result in any

obvious problems, though it does mean that callers that may be working

with multiple instances of the same review request (such as API unit

tests) must be careful to clear caches if re-querying for a draft.

Testing Done

All unit tests pass.

Spent some time running with this patch locally, testing the typical
functionality of posting changes, updating changes, publishing, deleting.
Never hit any issues. It will need more production testing before
release.

Commits

Summary	ID
Cache the review request's draft on the instance. `ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.	6e538a18c7cf6af0918224b285ebea775cd7cdda

Summary

Cache the review request's draft on the instance.

`ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.

6e538a18c7cf6af0918224b285ebea775cd7cdda

Issues

Description	From	Last Updated
'django.db.models.QuerySet' imported but unused Column: 1 Error code: F401	reviewbot	2 days, 11 hours ago
redefinition of unused 'User' from line 9 Column: 5 Error code: F811	reviewbot	2 days, 11 hours ago
There are no open issues

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_signal_handlers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'django.db.models.QuerySet' imported but unused

Column: 1
Error code: F401
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
redefinition of unused 'User' from line 9

Column: 5
Error code: F811
```

Change Summary:

Removed unused and duplicate imports.

Commits:

	Summary	ID
	Cache the review request's draft on the instance. `ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.	044e0dbe58217f971a494ae49e94b792bcbfc102
	Cache the review request's draft on the instance. `ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.	b21fd72b4639e44507dfd9924c8c9c26eb09860f

Diff:

Revision 2 (+1376 -184)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_page_data.py
	reviewboard/reviews/tests/test_signal_handlers.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/draft_diffcommit.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Change Summary:

Fixed query assertions. These were implemented when I first began working on this,
but not finished. The fact that they didn't work was masqueraded by a bug in Djblets
with assertQueries.

New tests do a better job of clearing caches before testing, to ensure we fetch drafts
only when necessary and only once.

Commits:

	Summary	ID
	Cache the review request's draft on the instance. `ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.	b21fd72b4639e44507dfd9924c8c9c26eb09860f
	Cache the review request's draft on the instance. `ReviewRequest.get_draft()` fetched the draft every time it was called. This was somewhat acceptable once, but with the introduction of actions and some fields, we ended up calling this a lot, leading to an excess of database queries on the review request pages. This also meant that all the callers got separate instances, which could in theory cause some problems with state synchronization and data loss during mutation. We now cache this on the `ReviewRequest` object, so that repeated calls will return the same draft instance. We clear this when calling `ReviewRequest.clear_local_caches()`, which includes when the draft is deleted or published. The codebase was audited to make sure this wouldn't result in any obvious problems, though it does mean that callers that may be working with multiple instances of the same review request (such as API unit tests) must be careful to clear caches if re-querying for a draft.	6e538a18c7cf6af0918224b285ebea775cd7cdda

Diff:

Revision 3 (+1796 -608)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_detail_view.py
	reviewboard/reviews/tests/test_review_request_page_data.py
	reviewboard/reviews/tests/test_signal_handlers.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/draft_diffcommit.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```