search

theriverman / django-minio-backend

Minio Backend for Django
https://pypi.org/project/django-minio-backend/
MIT License
111 stars 22 forks source link

Bucket name is wrong when read data from minio #8

Closed rcy17 closed 3 years ago

rcy17 commented 4 years ago

environment

rcy17 commented 4 years ago

I'm not sure if I really got it, but currently I just modify djabgo_minio_backend.models like this:

    def _open(self, object_name, mode=None, bucket_name=None,  request_headers=None, sse=None):
        if bucket_name is None:
            bucket_name = self._BUCKET
        return self.client.get_object(bucket_name, object_name, request_headers, sse)

Now it works.

theriverman commented 4 years ago

Thanks for your detailed report!

I've just tested it in my local environment after bumping Django to 3.x and the underlying minio-sdk to the latest version and all seems to work. Next thing to see if it's related to the used Minio version itself (I'm locally running version 2019-09-18).

Could you provide your get_image_path implementation and/or tell me its typical return value and type, please?

Modifying _open() shouldn't be needed, because the Exception is raised in the SDK itself.

Edit: Bumped Minio to latest version, still OK!

Thanks for reporting the issue with is_minio_available, I've updated the README.

theriverman commented 4 years ago

Please reopen this issue if you're still facing this error.

laymonage commented 4 years ago

Hi there, I think this is still an issue.

I didn't encounter the issue directly when fetching the object, but when I try to access the file using instance.image.open(), I got the same error.

What was the file name that you used for the test, @theriverman? I suppose if the whole file path in the bucket does not violate the bucket naming rules, it won't be an issue. However, try uploading the file under a directory, such as mydir/myfile.png.

As @rcy17 has pointed out, your _open method signature is wrong. In order to write a custom storage system, you need to implement _open(name, mode='rb') as specified in the Django docs. With your current _open method signature, the file name gets passed as the bucket_name.

I don't see any reason why you would add bucket_name as a parameter (since there's no point trying to access the file from a different bucket), so I think the following fix is enough:

    def _open(self, object_name, request_headers=None, sse=None):
        return self.client.get_object(self._BUCKET_NAME, object_name, request_headers, sse)

    def open(self, object_name, request_headers=None, sse=None):
        return self._open(object_name, request_headers, sse)

If you don't want to override open, then you'll have to add mode=None before request_headers so that the default value of mode='rb' doesn't mistakenly get passed as request_headers (see https://github.com/django/django/blob/master/django/core/files/storage.py#L34)

theriverman commented 4 years ago

Hi @laymonage,

You're right, this was still an issue. I've read the trackback superficially copied by @rcy17 . I have modified the _open method in the following way:

    def _open(self, object_name, mode='rb', **kwargs):
        """
        Implements the Storage._open(name,mode='rb') method
        :param name (str): object_name [path to file excluding bucket name which is implied]
        :kwargs (dict): pass on to the underlying minIO client's get_object() method
        """
        resp: urllib3.response.HTTPResponse = urllib3.response.HTTPResponse()

        if mode != 'rb':
            raise ValueError('Files retrieved from minIO are read-only. Use save() method to override contents')
        try:
            resp = self.client.get_object(self._BUCKET_NAME, object_name, kwargs)
            file = File(file=io.BytesIO(resp.read()), name=object_name)
        finally:
            resp.close()
            resp.release_conn()
        return file

According to Django docs, the _open method shall return a File object, so I wrap the returned bytes into that. Also, according to the minIO SDK, the connection pool created by self.client.get_object shall be closed and released, so I put all this into a try/finally block.

Please let me know your opinion on this approach. Does it satisfy the scenarios when instance.image.open() is called?

laymonage commented 4 years ago

Thanks for the quick response @theriverman! I haven't got the time to test it, but I believe it's a step in the right direction :+1: