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

Statics no in FLASK_STATIC_DIGEST_HOST_URL root #21

Closed
ybenitezf opened this issue Jun 3, 2021 · 11 comments
Closed

Statics no in FLASK_STATIC_DIGEST_HOST_URL root #21

ybenitezf opened this issue Jun 3, 2021 · 11 comments

Comments

@ybenitezf
Copy link

ybenitezf commented Jun 3, 2021

I have noted that FLASK_STATIC_DIGEST_HOST_URL only work's if the statics are put on the root of the CDN, for example in a conventional/normal app, with the statics in the static folder, when i put FLASK_STATIC_DIGEST_HOST_URL='https://assets.example.com/myapp/static/' an URL for static/myimg.png will be https://assets.example.com/static/myimg.png and not https://assets.example.com/myapp/static/myimg.png as intended. So no sharing?

I ask in the event that ones needs to share the same CDN host for several app's. I know that the file names will include the hashes and that if two files conflict it means it is the same file, but i wold like to maintain the separation. Any thoughts ?

@nickjj
Copy link
Owner

nickjj commented Jun 3, 2021

Thanks, looks like I'll need to look into this in more detail and come up with a better strategy on joining the url paths.

It's this code:

    def _prepend_host_url(self, host, filename):
        return urljoin(self.host_url,
                       "/".join([self.static_url_path, filename]))

Maybe if it changes to:

    def _prepend_host_url(self, filename):
        return urljoin(self.host_url,
                       "".join([self.host_url, self.static_url_path, filename]))

It might work but I noticed while messing around with it in a REPL that it's very picky about having / at the end of some URLs, because it either doesn't put a / or it doubles up // depending on what's defined for your FLASK_STATIC_DIGEST_HOST_URL (self.host_url) or your self.static_url_path from Flask.

@ybenitezf
Copy link
Author

ybenitezf commented Jun 4, 2021

I mean, it is not a problem per se, i think the behavior now is correct as it is, FLASK_STATIC_DIGEST_HOST_URL as the name suggest is for a host.

Maybe it makes sense to add another variable, FLASK_STATIC_DIGEST_HOST_URL_PREFIX that can be added to the url:

    def _prepend_host_url(self, filename):
        return urljoin(self.host_url,
                       "".join([self.host_url, self.host_prefix, self.static_url_path, filename]))

Doing some tests in the console:

host_url = "https://assets.example.com/"
prefix = "myapp"
static_url_path = "static"
filename = "myimg.png"

print( urljoin(host_url, "/".join([prefix, static_url_path, filename])) )
# will print 'https://assets.example.com/myapp/static/myimg.png'

# even adding an aditional /
prefix = "myapp/"
print( urljoin(host_url, "/".join([prefix, static_url_path, filename])) )
# will print 'https://assets.example.com/myapp/static/myimg.png'
prefix = "myapp/someother/folder"
print( urljoin(host_url, "/".join([prefix, static_url_path, filename])) )
# will print 'https://assets.example.com/myapp/someother/folder/static/myimg.png'

Would it be okay if I ask you for a PR with that idea?

@nickjj
Copy link
Owner

nickjj commented Jun 4, 2021

The tricky part is when the host_url and / or static_url_path have a trailing or leading /, then urljoin starts to do weird things.

I think the current FLASK_STATIC_DIGEST_HOST_URL should support having a path instead of going with a separate variable. I can't really think of a case where it's more beneficial to have a separate variable for this since the bottom line is you would want all of your static files prefixed with this value, whether it's https://cdn.example.com or https://cdn.example.com/todoapp.

Ideally this would be a backwards compatible change with no change other than now your custom path would get used instead of being stripped out if you happen to use one.

If you want to take a shot at a PR that would be great. I would try to test it with the following combos:

# FLASK_STATIC_DIGEST_HOST_URL 
https://cdn.example.com
https://cdn.example.com/
https://cdn.example.com/myapp
https://cdn.example.com/myapp/
https://cdn.example.com/myapp/anotherdir
https://cdn.example.com/myapp/anotherdir/

# static_url_path (normally configured by Flask)
""
/
static
/static
static/
/static/
static/something
/static/something
static/something/
/static/something/

# filename
hey.png
images/hey.png
images/card/hey.png

Ideally it would work on both Linux and Windows. If you do import posixpath I think this will automatically convert os.path to use the correct value for your OS if you happen to end up using this in the solution.

@ybenitezf
Copy link
Author

Thanks, yup you are right:

I think the current FLASK_STATIC_DIGEST_HOST_URL should support having a path instead of going with a separate variable. I can't really think of a case where it's more beneficial to have a separate variable for this since the bottom line is you would want all of your static files prefixed with this value, whether it's https://cdn.example.com or https://cdn.example.com/todoapp.

I'll give it a shot

@Midnighit
Copy link

I'm having the same problem and as far as I can tell, this hasn't been resolved yet. Specifically my issue is that I'm using a Google storage bucket as CDN. Since those always have the format https://storage.googleapis.com/<bucket_name>, there is very little I can do to avoid the subdirectory.
As far as I understand it, your usage of urllib.parse.urljoin is the issue.
This stackoverflow question explained it pretty well:
https://stackoverflow.com/questions/10893374/python-confusions-with-urljoin
Seeing as the second argument of the urljoin is the return value of the flask url_for function and that always seems to prepend a slash to the url, the <bucket_name> is always lost no matter what I use as FLASK_DIGEST_HOST_URL.
Unless I'm missing some url_for argument that changes this?

@nickjj
Copy link
Owner

nickjj commented May 9, 2024

Hi @Midnighit, yep there's still a pending PR to potentially resolve it.

@Midnighit
Copy link

Since this PR has been dormant for almost 3 years now: any chance that this could be resolved even if the original author of the PR isn't reacting anymore? As of now I'm resolving it by creating my own CustomFlaskStaticDigest and overriding the original static_url_for method. The workaround is rather crude (strip any potential leading / of the result of url_for if a host url is given) and I would prefer a cleaner solution in flask-static-digest directly.

@nickjj
Copy link
Owner

nickjj commented May 10, 2024

Sure, how do you feel about opening a new PR to work towards this?

@Midnighit
Copy link

I will try. That would be my first time contributing this way with a PR though, so please bear with me. :-)
Either way, I probably won't get to it before Monday (have a nice weekend!).

@ybenitezf
Copy link
Author

Sorry guys, got my hand fulls right now and can not contribute

@nickjj
Copy link
Owner

nickjj commented May 17, 2024

It's all good, thanks for getting this started.

It's been resolved in: #38 and is published in v0.4.1.

@nickjj nickjj closed this as completed May 17, 2024
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 a pull request may close this issue.

3 participants