Summary

Add an extra_data field to FileAttachment.

Review Request #12747 — Created Jan. 13, 2023 and submitted Feb. 1, 2023, 3:17 p.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This adds an extra_data field to our FileAttachment model and support
for modifying the field through file attachment forms and the web API.

Testing Done

Upgraded the database and ran unit tests.

Commits

Summary	ID
Add an extra_data field to the FileAttachment model.	be70a777ce1bf9610ee57f96372de91185e2e08f

Issues

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 …	chipx86	Jan. 26, 2023, 5:29 a.m.
I was trying to determine if it was safe to use Django's forms.JSONField, as it's meant to be used with …	chipx86	Jan. 27, 2023, 12:30 p.m.
This can just be review_request.	chipx86	Jan. 26, 2023, 5:33 a.m.
For this test and the others, let's be more comprehensive with the data and the results. We should cover: Strings …	chipx86	Jan. 30, 2023, 11:03 a.m.
Since this is changing anyway, can you convert this to single quotes?	chipx86	Jan. 26, 2023, 7:21 a.m.
Can you add a __future__ import here as well?	chipx86	Jan. 26, 2023, 7:29 a.m.
The type should ideally be Dict[str, Any].	chipx86	Jan. 26, 2023, 7:30 a.m.
Add a trailing comma here.	david	Jan. 26, 2023, 5:24 a.m.
You'll want to make sure you have mypy and pyright set up and checking code. None can't be assigned to …	chipx86	Jan. 26, 2023, 7:59 a.m.
Add a trailing comma here.	david	Jan. 26, 2023, 5:25 a.m.
Can you add a __future__ import here?	chipx86	Jan. 26, 2023, 8:08 a.m.
Same comments as above.	chipx86	Jan. 26, 2023, 8:04 a.m.
Add a trailing comma here.	david	Jan. 26, 2023, 5:25 a.m.
Same comments as above.	chipx86	Jan. 26, 2023, 8:04 a.m.
Add a trailing comma here.	david	Jan. 26, 2023, 5:25 a.m.

flake8 passed.

JSHint passed.

Ship it!

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma here.
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma here.
```
reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma here.
```
reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma here.
```

reviewboard/attachments/forms.py (Diff revision 1)

The issue has been resolved. 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.

reviewboard/attachments/forms.py (Diff revision 1)

The issue has been resolved. 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?

maubin Jan. 26, 2023, 6:55 a.m.

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.

reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This can just be review_request.
```

reviewboard/attachments/tests.py (Diff revision 1)

The issue has been resolved. 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.

maubin Jan. 26, 2023, 6:55 a.m.

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?

chipx86 Jan. 26, 2023, 9:15 p.m.

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.

maubin Jan. 27, 2023, 12:39 p.m.

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.

chipx86 Jan. 27, 2023, 3:46 p.m.

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.

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Since this is changing anyway, can you convert this to single quotes?
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add a __future__ import here as well?
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The type should ideally be Dict[str, Any].
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. 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].

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add a __future__ import here?
```
reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comments as above.
```
reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comments as above.
```

Commits:

	Summary	ID
	Add an extra_data field to the FileAttachment model.	9c8f727923e76276f50ffcc97928c28df5781870
	Add an extra_data field to the FileAttachment model.	8872e6cacaa1ebcc835f4fb341968ec5c2b33d4d

Diff:

Revision 2 (+972 -70)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_extra_data.py
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/resources/user_file_attachment.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Add an extra_data field to the FileAttachment model.	8872e6cacaa1ebcc835f4fb341968ec5c2b33d4d
	Add an extra_data field to the FileAttachment model.	be70a777ce1bf9610ee57f96372de91185e2e08f

Diff:

Revision 3 (+1122 -70)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_extra_data.py
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/resources/user_file_attachment.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (22d3d50)