Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return entire file object from DRF serializer #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danlamanna
Copy link
Contributor

No description provided.

@brianhelba
Copy link
Contributor

@danlamanna Can you rebase this? It will be a little easier to think about with #278 in place.

@danlamanna
Copy link
Contributor Author

@brianhelba done

@@ -29,6 +29,6 @@ def to_internal_value(self, data: Union[str, File]) -> str: # type: ignore[over

# fields.S3FileField.save_form_data is not called by DRF, so the same behavior must be
# implemented here
internal_value = file_object.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to violate the entire point of the above comment

fields.S3FileField.save_form_data is not called by DRF, so the same behavior must be implemented here

In particular, any FieldFile value will pass through the logic here https://github.com/django/django/blob/d4c5d2b52c897ccc07f04482d3f42f976a79223c/django/db/models/fields/files.py#L190-L202 . Changing this from a str to a S3PlaceholderFile will at least change how saves are handled.

In fact, I'm worried that this might be outright breaking the save process, but our tests just aren't specific enough to pick it up.

If you think this works, could we update the tests to add new assertions or cases to ensure that the intended save behavior (update the DB, don't touch the Storage) is still happending with this?

Copy link
Contributor

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

2 participants