Rework the review request draft API for better performance and fixes.

Review Request #10346 — Created Dec. 3, 2018 and submitted

Information

Review Board
release-3.0.x
ca710c4...

Reviewers

The review request draft API has been around a long time, and the code
hasn't aged particularly well. It tried to be a bit clever in terms of
how it handled mutable fields, making use of a mutable keyword in
field definitions, looping through them and setting attributes as
appropriate, and specially-handling other special fields.

The approach left the API open for some bugs:

  • If the depends_on, target_groups, etc. fields are set by the
    caller, they'll be saved to the database, even if validation were to
    otherwise fail from an invalid value somewhere.
  • Unsaved field data may still end up in the 'draft' key in error
    payloads.
  • The API was also a bit slow. Every entry for depends_on, etc. are
    queried individually, leading to much more API calls than necessary.
    On top of this, all fields on a draft are saved with every request,
    opening the door to lost data if two clients are modifying two
    separate fields at the same time.

This change redoes the handling of the API to be safer and faster.

All validation now happens up-front before any changes are made, and any
resulting error payload contains the existing draft state instead of one
with unsaved changes provided.

Values triggering database lookups now results in one lookup per field
rather than one per item per field, making the call faster.

Only the fields actually changed are saved back to the database, making
it even faster still, and preventing that lost data scenario. Similarly,
relations are no longer saved until all validation has passed.

The code in general has been cleaned up quite a bit, making it easier to
understand the flow of logic.

While technically this does change some behavior (in terms of the
prematurely-saved relation values and draft content in error payloads),
that behavior was unintentional and buggy, and we're now on a good
foundation going forward.

Unit tests passed.

Tested modifying all the fields by hand.

Description From Last Updated

Can we put one arg per line now?

daviddavid

Leftover debug output

daviddavid
david
  1. 
      
  2. reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    Can we put one arg per line now?

  3. Show all issues

    Leftover debug output

  4. 
      
chipx86
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (803d0e5)