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

proposing to render a filename for downloaded files #607

Conversation

bahag-raesenerm
Copy link

hey T.Franzel,

please have a look at this PR and tell me what you think.

Also if the test coverage is sufficient or if you'd like to have to test the path where a spectacular_settings.title is available and used.

greetings and stay healthy,
Marius

@bahag-raesenerm
Copy link
Author

pipeline seems to fail for DRF 3.10 - trying to find the according documentation and then refactor the PR.

@tfranzel
Copy link
Owner

tfranzel commented Dec 8, 2021

Hi @bahag-raesenerm,

thanks, I'm doing well. Hope you do too. Thank you for the PR. This is actually a thing i was unhappy with for quite some time. However, I'm undecided which is the best solution for this. Here are my thought from the top of my head

  • There is no good reason to limit this to an explicit format param. I think we should provide the header always, since it is ignored when not applicable.
  • In theory I would prefer Content-Disposition: inline, to display the schema in the browser, however then we likely loose the filename.
  • The the title as filename makes sense, but some string normalization is probably advisable.

@bahag-raesenerm
Copy link
Author

Hey again,

so I've just used the format param condition since I could figure out fast enough how I'd else decide if it's a json or yaml file, also this renders it backwards compatible, right?
But, to be honest, I was somehow surprised that the filename wasn't already provided when downloading the file, so if you ask me I'd say make it a default.
about Content-Disposition: inline I'm not sure, would it break programmatic consumers? I can imagine use cases where it's a requirement to have a on demand specification available.
also I've nearly gone the default route about the filename and did some normalization but then I remembered a meme I've seen recently which went similar to: "you are old if you still fear whitespaces in filenames in 2021" - but yeah, no big deal to have this slugified or so.

about the incompatibility with drf 3.10 I've meanwhile got this via irc, just putting it here for later reference:

<john_doe> let me check the diff, I'll tell you what to do
<john_doe> Now: response.headers["Cache-Control"]
<john_doe> Before: response._headers["cache-control"]
<john_doe> and beware, on older version, the dict does not contains the value, but a tuple
<john_doe> ex: 3.12: response.headers["Cache-Control"] == "no-store"
<john_doe> ex: 3.10: response._headers["cache-control"] == ("Cache-Control", "no-store")

@tfranzel
Copy link
Owner

about Content-Disposition: inline I'm not sure, would it break programmatic consumers?

pretty sure programmatic consumers will simply ignore the header. i believe it is pretty much ignored except by browsers.

about the incompatibility with drf 3.10 I've meanwhile got this via irc, j

i don't consider this a big issue. py3.6, django 2.2 and DRF 3.10 will go EOL very soon anyway. We could simply skip the offending test for this DRF version.

i will test if we can combine inline with a filename. https://www.w3.org/Protocols/HTTP/Issues/content-disposition.txt does allow it but https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition does not explicitly show it.

tfranzel added a commit that referenced this pull request Dec 19, 2021
tfranzel added a commit that referenced this pull request Dec 19, 2021
tfranzel added a commit that referenced this pull request Dec 19, 2021
tfranzel added a commit that referenced this pull request Dec 19, 2021
tfranzel added a commit that referenced this pull request Dec 19, 2021
ease schema browser handling with "Content-Disposition" #607
@tfranzel
Copy link
Owner

hey, i found a more general approach to this in #621. i went ahead and merged it, but feedback is still welcome.

chrome is a bit picky about inline when the MIME type is unknown to chrome, though these are the recommended mimetypes. inlining works for ?format=json. The filename is honored in any case attachment or inline apparently does not matter.

@tfranzel tfranzel closed this Dec 19, 2021
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.

None yet

2 participants