Add an extra_data field to FileAttachment.

 






 

 


  Review Request #12747
  —
  Created Jan. 13, 2023 and submitted 

 




















 

  

   Information

  





 
 

  maubin
 









 
 

  Review Board
 









 
 

  release-6.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This adds an extra_data field to our FileAttachment model and support

for modifying the field through file attachment forms and the web API.

 








  


 
 

  
Upgraded the database and ran unit tests.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     
Add an extra_data field to the FileAttachment model.


    		
    be70a777ce1bf9610ee57f96372de91185e2e08f

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
While here, since we're adding typing, we're also going to need a from __future__ import annotations import (in its own …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
I was trying to determine if it was safe to use Django's forms.JSONField, as it's meant to be used with …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This can just be review_request.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
For this test and the others, let's be more comprehensive with the data and the results. We should cover: Strings …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Since this is changing anyway, can you convert this to single quotes?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Can you add a __future__ import here as well?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
The type should ideally be Dict[str, Any].

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Add a trailing comma here.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
You'll want to make sure you have mypy and pyright set up and checking code. None can't be assigned to …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Add a trailing comma here.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Can you add a __future__ import here?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Same comments as above.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Add a trailing comma here.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Same comments as above.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Add a trailing comma here.

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 





 

  

   




david




   

   









  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Add a trailing comma here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Add a trailing comma here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Add a trailing comma here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Add a trailing comma here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/attachments/forms.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    While here, since we're adding typing, we're also going to need a from __future__ import annotations import (in its own import group at the top).
    

    Mind also moving the import os to go before the from imports? This is older code, but would be nice to get that to conform to standards while making related changes.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/attachments/forms.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I was trying to determine if it was safe to use Django's forms.JSONField, as it's meant to be used with models.JSONField. I think it is, but I don't think it's ideal. We have some transformative stuff for JSON that isn't built into Django's version.
    

    So I think we should be using djblets.db.fields.json_field.JSONFormField instead.
    

    In either case, though, I don't think you need clean_extra_data. Were you hitting a case where this was needed?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I see, I'll switch to djblets' JSONFormField. I added the clean_extra_data method because I saw it in some other forms that have an extra data field. Once I add some more tests I'll check if I actually need it and get rid of it accordingly.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/attachments/tests.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This can just be review_request.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/attachments/tests.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    For this test and the others, let's be more comprehensive with the data and the results. We should cover:
    

      

    • Strings
      • 

    • Integers
      • 

    • Booleans
      • 

    • None
      • 

    • Nested objects
      • 

    • Nested lists,
      • 

    • Date/time fields (important: Django and Djblets serialize this differently)
      • 

    

    Given that, data should be in a multi-line dict form, as should the value going into json.dumps, for here and other new tests.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Got it. I am however confused on what you mean about the Date/time fields. Could you give some examples of what the data would look like to test this?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Sure, so for Django-based JSON support (both in Django's and Djblet's), there's a Serializer class that takes in data (primitive JSON-compatible data along with some other Python types, like datetime) and converts it to JSON. datetime becomes a ISO8601 string, since JSON lacks a date/time type. But, for historical reasons (having to do with older Django behavior and limitations in some databases), we end up formatting that string differently (stripping milliseconds from the results). I don't believe Django does that anymore, which means there'd be a different result if using their JSON field stuff vs. ours.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Ok, I've updated the tests but I'm only setting extra_data to a dictionary or a JSON string of a dictionary. There are no tests where I set extra_data to a datetime object, since it doesn't make sense to have our extra_data set to anything other than a dictionary (right?). When I include a datetime object in the extra_data dictionary that I pass to the form, and then access it from the file_attachment.extra_data, I just get a datetime object and not an ISO8601 string.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      extra_data should be a dictionary in all cases, but you can fill that with keys for different data types.
      

      Since extra_data is really just a dictionary, anything you store will be retained as-is until you load back from the database. At that point, you should see a string for the datetime.
      
 
      

      5. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Since this is changing anyway, can you convert this to single quotes?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you add a __future__ import here as well?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    The type should ideally be Dict[str, Any].
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    You'll want to make sure you have mypy and pyright set up and checking code. None can't be assigned to something that isn't wrapped in Optional.
    

    Similar to above, extra_fields should be a Dict[str, Any].
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you add a __future__ import here?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Same comments as above.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Same comments as above.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 




  13. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    14. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




maubin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-6.x (22d3d50)





 


 










  

 









     

    

   

  

  

   
   Loading...