Fix circular references in all API resources.

 






 

 


  Review Request #7654
  —
  Created Sept. 21, 2015 and submitted 

 


















 

  

   Information

  





 
 

  david
 









 
 

  Djblets
 









 
 

  release-0.9.x
 









 
 

  
 









 
 

  
 









 
 

  1e341a6...
 






 




 

  

   Reviewers

  





 
 

  djblets
 









 
 

  
 






 








 





  


 
 

  
Once upon a time, I had fixed circular references in the API serialization.

This regressed with the introduction of field limiting.


The core problem was that for foreign key and queryset types, we'd just

serialize that field as the list of objects, rather than recursing and asking

each of those to serialize themselves using the current state. Since the list

of expanded fields will always be the same when the encoder tried to serialize

using the global state, we'd hit the circular reference.


By asking each of those objects to serialize themselves with the limited list

of expanded fields, we'll only recurse once for each type.

 








  


 
 

  
    

  • Set up two review requests which depended on each other. Saw that I could
    
  make changes to their drafts.
    • 

  • Added a new unit test and fixed existing unit tests to test the correct
    
  behavior.
    • 


 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
QuerySets are always going to return the same object, so we should pre-fetch the right serializer before processing the list …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
list comprehension redefines 'obj' from line 724

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
The formatting is kinda weird here. Can we wrap some arguments to the next line instead? Actually, since we're repeating …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Col: 80 E501 line too long (84 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Col: 80 E501 line too long (80 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Col: 80 E501 line too long (84 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Col: 80 E501 line too long (80 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   




reviewbot




   

   







  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/base.py
    djblets/webapi/tests/test_webapiresource.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/base.py
    djblets/webapi/tests/test_webapiresource.py


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/resources/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
     list comprehension redefines 'obj' from line 724

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Col: 80
 E501 line too long (84 > 79 characters)

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Col: 80
 E501 line too long (80 > 79 characters)

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I'm missing something about how this solves the problem. Aren't we sharing the same request and other state, containing all the expand information? How does it not continue to expand indefinitely?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      So my initial fix for this can be seen on lines 788-795 in r1 of the diff: we temporarily remove the field name from the expansion list and then recurse.
      

      Unfortunately, the field limiting change altered it so instead of recursing and serializing those from here, we'd just "serialize" it to a list of models. These would then get actually serialized by the top-level JSON encoder, which always had the original expand list.
      
 
      

      2. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/resources/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    QuerySets are always going to return the same object, so we should pre-fetch the right serializer before processing the list (probably based on the first item, if not an empty list). That'll help make this less expensive.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/resources/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    The formatting is kinda weird here. Can we wrap some arguments to the next line instead?
    

    Actually, since we're repeating the same queryset serialization logic as before, let's move this into a _serialize_queryset utility function.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'll fix up the formatting, but the "serialization logic" that's common is just a trivial list comprehension and really doesn't make sense to extract into a method.
      
 
      

      2. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




reviewbot




   

   







  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/base.py
    djblets/webapi/tests/test_webapiresource.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/base.py
    djblets/webapi/tests/test_webapiresource.py


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Col: 80
 E501 line too long (84 > 79 characters)

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Col: 80
 E501 line too long (80 > 79 characters)

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-0.8.x (6fc80eb)





 


 










  

 









     

    

   

  

  

   
   Loading...