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

PUT object with Content-Length header. #73

Merged
merged 7 commits into from
Jan 16, 2017

Conversation

valerytschopp
Copy link
Contributor

This is required by some Swift object storage implementation, like Ceph RADOS Gateway

@coveralls
Copy link

coveralls commented Jan 12, 2017

Coverage Status

Coverage increased (+0.03%) to 92.105% when pulling aa2f026 on valerytschopp:put_object_content_length into 169d41a on blacktorn:master.

@einarf
Copy link
Contributor

einarf commented Jan 13, 2017

👍 for tests.

Looks perfectly fine to me, but I don't know too much about what content can possibly be. Will it always be something that can be considered an fd?

@dennisv
Copy link
Owner

dennisv commented Jan 13, 2017

Looking at the source of swiftclient it can be other things as well:
https://github.com/openstack/python-swiftclient/blob/70c90b2243c2df9857a188cbd61d340b7e191d0d/swiftclient/client.py#L1228-L1230

    :param contents: a string, a file-like object or an iterable
                     to read object data from;
                     if None, a zero-byte put will be done

@einarf
Copy link
Contributor

einarf commented Jan 13, 2017

https://docs.djangoproject.com/en/1.9/_modules/django/core/files/storage/
"The content should be a proper File object or any python file-like object"

It seems like the Django File class is used that does a lot of magic.

@Blacktorn What matters is probably more what the the Django Storage supports and not Swift in itself.

Maybe we should use File or ContentFile in tests from django.core.files passing those into save. If that passes I think we're fine. As long as people can stream upload without actually dealing with a file on a storage device we are good (I'm assuming the django wrappers handle this and implements an interface for streams and bytes to acts as an file)

@@ -262,13 +262,15 @@ def _save(self, name, content, headers=None):
content.seek(0)
else:
content_type = mimetypes.guess_type(name)[0]
content_length = os.fstat(content.fileno()).st_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we always get a Django File object or one if their subclasses we should probably use File.size to get the byte size of the object.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the Django docs, that would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

A quick test:

>>> from django.core.files.base import ContentFile
>>> f1 = ContentFile("esta sentencia está en español")
>>> import os
>>> os.fstat(f1.fileno())
Traceback (most recent call last):
  File "<console>", line 1, in <module>
io.UnsupportedOperation: fileno
>>> f1.size
30

@coveralls
Copy link

coveralls commented Jan 14, 2017

Coverage Status

Coverage increased (+0.03%) to 92.105% when pulling bdaf962 on valerytschopp:put_object_content_length into 169d41a on blacktorn:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.105% when pulling bdaf962 on valerytschopp:put_object_content_length into 169d41a on blacktorn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.105% when pulling bdaf962 on valerytschopp:put_object_content_length into 169d41a on blacktorn:master.

@valerytschopp
Copy link
Contributor Author

Pull request updated assuming we always get a Django File object (or a subclass) and use File.size to get the byte size of the object.

@einarf
Copy link
Contributor

einarf commented Jan 14, 2017

Perfect. I think we probably should expand test_save with different variants also calling the proper save method instead of _save, but that is probably a bit outside the scope of this pull request.

Looks ready for merge 👍

@dennisv dennisv merged commit b1ecfc8 into dennisv:master Jan 16, 2017
@dennisv
Copy link
Owner

dennisv commented Jan 16, 2017

Thanks @valerytschopp !

@valerytschopp valerytschopp deleted the put_object_content_length branch January 19, 2017 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants