Include Content-MD5 header when body is empty

[wimglenn]

Closes #1979

[codecov-io]

Codecov Report

Merging #1985 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop   #1985      +/-   ##
==========================================
+ Coverage    92.98%     93%   +0.02%     
==========================================
  Files           60      60              
  Lines        10784   10784              
==========================================
+ Hits         10027   10030       +3     
+ Misses         757     754       -3

Impacted Files	Coverage Δ
botocore/handlers.py	96.51% <100%> (ø)	⬆️
botocore/credentials.py	98.87% <0%> (+0.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb2f126...223f3ea. Read the comment docs.

[swetashre]

@wimglenn - Can you write a test case to make sure the header is present ?

[wimglenn]

@swetashre Please check the CI failure. It does not look related to my change at all.

[wimglenn]

@swetashre Is there anything more I can do to help get this bug fixed?

[wimglenn]

@swetashre Is it possible to get a review or get this patch from February merged? What's the hold-up?

[joguSD]

@wimglenn Conceptually, these changes sound alright to me. We need to do some confirmation on our side around all the exact expectations for the content-md5 of empty bodies. We also have some changes in the works around this code path that will end up conflicting. The changes might end up fixing this issue anyways. We'll follow up on this after those changes go out.

[nateprewitt]

Hi @wimglenn, just wanted to follow up from our last comment. The changes referenced are now in place. Please feel free to rebase this on top of the develop branch and we can take another look. Thanks!

[jangrewe]

@nateprewitt Maybe you can have a look at #2181 - this fix works fine for our issue.

[wimglenn]

@nateprewitt @joguSD @swetashre Rebased on develop. We're actually still on a forked botocore, and are blocked from updating boto3 because of this issue, so it would be helpful to get the bugfix upstream..

[nateprewitt]

Thanks again for this PR, @wimglenn! I'm merging this into a feature branch for a couple more tweaks and we're going to try to get this merged into live asap.

[radekwlsk]

@wimglenn @nateprewitt FYI: In Django (and especially django-storages with boto3) when using StringIO based ContentFile (file-like object that takes raw content, string or bytes) to create S3 object this change makes it fail on save to S3 as only bytes content works properly.

For me the issue arised when using empty file (ContentFile("")) to create S3 object stub to save it's reference to database before file content is uploaded under the key.

TypeError: Unicode-objects must be encoded before hashing
  ...
  File "django/db/models/fields/files.py", line 87, in save
    self.name = self.storage.save(name, content, max_length=self.field.max_length)
  File "django/core/files/storage.py", line 52, in save
    return self._save(name, content)
  File "storages/backends/s3boto3.py", line 451, in _save
    obj.upload_fileobj(content, ExtraArgs=params)
  File "boto3/s3/inject.py", line 619, in object_upload_fileobj
    return self.meta.client.upload_fileobj(
  File "boto3/s3/inject.py", line 539, in upload_fileobj
    return future.result()
  File "s3transfer/futures.py", line 106, in result
    return self._coordinator.result()
  File "s3transfer/futures.py", line 265, in result
    raise self._exception
  File "s3transfer/tasks.py", line 126, in __call__
    return self._execute_main(kwargs)
  File "s3transfer/tasks.py", line 150, in _execute_main
    return_value = self._main(**kwargs)
  File "s3transfer/upload.py", line 692, in _main
    client.put_object(Bucket=bucket, Key=key, Body=body, **extra_args)
  File "botocore/client.py", line 357, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "botocore/client.py", line 652, in _make_api_call
    handler, event_response = self.meta.events.emit_until_response(
  File "botocore/hooks.py", line 360, in emit_until_response
    return self._emitter.emit_until_response(aliased_event_name, **kwargs)
  File "botocore/hooks.py", line 243, in emit_until_response
    responses = self._emit(event_name, kwargs, stop_on_response=True)
  File "botocore/hooks.py", line 211, in _emit
    response = handler(**kwargs)
  File "botocore/utils.py", line 2219, in conditionally_calculate_md5
    md5_digest = calculate_md5(body, **kwargs)
  File "botocore/utils.py", line 2196, in calculate_md5
    binary_md5 = _calculate_md5_from_file(body)
  File "botocore/utils.py", line 2209, in _calculate_md5_from_file
    md5.update(chunk)

I suppose making it ContentFile(b"") will resolve that issue, as with string it uses StringIO instead of BytesIO buffer. But maybe text content could be handled here? To encode it first, before calculating md5.

[wimglenn]

@radekwlsk Yes I would think that's either an error in django-storages, or bug which was lurking in user code using it, that code should use BytesIO in the first place. Better to make sure you're sending bytes and not text, it's outside of the responsibility for botocore to guess any encoding to use.

[nateprewitt]

Hi @radekwlsk, thanks for bringing this to our attention! It looks like django-storages has been violating the API constraint but happened to be getting away with it only for "falsey" files. The boto3 interface they're using states that the fileobj supplied must be in binary mode to work correctly.

@wimglenn is correct that this will likely need to be fixed in that project since guessing around file encodings leads to a lot of unexpected failures.

[radekwlsk]

Hi @radekwlsk, thanks for bringing this to our attention! It looks like django-storages has been violating the API constraint but happened to be getting away with it only for "falsey" files. The boto3 interface they're using states that the fileobj supplied must be in binary mode to work correctly.

Thanks for quick response! I've already reported that isssue in django-storages. If there is no response I may create a PR myself then.