Generate WebAPI resource paths with backtracking

Review Request #10117 — Created Aug. 13, 2018 and submitted

Information

Review Board
release-4.0.x
a508824...

Reviewers

The old method of generating fake resource paths assumed that the first
model instance that matched resource.get_queryset(...) would be
suitable in all cases, i.e., this object would have all optional
relationships that corresspond to models. For example, it assumed that
the last modified review request would have all of the following:
- A review request draft
- File attachments
- Screenshots
- Reviews
- Draft file attachments
- Draft screenshots
- Draft reviews
- Review replies
- etc.

This is problematic when we want to add a new model instance (e.g., for
the in-process DVCs work which adds new models) because all these child
objects will have to be created for the last modified model instance.

Now, we use a backtracking algorithm to iterate over path-model pairs so
that if a model does not have the requisite child objects we can
continue onto the next instance. This way, we only need to add objects
to the database that correspond to new resources, instead of duplicating
objects from the previous parent object or recreating them entirely.

Built the docs.
With this patch applied, the docs in /r/10118/ build correctly.

Description From Last Updated

Typo in the description: "fakes resource paths" -> "fake resource paths" Also in the second paragraph: "the in process DVCs …

chipx86chipx86

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

F841 local variable 'inner_exc' is assigned to but never used

reviewbotreviewbot

F841 local variable 'e' is assigned to but never used

reviewbotreviewbot

Needs to be the full namespace.

chipx86chipx86

Blank line between these.

chipx86chipx86

Why are we looping and then returning in the first iteration?

chipx86chipx86

The wording seems pretty off: "Could not path for ...". I think this is missing a word.

chipx86chipx86

Can you add an assertion error message explaining what the comment explained? That way if it's actually hit, we know …

chipx86chipx86

The not (not ..) is kinda weird. Can we rewrite to inverse the clauses, so we have: if (not resource.singleton …

chipx86chipx86

Mind adding a comment saying why we might be hitting this from the yield? It'll help with making this more …

chipx86chipx86

Same comment as above.

chipx86chipx86

bool

chipx86chipx86

bool

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Commit:
93785c0f495f0d74c47c9c98b2bc95f01246ddab
58cbabbb51769f1378bd0c806bf5a27cfcb74e25

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. Show all issues

    Typo in the description: "fakes resource paths" -> "fake resource paths"

    Also in the second paragraph: "the in process DVCs work" "the in-progress DVCS work"

    1. Still has "DVCs". Should be "DVCS".

      Also, "in-process" should be "in-progress," right?

  3. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
    Show all issues

    Needs to be the full namespace.

  4. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
     
    Show all issues

    Blank line between these.

  5. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
     
     
    Show all issues

    Why are we looping and then returning in the first iteration?

  6. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
    Show all issues

    The wording seems pretty off: "Could not path for ...". I think this is missing a word.

  7. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
    Show all issues

    Can you add an assertion error message explaining what the comment explained? That way if it's actually hit, we know why instead of having to dig into code.

  8. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    The not (not ..) is kinda weird. Can we rewrite to inverse the clauses, so we have:

    if (not resource.singleton and ...):
        q = resource.get_queryset(...)
        ...
    else:
        try:
            yield ...
        except ...
    
  9. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
     
    Show all issues

    Mind adding a comment saying why we might be hitting this from the yield? It'll help with making this more maintainable (since it is a more complicated set of logic).

    1. I apparently had a brain fart while I was writing this. This code does nothing.

  10. docs/manual/_ext/webapidocs.py (Diff revision 3)
     
     
     
    Show all issues

    Same comment as above.

  11. 
      
brennie
chipx86
  1. 
      
  2. docs/manual/_ext/webapidocs.py (Diff revision 4)
     
     
    Show all issues

    bool

  3. docs/manual/_ext/webapidocs.py (Diff revision 4)
     
     
    Show all issues

    bool

  4. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (1625f5c)