Fish Trophy

  
chipx86 got a fish trophy!

  
  Fish Trophy

 






  





 

 

  

  

 




  







 




  


 
 

  
Add generic view mixins for ETags and finer-grained dispatching.

 






 

 


  Review Request #8998
  —
  Created June 5, 2017 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Djblets
 









 
 

  release-0.10.x
 









 
 

  
 









 
 

  
 









 
 

  6df4df5...
 






 




 

  

   Reviewers

  





 
 

  djblets
 









 
 

  
 






 








 





  


 
 

  
There are two new mixins added for generic views: One for working with

ETags, and one for more complex views that need to alter dispatch logic.


ETagViewMixin is a mixin that allows for computing an ETag for a generic

view, checking if the client already has a copy of the content based on

that ETag, and setting appropriate headers in the response. This supports

HTTP GET and HTTP HEAD (but currently no other methods).


PrePostDispatchViewMixin is useful for views that want to run logic

in the dispatch phase but also make use of mixin that add decorators to

dispatch(). Since the view's dispatch() would run before the ones

decoratored in the mixins, there wasn't a good way to override dispatch

behavior (calculating state, checking access permissions, or augmenting

responses).


This mixin solves this by splitting the dispatch() method into a

pre_dispatch() and post_dispatch(). When placed after mixins adding

decorators and before mixins altering dispatch() logic, views will be

able to get custom logic into the dispatch phase.


pre_dispatch() can calculate state and optionally return a response to

send directly to the client (useful for access checks).


post_dispatch() can take a response from the HTTP handler and alter

it. It can even return a custom response, if needed.

 








  


 
 

  
Unit tests pass.


Tested manually with some new views I'm writing.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Module docstring?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Module docstring?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
encode_etag should never return None (since it's doing a sha-1). More likely we should check the truthiness of the result …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Module docstring?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
F401 'django.http.HttpRequest' imported but unused

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Module docstring?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
F401 'django.http.HttpRequest' imported but unused

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Why not just use a spy here to ensure the functions are called?

      		
      
       brenniebrennie
      
      
       
      
     




     

      
       
       
Missing module-level docstring.

      		
      
       brenniebrennie
      
      
       
      
     




     

      
       
       
Missing module-level docstring.

      		
      
       brenniebrennie
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/views/generic/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Module docstring?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/views/generic/etag.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Module docstring?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/views/generic/etag.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    encode_etag should never return None (since it's doing a sha-1). More likely we should check the truthiness of the result of self.get_etag_data() before encoding it.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yeah, you're right. I originally left it up to the subclass to do encode_etag, which is where this comes from.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Module docstring?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Unit tests don't need them. They'll never be in the docs.
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Module docstring?
    
  
    
 
    

 
    
  



 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 





 

  

   




brennie




   

   







  

 



 

  


   

   

    
 
brennie


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Why not just use a spy here to ensure the functions are called?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It's not actaully as easy to do as it seems.
      

      When you call a view instance (the one returned from as_view()), you're actually getting a decorator that creates a new instance of the view that handles the request. This allows the class to manipulate state on the instance without risking trampling over other threads. Because of this, we don't have an instance we can spy on. We'd have to override __init__, add the spies in there, and set a variable in the test that points to that instance (storing it in a list or dictionary or something, for scoping reasons). While that might be ideal for more complex tests, it's not really worth it in this case.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Interesting. Wouldn't we still be able to spy on the class itself instead of the instance?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      No, we have to spy on an instance, otherwise we get an error due to bound vs. unbound methods. With JavaScript we can, because it's all just plain functions without distinctions, going up the prototype chain, but kgb/Python doesn't work the same way.
      
 
      

      4. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




brennie




   

   









  

 



 

  


   

   

    
 
brennie


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing module-level docstring.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Not really necessary for unit test files. They'll never be in the docs, and their purpose is more clear.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      It's true that they're not necessary for generating docs, but it's still good form to have them for every python file, and when we eventually get close enough to full doc coverage to turn on reviewbot checkers for it, we'll want them.
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing module-level docstring.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-0.10.x (127d0ce)





 


 










  

 









     

    

   

  

  

   
   Loading...