Require validation for DiffCommits when posting

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

Information

Review Board
release-4.0.x
1e5bd7d...

Reviewers

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.

Description From Last Updated

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

chipx86chipx86

F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

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 …

chipx86chipx86

This can wrap.

daviddavid

You can now split this across lines at a .

chipx86chipx86

bool

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

"Validation metadata"

chipx86chipx86

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

chipx86chipx86

Shouldn't include literals in summaries.

chipx86chipx86

Extra period on this line.

daviddavid

Too many periods.

chipx86chipx86

"exceptions"

chipx86chipx86

Maybe better as: if not validation_info: return {} # .. rest of the method

chipx86chipx86

b64decode can also raise TypeError.

chipx86chipx86

The return can be inline.

chipx86chipx86

Maybe "A form for validating DiffCommits."

chipx86chipx86

"Parent diff"

chipx86chipx86

"Validation metadata"

chipx86chipx86

"Base commit ID"

chipx86chipx86

No literals in summaries.

chipx86chipx86

Too many periods.

chipx86chipx86

Same comments as above. Seems we have some stuff we could easily pull out into a base class?

chipx86chipx86

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

chipx86chipx86

We should pull out the specific functions we need.

chipx86chipx86

This can be wrapped.

daviddavid

This can be broken into multiple lines now at any .

chipx86chipx86

No literals in summaries.

chipx86chipx86

Mixing import groups here.

chipx86chipx86

Shouldn't DiffCommit be handling this?

chipx86chipx86

Ordering issues.

chipx86chipx86

The DIFF_* should be in alphabetical order.

chipx86chipx86

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

chipx86chipx86

Trailing comma.

chipx86chipx86

Can we use keyword arguments for all calls to this, here and everywhere else in the change? It's a complex …

chipx86chipx86

This will create a tuple of strings.

chipx86chipx86

This creates a tuple.

chipx86chipx86

This too.

chipx86chipx86

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.

chipx86chipx86

It doesn't really return "OK." I feel that can be misleading. We probably can remove the whole "either OK or …

chipx86chipx86

Double backticks for validation_info. Also, "field" and not "parameter."

chipx86chipx86

Can you switch to a named logger for the module?

chipx86chipx86

Missing trailing comma.

chipx86chipx86

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

reviewbotreviewbot

F821 undefined name 'on_info'

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

F821 undefined name 'on_info'

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

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

reviewbotreviewbot

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

chipx86chipx86

Missing trailing comma.

chipx86chipx86

You can consolidate these.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Name should be GetFileExistsInHistoryTests.

chipx86chipx86

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

chipx86chipx86

Extra blank line here.

chipx86chipx86

Missing a period.

chipx86chipx86

Extra blank line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Can you explain why here?

chipx86chipx86

E501 line too long (82 > 79 characters)

reviewbotreviewbot

F821 undefined name 'revision'

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

This sounds confusing. Can you say: The "%s" repository does not support review requests with commit history.

chipx86chipx86

Blank line between these.

chipx86chipx86
brennie
Review request changed
Change Summary:

Fix unit test failures.

Commit:
c73a8395ee5a8c463733f9933cd1693ef85a84a2
26ef4844d2fe1de202a5e98cb3e484ea1f1e2bd2

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    This can wrap.

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

    Extra period on this line.

  4. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     
    Show all issues

    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. Show all issues

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

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

    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)
     
     
    Show all issues

    You can now split this across lines at a .

  5. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     
    Show all issues

    bool

  6. reviewboard/diffviewer/filediff_creator.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    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.

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

    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.

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

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

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

    "Validation metadata"

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

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

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

    Shouldn't include literals in summaries.

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

    Too many periods.

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

    "exceptions"

  14. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
    Show all issues

    Maybe better as:

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

    b64decode can also raise TypeError.

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

    The return can be inline.

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

    Maybe "A form for validating DiffCommits."

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

    "Parent diff"

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

    "Validation metadata"

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

    "Base commit ID"

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

    No literals in summaries.

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

    Too many periods.

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

    Same comments as above.

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

  24. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

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

    We should pull out the specific functions we need.

    1. Again, see previous reply.

  26. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     
    Show all issues

    This can be broken into multiple lines now at any .

  27. Show all issues

    No literals in summaries.

  28. reviewboard/diffviewer/tests/test_forms.py (Diff revision 4)
     
     
     
     
    Show all issues

    Mixing import groups here.

  29. reviewboard/testing/testcase.py (Diff revision 4)
     
     
    Show all issues

    Shouldn't DiffCommit be handling this?

  30. Show all issues

    Ordering issues.

  31. Show all issues

    The DIFF_* should be in alphabetical order.

  32. 
      
brennie
brennie
Review request changed
Change Summary:

Addressed remaining feedback

Commit:
efd916a5a0e84bc2a7904c7970ecbf576d1933e0
1ac95ff3d8d386703ac12ca8aaadc9fce9e515b3

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
Change Summary:

Keep track of add/modify/remove in parent diff

Commit:
b91eaa8d612bfcedd7a453a5e531e36964349ce0
8f0ea175b4fa6f17d0516a4bb5d62ef7a5ef968a

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

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

    Trailing comma.

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

    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)
     
     
     
     
     
    Show all issues

    This will create a tuple of strings.

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

    This creates a tuple.

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

    This too.

  8. Show all issues

    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. Show all issues

    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. Show all issues

    Double backticks for validation_info.

    Also, "field" and not "parameter."

  11. Show all issues

    Can you switch to a named logger for the module?

  12. Show all issues

    Missing trailing comma.

  13. 
      
brennie
Review request changed
Change Summary:

Addressed issues, added docs.

Commit:
255222c19ecfa14618aba5461f65a599aacac551
74eab7d1571527aba49415f4f887f065dd0900a8

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Change Summary:

actually commit changes >.>

Commit:
74eab7d1571527aba49415f4f887f065dd0900a8
55fd26fa7172fbfc4137b0c2dc9190add0a6831a

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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)
     
     
    Show all issues

    Missing trailing comma.

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

    You can consolidate these.

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

    Missing a trailing period.

  6. Show all issues

    Name should be GetFileExistsInHistoryTests.

  7. Show all issues

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

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

    Extra blank line here.

  9. Show all issues

    Missing a period.

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

    Extra blank line.

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

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

  12. Show all issues

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

  13. Show all issues

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

  14. Show all issues

    Can you explain why here?

  15. 
      
brennie
Review request changed
Commit:
858f8d77098b2ad76f1e06d3d518111923b05905
5973997c843000d9dd417f41feb5e88f12bae3eb

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Change Summary:

Fix issues introduced with git rerere

Commit:
5973997c843000d9dd417f41feb5e88f12bae3eb
c23ee9cf286fbca7de21d4b0a45fa3d68ecb0a0c

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Small issues.

  2. Show all issues

    This sounds confusing. Can you say:

    The "%s" repository does not support review requests with commit history.
    
  3. Show all issues

    Blank line between these.

  4. 
      
brennie
brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (36b5970)