Require validation for DiffCommits when posting

Review Request #9920 — Created May 9, 2018 and submitted

brennie
Review Board
release-4.0.x
10119, 9905
9921
1e5bd7d...
reviewboard

We now require that diffs for an entire series of commits be validated
before posting to the DraftDiffCommitResource. Validation is performed
by making a series of POST requests to the
ValidateDiffCommitResource, each of which returns validation data to
be included in the next request. The validation data from each request
to the validation resource must then be used to POST to the
DraftDiffCommitResource.

This ensures that we do not upload broken diffs and also allows us to
upload more complex commit series. Previously, only linear commit series
where each commit modified different files were capable of being
uploaded. Now all linear commit series can be uploaded.

Posted /r/9609/ through to this commit to a single review request.
Ran unit tests.

  • 0
  • 0
  • 73
  • 2
  • 75
Description From Last Updated
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    This can wrap.

  3. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Extra period on this line.

  4. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    This can be wrapped.

  5. 
      
brennie
chipx86
  1. Apparently I never published this review, which I made a while back. My apologies if comments are pretty stale now. I'll do another as I get through the chain back to this.

  2. Can you rename the file to "commit_utils.py"?

  3. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    This is way too generic a name. I'd prefer get_file_exists_for_validation.

    Not sure this file is the right place, actually. This is about validation and not commits themselves.

    1. Except this isnt only used for validation. This is used for reviewboard/diffviewer/managers.py as well when we create the actual commits.

  4. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    You can now split this across lines at a .

  5. reviewboard/diffviewer/filediff_creator.py (Diff revision 4)
     
     
     
     
     
     
     

    Did you mean to unindent the line? Doesn't seem like it would particularly matter in this change.

    1. Yes, I did. In the old behaviour, we only appended to filediffs when we weren't validating and therefore would return an empty list. This makes it so we append to the list so we can instantiate them during validation without creating them in teh db.

  6. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Can you import specifically waht you need from the module, instead of the module itself?

    1. So we are using get_file_exists from that module (and re: my other comment, I don't believe renaming it to what you suggested is suitable), but the code in this module does:

              get_file_exists = partial(commit_utils.get_file_exists,
                                        validation_info or {},
                                        self.repository,
                                        self.cleaned_data['parent_id'])
      
              # ...
      
              return create_filediffs(
                  # ...
                  get_file_exists=get_file_exists,
                  # ...
              )
      

      which would override the global name.

      I personally don't see a big deal in importing the module vs importing bits of it in this case.

    2. It could be _get_file_exists in this case. It's not a big deal, but it's worth noting that we used to import this way from diffutils all over the place and it made it a bit more annoying to maintain. We eventually did a cleanup. I feel like as commit_utils grows, we're likely going to do the same thing.

  7. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     

    I imagine this should be a HiddenInput? We don't want users mucking about in it if we display this anywhere.

  8. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Validation metadata"

  9. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     

    The text should specifically be self.fields['validation_info'].error_messages['required']. No need for ugettext.

  10. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Shouldn't include literals in summaries.

  11. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Too many periods.

  12. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "exceptions"

  13. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Maybe better as:

    if not validation_info:
        return {}
    
    # .. rest of the method
    
  14. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     

    b64decode can also raise TypeError.

  15. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     

    The return can be inline.

  16. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Maybe "A form for validating DiffCommits."

  17. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Parent diff"

  18. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Validation metadata"

  19. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Base commit ID"

  20. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    No literals in summaries.

  21. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Too many periods.

  22. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same comments as above.

    Seems we have some stuff we could easily pull out into a base class?

  23. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     

    This seems like code we have elsewhere. I'd like to deduplicate this if possible.

  24. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    We should pull out the specific functions we need.

    1. Again, see previous reply.

  25. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    This can be broken into multiple lines now at any .

  26. No literals in summaries.

  27. reviewboard/diffviewer/tests/test_forms.py (Diff revision 4)
     
     
     
     

    Mixing import groups here.

  28. reviewboard/testing/testcase.py (Diff revision 4)
     
     

    Shouldn't DiffCommit be handling this?

  29. Ordering issues.

  30. The DIFF_* should be in alphabetical order.

  31. 
      
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 10)
     
     

    Can we include the error details in this? That'd help with debugging.

  3. reviewboard/diffviewer/forms.py (Diff revision 10)
     
     

    Trailing comma.

  4. reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 10)
     
     
     
     
     
     
     

    Can we use keyword arguments for all calls to this, here and everywhere else in the change? It's a complex enough function where we should ensure calls are self-documenting.

    1. We can't do it but can't use kwargs in partial() because otherwise they will not be used in the correct positions.

  5. reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
     
     
     
     
     

    This will create a tuple of strings.

  6. reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
     
     
     
     
     
     

    This creates a tuple.

  7. reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
     
     
     
     
     
     
     

    This too.

  8. Can you use references throughout the docstrings whenever you're referencing another resource? They're in the form of webapi2.0-<blah>-resource or webapi2.0-<blah>-list-resource.

  9. It doesn't really return "OK." I feel that can be misleading. We probably can remove the whole "either OK or an error" and just say "a result representing whether ..."

    1. I mean, it does return 200 OK.

  10. Double backticks for validation_info.

    Also, "field" and not "parameter."

  11. Can you switch to a named logger for the module?

  12. Missing trailing comma.

  13. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
Review request changed

Change Summary:

  • Do not include parent diff into in validation_info
  • Allow parent_diff in subsequent commit validation

Commit:

-46b58240dddcd476a501b02e14637bfcf0ef8625
+25c580136748405934fa83a92cd8a2ad5763f26b

Diff:

Revision 15 (+1905 -29)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Change Summary:

Clean up unit tests due to validation changes. No longer process parent filediffs in validator

Commit:

-2c175870b6f9daf20059e7c2ce98d2e601ac6b78
+4ca0eb376ddd68f25e24767e89afa03a40dfd336

Diff:

Revision 17 (+1895 -29)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 18)
     
     
     

    Newlines in exceptions are generally not going to be handled well, either in HTML or in logs. Maybe:

    Could not parse validation info %(validation_info)s: %(exc)s
    
  3. reviewboard/diffviewer/forms.py (Diff revision 18)
     
     

    Missing trailing comma.

  4. reviewboard/diffviewer/forms.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    You can consolidate these.

  5. reviewboard/diffviewer/managers.py (Diff revision 18)
     
     
     

    Missing a trailing period.

  6. Name should be GetFileExistsInHistoryTests.

  7. For here and any others, can we use called_with() and check the arguments?

  8. reviewboard/diffviewer/tests/test_forms.py (Diff revision 18)
     
     
     
     

    Extra blank line here.

  9. Missing a period.

  10. reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 18)
     
     
     
     

    Extra blank line.

  11. reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 18)
     
     
     
     
     
     

    You can align these starting on the initial call. There should be plenty of room.

  12. Can you include the repository ID? That'll help with RBCommons.

  13. This line's too long. You can break at a .

  14. Can you explain why here?

  15. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Small issues.

  2. This sounds confusing. Can you say:

    The "%s" repository does not support review requests with commit history.
    
  3. Blank line between these.

  4. 
      
brennie
brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (36b5970)
Loading...