Cache the review request's draft on the instance.

Review Request #14376 — Created March 19, 2025 and updated — Latest diff uploaded

Information

Review Board
release-7.1.x

Reviewers

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.

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.

Changes between revision 2 and 3

orig
1
2
3

Commits

Summary ID Author
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 Christian Hammond
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 Christian Hammond
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
Loading...