Optimize and improve data safety during the creation of drafts.

Review Request #10357 — Created Dec. 11, 2018 and submitted — Latest diff uploaded

Information

Review Board
release-3.0.x
07b7a83...

Reviewers

The act of creating drafts could result in quite a number of SQL
queries. There's the initial get-or-create of the draft itself, followed
by an association of a ChangeDescription, the saving of extra_data
(as we previously could not provide the value to copy during the
get-or-create), and then a clear-and-populate of each related object
(dependencies, file attachments, screenshots, target groups/people). One
call site added onto this, replacing the ChangeDescription with yet
another one.

The expense and order of operations could result in conflicts if two
clients attempted to perform API calls that created a draft at the same
time. Both would perform a get-or-create, with one of them winning and
the other getting the winner's draft, as we'd hope for, but then the act
of re-saving for extra_data could result in data loss from the other
operation's modifications. This was unlikely to happen much in practice,
but we recently saw evidence that it's possible.

This change completely redoes draft creation. We now provide the
extra_data to copy as part of the get-or-create, preventing data loss
here.

We then handle the ChangeDescription, creating one if necessary, or
using one provided by a caller if they want (reducing a save during the
reopening of review requests). If we're replacing one, we make sure to
delete the old one.

We then move on to the Many-to-Many fields (dependencies, reviewers,
file attachments, etc.). Rather than performing an assignment to those
fields (which triggers a clear and populate), we now use add, which
will just populate. Since we know we're working with a new draft, we
know we don't have to clear anything. On top of this, we reduce the
expense of the add operation, manually looking up IDs directly from the
"through" table and adding those. This prevents the need to JOIN with
the through table in order to grab those IDs. This complicates the logic
just a bit, but means a lot less data needs to be computed on the
database and returned to the client, helping for large databases.

All unit tests pass.

Manually tested various draft-related operations. Updating fields,
file attachments, dependencies. Didn't hit any issues.

    Loading...