• 
      

    Fix various issues in FileAttachment.objects.get_for_filediff.

    Review Request #13538 — Created Feb. 12, 2024 and submitted

    Information

    Review Board
    master

    Reviewers

    The get_for_filediff method for getting file attachments attached to
    diffs had a few major issues that were preventing it from functioning
    correctly.

    First, it was querying solely based on the repository, file path, and
    revision. This meant we could theoretically end up returning
    FileAttachment objects which were attached to a different review
    request than the given FileDiff. While our database schema
    theoretically would allow for a single FileAttachment to be attached to
    multiple review requests, in practice everything assumes that it's more
    of a foreign key relationship than a many to many relationship.

    Second, when a review request is created with commit history, there are
    actually multiple FileDiffs that all go into a single changed file. At
    the very least, we'll have a FileDiff for the commit and a FileDiff for
    the cumulative diff.

    When we're posting changes from RBTools, we attach the FileAttachment to
    the FileDiff corresponding to the commit. This meant that even though
    the file was uploaded, it wasn't visible in the diff viewer because we'd
    usually be looking at the cumulative diff.

    This change makes it so FileAttachment.objects.get_for_filediff will
    first try the simple case, and if that isn't found, it will check to see
    if the DiffSet is created with commit history. If that's the case, we'll
    look for the most recent FileDiff object corresponding to the given file
    and use that instead.

    Finally, we mostly were relying on this function to raise
    ObjectDoesNotExist, but we also had a case where it could return
    None. I've changed it to always return None when there are no
    matching file attachments.

    • Made some queries to filediff API resources and saw that the serialized
      attachment fields no longer contained links to file attachments
      connected to other review requests.
    • Posted a change with RBTools that included binary files. Saw that I
      could now see those files in the diff viewer.
    • Ran unit tests.
    Summary ID
    Fix various issues in FileAttachment.objects.get_for_filediff.
    The `get_for_filediff` method for getting file attachments attached to diffs had a few major issues that were preventing it from functioning correctly. First, it was querying solely based on the repository, file path, and revision. This meant we could theoretically end up returning `FileAttachment` objects which were attached to a different review request than the given `FileDiff`. While our database schema theoretically would allow for a single FileAttachment to be attached to multiple review requests, in practice everything assumes that it's more of a foreign key relationship than a many to many relationship. Second, when a review request is created with commit history, there are actually multiple FileDiffs that all go into a single changed file. At the very least, we'll have a FileDiff for the commit and a FileDiff for the cumulative diff. When we're posting changes from RBTools, we attach the FileAttachment to the FileDiff corresponding to the commit. This meant that even though the file was uploaded, it wasn't visible in the diff viewer because we'd usually be looking at the cumulative diff. This change makes it so `FileAttachment.objects.get_for_filediff` will first try the simple case, and if that isn't found, it will check to see if the DiffSet is created with commit history. If that's the case, we'll look for the most recent FileDiff object corresponding to the given file and use that instead. Finally, we mostly were relying on this function to raise `ObjectDoesNotExist`, but we also had a case where it could return `None`. I've changed it to always return `None` when there are no matching file attachments. Testing Done: - Made some queries to filediff API resources and saw that the serialized attachment fields no longer contained links to file attachments connected to other review requests. - Posted a change with RBTools that included binary files. Saw that I could now see those files in the diff viewer. - Ran unit tests.
    22281d659f2f85975cbf8130d486136ac20358d4
    Description From Last Updated

    Can we update unit tests for this, given that there are some notable fixes in here?

    chipx86chipx86

    line too long (80 > 79 characters) Column: 80 Error code: E501

    reviewbotreviewbot

    line too long (80 > 79 characters) Column: 80 Error code: E501

    reviewbotreviewbot

    This function is missing a final explicit return None. It only has an implicit one. In fact, if we have …

    chipx86chipx86

    Tiny tiny nit, but we should consistently access using .pk instead of .id. They're technically interchangeable, but .pk is considered …

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

    flake8

    david
    maubin
    1. Ship It!
    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      Can we update unit tests for this, given that there are some notable fixes in here?

      1. There are going to be some more changes to this file. I've got a note to add testing for this once that settles.

    3. reviewboard/attachments/managers.py (Diff revision 2)
       
       
      Show all issues

      This function is missing a final explicit return None. It only has an implicit one.

      In fact, if we have one, we can eliminate a few returns.

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

      Tiny tiny nit, but we should consistently access using .pk instead of .id. They're technically interchangeable, but .pk is considered the "true" field for the primary key.

    5. 
        
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.x (d7a7e57)