Allow Content-Type to be specified and saved as S3 metadata #207
Conversation
tests/test_views.py
Outdated
| # Test default Content-Disposition when none is specified | ||
| assert actual_content_disposition == content_disposition | ||
| else: | ||
| assert actual_content_disposition == 'attachment; filename="test.txt"' |
There was a problem hiding this comment.
This test case is getting very complex, which could make it expensive to maintain. Is there a sane way to break it up a bit?
( @brianhelba may have something to say about this )
There was a problem hiding this comment.
Copying the test for each use case would just double the complexity, so I've been avoiding it.
The only solution I can think of would be a test helper that does most of the upload process on it's own, which I consider something of an antipattern. I'm open to suggestions though, this is a very bloated test.
There was a problem hiding this comment.
The body itself is big and branchy, but also just the cardinality of the cartesian product of all these parametrize decorators is getting big. That's fine if we really want to test that whole space, but if not, it'd be good to identify the most important cases and maybe collapse the decorators some.
There was a problem hiding this comment.
I agree, I think we should rework this whole test. I'm ok with merging the PR with this, as long as we refactor things at some point.
There was a problem hiding this comment.
I'm not 100% sure but it looks like maybe @dchiquito already went through and removed some of the complexity here. It looks less scary to me now, at least in terms of this changeset.
s3_file_field/_multipart_boto3.py
Outdated
| resp = self._client.create_multipart_upload( | ||
| Bucket=self._bucket_name, | ||
| Key=object_key, | ||
| **boto3_kwargs, # type: ignore |
There was a problem hiding this comment.
What's the MyPy error here? If it can't be fixed, could you use a more specific ignore?
There was a problem hiding this comment.
I have an idea for how to fix this. I'll try locally.
| object_key: str, | ||
| content_type: Union[str, None] = None, | ||
| ) -> str: | ||
| boto3_kwargs = {} |
There was a problem hiding this comment.
Since we know the file name at this point, can we set ContentDisposition too?
There was a problem hiding this comment.
We could, but AWS already provides a sensible default value when it is not set. I don't think at this point we would be able to make a better guess than that.
| object_key: str, | ||
| content_type: Union[str, None] = None, | ||
| ) -> str: | ||
| metadata = {} |
There was a problem hiding this comment.
Since we know the file name at this point, can we set Content-Disposition too?
For some reason the default Content-Type for objects uploaded during CI testing is `binary/octet-stream`, instead of the standard `application/octet-stream`. It doesn't make sense for the upload test to be verifying the default Content-Type returned when no Content-Type is specified, as that is an implementation detail of the object store.
brianhelba
changed the title
brianhelba
changed the title
No description provided.