Add an extra_data field to FileAttachment.

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

Information

Review Board
release-6.x

Reviewers

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
  1. 
      
  2. Show all issues

    Add a trailing comma here.

  3. Show all issues

    Add a trailing comma here.

  4. Show all issues

    Add a trailing comma here.

  5. Show all issues

    Add a trailing comma here.

  6. 
      
chipx86
  1. 
      
  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. 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.

  4. reviewboard/attachments/tests.py (Diff revision 1)
     
     
    Show all issues

    This can just be review_request.

  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. 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. 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. 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.

  6. Show all issues

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

  7. Show all issues

    Can you add a __future__ import here as well?

  8. Show all issues

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

  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. Show all issues

    Can you add a __future__ import here?

  11. Show all issues

    Same comments as above.

  12. Show all issues

    Same comments as above.

  13. 
      
maubin
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (22d3d50)