Proposal issue 15

[favoyang]

Try to fix issue #15. This is based pull request #10 from@jxltom, which added support for public/public-read bucket.

[jxltom]

This is unnessasry since self.bucket.sign_url in url method can already generate full aliyun oss url even MEDIA_URL is only set like /media/

In detail, the self.bucket.sign_url method will search the key, which is the combination of MEDIA_URL and file name, in OSS and then generates the full path of file in OSS.

[favoyang]

The point for this push has mentioned in #15 is about to support Aliyun CDN (to save money and get benefit from CDN).

In a typical oss+cdn setup,

• Create a public-read oss bucket.
• Upload media files with appropriate OSS settings.
• Serve media files using your cdn url i.e. static.yourdomain.com

sign_url always works, but it has nothing to do with CDN.

[jxltom]

Sorry you are right, I missed the CDN part...

But is it better to set self.location = getattr(settings, 'OSS_MEDIA_LOCATION', settings.MEDIA_URL), so people who are not using CDN can still fall back to use MEDIA_URL setting?

[favoyang]

Hmm, Django specify MEDIA_URL as the address to serve media file. It's a common setup to use default value "/media/" in dev mode, and switch to "http://media.yourdomain.com/..." in production mode.

I smell using MEDIA_URL as "oss_location" is a bad decision. Then for above production setup, your oss path will included your domain. Same for STATIC_URL.

So I decide to use '/media/', '/static/' as the oss default location. It's django's default value for MEDIA_URL and STATIC_URL in dev mode. Most developer won't change that, so they're fine to use this patch.

[jxltom]

Using MEDIA_URL to serve both static and media files are very confusing.

I believe by introducing a setting such as OSS_CDN_URL setting can solve this CDN issue.

def url(self, name):
    key = self._get_key_name(name)

    if self.bucket_acl == BUCKET_ACL_PRIVATE:
        return self.bucket.sign_url('GET', key, expires=self.expire_time)
       
   if getattr(settings, 'OSS_CDN_URL'):
       return urljoin(settings.OSS_CDN_URL, key)

    scheme, endpoint = self.end_point.split('//')
    return urljoin(scheme + '//' + self.bucket_name + '.' + endpoint, key)

[favoyang]

Using MEDIA_URL to serve both static and media files are very confusing.

No, MEDIA_URL is serving media files and STATIC_URL is serving static file. It's Django standard convention.

A little details

• for static file, mostly you use {% static 'file_path' %} template tag in template file to get that file url. Then you only need set right STATIC_URL for production mode.
• for media file, you use model.filefield.url to get the file url. The default file storage backend will construct the url by joining MEDIA_URL and file path. See base_url(), url() method in this file.. Then the OSS storage backend should just follow the conversion by using MEDIA_URL.

Hence, there's almost no chance to get a static file url via the storage.url method you provide. Then the OSS_CDN_URL is useless.

In another word, the static file only use collectstatic to upload file to storage backend (one way communication). Wanna a static file url? Do you work by constructing it using {% static 'file_path' %} template tag.

Hope this help.

And thanks for the discussion and patch @jxltom.

[jxltom]

You updated url method in OssStorage which will be used by both OssMediaStorage and OssStaticStorage and it has following lines. Obviously, if users set MEDIA_URL, both static and media files will be served by MEDIA_URL....

if settings.MEDIA_URL.startswith("http"):
    return urljoin(settings.MEDIA_URL, key)

[favoyang]

Again, good catch, I will see what I can do later. Probably reconsider your OSS_CDN_URL idea.

You got my idea that keep django convention and intro less new settings. So user can switch from one storage to another storage seamless.

[jxltom]

The static template tag will eventually call for url function in static file storage. Refer to https://github.com/django/django/blob/f40e71a957aa00b4572c19b269179cded6c8c500/django/templatetags/static.py#L115. It looks like you have a misunderstanding on how static files are served by static template tag.

Anyway, no matter for static or media files, you need to take good care of them in the url method.

[favoyang]

Yes, after checking the implementation of static template tag, it will eventually invoke the url method of STATICFILES_STORAGE as long as the contrib.staticfiles app is installed (it's by default). So my understanding of how static file url generated is wrong.

The default FileSystemStorage intros a base_url variable and fall back to settings.MEDIA_URL. The StaticFilesStorage set base_url to settings.STATIC_URL. That's how it handle static and media urls. Well a little tricky, but we can just follow the same approach.

[jxltom]

Very good. But it still breaks the compatibility with older django-oss-storage versions.

For uses who are not using CDN and set MEDIA_URL such as /mymedias/, with your PR, MEDIA_URL and OSS_MEDIA_LOCATION must be all set to /mymedias/ otherwise it won't work as expected. For users using older versions of django-oss-storage, their code will break if they upgrade.

I suggest using location=getattr(settings, 'OSS_MEDIA_LOCATION', settings.MEDIA_URL) ( same to OssStaticStorage as well) to fix the compatibility.

[favoyang]

@jxltom Thanks for the suggestion. Yes, my approach breaks back compatibility for users who changed MEDIA_URL. And your approach fails if user specify a full MEDIA_URL in production mode, and forget to set OSS_MEDIA_LOCATION. The actually url became something like $MEDIA_URL/$MEDIA_URL/$FILEPATH.

Conceptually assign MEDIA_URL to location is a bit confusing, however keep back compatibility means less pain. This logic seems satisfy both of us. Thought?

    fallback_location = settings.MEDIA_URL
    if fallback_location.startswith('http'):
        fallback_location = '/media/'
    location=getattr(settings, 'OSS_MEDIA_LOCATION', fallback_location)

[jxltom]

Nice work.

I'm fine with it as long as it doesn't break the backward compatibility and your last approach looks very good so far 😃

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ favoyang
❌ jxltom
_{You have signed the CLA already but the status is still pending? Let us recheck it.}