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

Update S3Boto3Storage class by updating size method to read the file … #366

Conversation

mabuaisha
Copy link
Contributor

Update size method of S3Boto3Storage to read size using size and not content_lenght

@codecov-io
Copy link

codecov-io commented Jul 25, 2017

Codecov Report

Merging #366 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   75.61%   75.61%           
=======================================
  Files          11       11           
  Lines        1546     1546           
=======================================
  Hits         1169     1169           
  Misses        377      377
Impacted Files Coverage Δ
storages/backends/s3boto3.py 85.71% <0%> (ø) ⬆️
storages/__init__.py 100% <100%> (ø) ⬆️

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 e88dd98...5e250ce. Read the comment docs.

@mabuaisha
Copy link
Contributor Author

@jschneier I add a fix for getting size of file when using boto3 and I need that fix so that django-storage and boto3 can working

…size using size attr instead of content_lenght

Update S3Boto3Storage class by updating size method to read the file size using size attr instead of content_lenght
@mabuaisha mabuaisha force-pushed the fix-issue-with-s3-getting-size branch from 2e0e47e to 5e250ce Compare July 25, 2017 15:44
@jschneier
Copy link
Owner

Merged in 4501f99. Version bump will come soon, please use master until then. Thanks for the report and patch.

@jschneier jschneier closed this Jul 26, 2017
@mabuaisha
Copy link
Contributor Author

@jschneier Thanks for merging this to master

@mabuaisha
Copy link
Contributor Author

mabuaisha commented Jul 27, 2017

@jschneier It seems the commit fix did not fix the issue because the following method

    def size(self, name):
        name = self._normalize_name(self._clean_name(name))
        if self.entries:
            entry = self.entries.get(name)
            if entry:
                return entry.size
            return 0
        return self.bucket.Object(self._encode_name(name)).content_length

entry instance could be a type of the following s3.Object which need to read from content_length or it could be an instance type of s3.ObjectSummary which need to read from size

So to solve the above issue I think we need to check the instance type before reading from size

if isinstance(entry, s3.Object):
    return entry.content_length

elif isinstance(entry, s3.ObjectSummary)
      return entry.size

Please let me know what do you think ?

@jschneier
Copy link
Owner

I'm wondering if this is a bug in boto3 that might depend on your version. What version of boto3 are you using?

@jschneier
Copy link
Owner

Nevermind, it appears to be intentional. We can use a hasattr check.

jschneier added a commit that referenced this pull request Jul 27, 2017
@jschneier
Copy link
Owner

Pushed 523ab59.

@mabuaisha
Copy link
Contributor Author

@jschneier Thanks for your released to handle that :)

jschneier added a commit that referenced this pull request Aug 14, 2017
jschneier added a commit that referenced this pull request Aug 14, 2017
nitely pushed a commit to satellogic/django-storages that referenced this pull request Jul 30, 2018
nitely pushed a commit to satellogic/django-storages that referenced this pull request Jul 30, 2018
nitely pushed a commit to satellogic/django-storages that referenced this pull request Jul 30, 2018
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.

3 participants