• 
  
    
 
    
 
    

    



    
    
    
    
     
    




    
 
    

  
    



    
 

 

  

  

 

    


  
    





    
 
    



  

    
 
 
    
  
    Add original file and patched file resources to the Web API
    
 
    

    



 
    
 
    

  Review Request #3008
  —
  Created March 25, 2012 and submitted 

 
    


    




    




    




 
    
  
    
   Information

  
    



    
 
 
    
  smacleod
 
    

    





    
 
 
    
  Review Board
 
    

    





    
 
 
    
  
 
    

    





    
 
 
    
  
 
    

    





    
 
 
    
  
 
    

    



 
    



 
    
  
    
   Reviewers

  
    



    
 
 
    
  reviewboard
 
    

    





    
 
 
    
  
 
    

    



 
    



    


    
 
    




  

    
 
 
    
  
    Adds resources for retrieving the original file, and the patched file from the Web API.

These new resources are children of the File Diff Resource, and provide a way for external tools to retrieve full copies of source files from a review request.

If the file represented by the FileDiffResource is new, the link to the OriginalFileResource will be removed. Alternatively, if the file is deleted, the link to the PatchedFileResource will be removed.
    
 
    

    





  

    
 
 
    
  
    Tested manually on local dev. Ran unit tests.
    
 
    

    




 
    

    





    
 
    









 





    
 
 
 
    
  
    
   
    
    
    
     
    
    
    
      
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
    
    
   
    
   
    
    
     
    
      Description
      From
      Last Updated
     
    
        
    



     
    
      
       
       
    Always have the imports listed alphabetically.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    I know the others aren't like this, but "is_new" would feel better to me.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    You can remove the blank line inbetween these.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Combine these. The summary is always right next to the """
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Why not combine these?
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Put the # TODO on the previous line.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Multi-line lists should have each entry on their own line, indented 4 spaces.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Nope.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Summary line.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Not really true. We're only removing links if we're operating on an object. The resource itself may still be a …
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Just: "if obj:"
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    I believe the only time that we will have an exception is when something goes very wrong. This means it's …
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    In this particular case, we should catch/log per-function, since they're both pretty heavy-weight and we'll end up wanting more detail.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    Probably not necessary. In the context of this function, obj basically means the filediff. Let's just use obj directly.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    

        
   
    
  
    
 
    

    




 
    

    



  
    
 
    

    


    


 













    
 





 
    
  
    
   
    



chipx86




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
chipx86


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      My comments on the first resource apply to the second as well.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 

















    2. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/diffviewer/models.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Always have the imports listed alphabetically.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      3. 






    3. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/diffviewer/models.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      I know the others aren't like this, but "is_new" would feel better to me.
      
  
      
 
      

 
      
  


      
 
      
 
        

  


      1. 
 
 
        
 
        
  
 
        
 
        
  
        I agree. I was a little hesitant naming it new, but decided to follow the previous convention.
        
 
        

        2. 


 
      


      

 
      

      4. 






    4. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      You can remove the blank line inbetween these.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      5. 






    5. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Combine these. The summary is always right next to the """
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      6. 






    6. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Why not combine these?
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      7. 






    7. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Put the # TODO on the previous line.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      8. 






    8. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Multi-line lists should have each entry on their own line, indented 4 spaces.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      9. 






    9. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Nope.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      10. 






    10. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Summary line.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      11. 






    11. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Not really true. We're only removing links if we're operating on an object. The resource itself may still be a list.
      
  
      
 
      

 
      
  


      
 
      
 
        

  


      1. 
 
 
        
 
        
  
 
        
 
        
  
        I'm not sure I understand how the resource may be a list. "If obj" => "obj is a filediff", which is not a list? If this assumption I'm making is incorrect, then isn't there possibility for breakage with:

filediff = obj

if filediff.new:
        
 
        

        2. 



      2. 
 
 
        
 
        
  
 
        
 
        
  
        A list can serialize each object in it in an "expanded" form. obj would be that entry in the list, rather than the object pertaining to that exact resource URL.

So, for a list of 20 items, get_links may be called 20 times, if the entries are expanded.
        
 
        

        3. 



      3. 
 
 
        
 
        
  
 
        
 
        
  
        That makes sense, I'll fix the comment so It reflects what's actually going on.
        
 
        

        4. 


 
      


      

 
      

      12. 






    12. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Just: "if obj:"
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      13. 




    13. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      14. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    




 SM





   
    
   
    
  
    
 
    


 

    












    
 





 
    
  
    
   
    



chipx86




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
chipx86


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 

















    2. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      I believe the only time that we will have an exception is when something goes very wrong. This means it's a problem in Review Board (or a server we talk to) and not a non-existent resource. So, we should return a 500 Internal Server Error instead. We should also log the error we get.

Given that, the except: should be "expect Exception, e:"

Same below.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      3. 






    3. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

     
       
      

     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      In this particular case, we should catch/log per-function, since they're both pretty heavy-weight and we'll end up wanting more detail.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      4. 






    4. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Probably not necessary. In the context of this function, obj basically means the filediff. Let's just use obj directly.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      5. 




    5. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      6. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    




 SM





   
    
   
    
  
    
 
    


 

    












    
 





 
    
  
    
   
    



chipx86




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
chipx86


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      Ship It!
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 















    2. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      3. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    




 SM





   
    
   
    
  
    
 
    


 
    
  
    

   
    
   
    
    
     Review request changed
    
    
   
    

  
    
  
    
  
    




    

  
    
   
    
    Status:
   
    
   
    

    Completed

   
    
  
    



  
    
   
    
    Change Summary:
   
    
   
    
    
    Pushed to master (554eaa50414255079e8ff658de05af30e9f12cb9)