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

Can't download text file #7572

Open
4 tasks done
igor-lemon opened this issue Sep 13, 2021 · 30 comments
Open
4 tasks done

Can't download text file #7572

igor-lemon opened this issue Sep 13, 2021 · 30 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@igor-lemon
Copy link

New Issue Checklist

Issue Description

Can't download a text file. The file opens in a browser, I need to save it manually.

Steps to reproduce

Upload some text file. Try to download it by link.

Actual Outcome

Expected Outcome

I want to download the file immediantly.

Environment

Server

  • Parse Server version: FILL_THIS_OUT
  • Operating system: FILL_THIS_OUT
  • Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): FILL_THIS_OUT

Database

  • System (MongoDB or Postgres): FILL_THIS_OUT
  • Database version: FILL_THIS_OUT
  • Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): FILL_THIS_OUT

Client

  • SDK (iOS, Android, JavaScript, PHP, Unity, etc): FILL_THIS_OUT
  • SDK version: FILL_THIS_OUT

Logs

@parse-github-assistant
Copy link

Thanks for opening this issue!

  • ❌ Please fill out all fields with a placeholder FILL_THIS_OUT, otherwise your issue will be closed. If a field does not apply to the issue, fill in n/a.

@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2021

Thanks for opening this issue!

I can imagine that people have different expectations, depending on how they use the dashboard. Some may want an image for example to be displayed immediately in the browser, others may want it to be downloaded. Changing this one way or the other may disrupt or improve someone's workflow.

Browsers usually have an option to to download a resource instead of displaying it directly in the browser.

For example in Safari on macOS, if you hold the option key while clicking on a link, the browser decides to download it, regardless of what the server recommends. You can try this out by going here and clicking the Raw button in the file header bar.

Conversely, I don't think browsers do have an easily accessible shortcut to display a resource in a browser. So it may make more sense to keep the current behavior of opening resources in the browser by default and give the user the option to use a browser shortcut to download a resource.

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

@igor-lemon can this issue be closed?

@igor-lemon
Copy link
Author

If you think that it isn't a popular solution you can close the issue and close the PR.

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

It changes the behavior, but how can we decide which behavior should be preferred? If I read the PR correctly, it sets the content-disposition header to attachment for every file supplied by the file router. That could be a breaking change for existing deployments.

Instead of hard-coding whether the header should be inline or attachment, maybe we can find a more flexible solution, keeping the current behavior in place as to avoid a breaking change.

Alternatively, there is also the HTML <a> element's download attribute that for some browsers even overrides the header, see this. Not sure how other browsers interpret this, but this could move the decision to the front-end, keeping the backend un-opinionated.

@igor-lemon
Copy link
Author

Maybe my needs are exclusive, but I really don't need to show the file in a browser.
I think the best solution - set custom headers for the file when we save it or add some option justDownload<bool>. But the problem that we need to save this info about files to some collection/table.
Or maybe a more easy solution just to set some option when we init the Parse Server, if you like this way I can try to implement it.

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

And why don't you use the download attribute?

@igor-lemon
Copy link
Author

And why don't you use the download attribute?

Because it doesn't work :)
Parse generates a not strong path to the file (I use Amazon S3 as file storage)

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

What do you mean by "not strong path"?

@igor-lemon
Copy link
Author

Sorry, not a direct path

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

Do you mean the path points directly to S3 and not via Parse Server?

@igor-lemon
Copy link
Author

Oh, no, I found a problem...
Parse Server doesn't return correct Content-Type header, just return null value.
If I want to set binary/octet-stream type

new Parse.File(`${id}.data`, { base64: base64data }, 'binary/octet-stream');

It doesn't work...

@igor-lemon
Copy link
Author

igor-lemon commented Oct 2, 2021

So, I can propose 2 ways:

  1. Make binary/octet-stream as default content type (if mime couldn't detect type of file)
    or
  2. Add Mime types map as +1 extra config

@igor-lemon
Copy link
Author

Hi @mtrezza!
Check pls the PR ^

@mtrezza
Copy link
Member

mtrezza commented Oct 2, 2021

Maybe let's go one step back just to think this through.

Looking at your code you already set the file type when uploading the data to the server:

new Parse.File(`${id}.data`, { base64: base64data }, 'binary/octet-stream');

There are two ways of retrieving the file:

  • a) via Parse Server
  • b) directly from storage

In case of (a), is the file type preserved, so that the issue only occurs for (b)?

@igor-lemon
Copy link
Author

igor-lemon commented Oct 2, 2021

Check the code in PR.
Parse Server doesn't check the real content type of file, uses mime lib which tries to detect the content type by file extension. I save a file with *.data extension and Parse Server returns null in headers сontent type, and it's not valid.

@mtrezza
Copy link
Member

mtrezza commented Oct 3, 2021

Then what's the point of setting the content type when saving the file in:

new Parse.File(`${id}.data`, { base64: base64data }, 'binary/octet-stream');

@igor-lemon
Copy link
Author

This question is not for me, you can ask your friends who developed it, why they write that I can set the content type of the file but when I want to download this file parse server return trash in the headers.
Will you merge the PR or not?

@mtrezza
Copy link
Member

mtrezza commented Oct 3, 2021

That's a good one 🙂 I thought you maybe know more since you looked into this.

Before merging the PR we need to understand what we currently have. If one can set the content type when uploading a file, it shouldn't have to be "guessed" again when downloading it. So the next step would be to look into the JS SDK and find out what that content type parameter is used for. Do you want to take a look?

@igor-lemon
Copy link
Author

igor-lemon commented Oct 3, 2021

Yep. I checked the problem just on the Parse Server part, JS SDK works well and content type very important param.
When we create a file we can set content type (an example when we received a Buffer and we know that it's an image and we need to set "image/png" content type). The example of my file:

$ file --mime-type lic.data 
lic.data: application/octet-stream

But when we want to download this file, the parse server just checks the extension from the file name. But if mime lib checks some not standard extension (*.data) then returns null. I propose a fast fix: leave the same way but if mime lib couldn't detect content type by file extension just return binary/octet-stream, it provides download file process, of course, the most correct way would be to check on the server-side the real content type of the file type and return this value.

@mtrezza
Copy link
Member

mtrezza commented Oct 3, 2021

Do you know whether the context type is stored as a parameter of the Parse.File on the server? If that was the case, we could use that.

I think the priorities could be:

  1. Content type set when uploading a file
  2. mimelib detection by file extension if (1) was not set

I'm not sure what the side effects are when adding a fallback to binary/octet-stream as a third option. That may be an incorrect content type in some cases and maybe it would be better to not specify a content type at all instead of an incorrect one?

That's why I'm thinking fixing (1) would solve the issue you mention and maybe also fix a bug.

@igor-lemon
Copy link
Author

Yes, Parse.File makes a correct file with the correct mime type.
No, just check the code in src/Routers/FilesRouter.js:71, parse server doesn't try to detect the content type of an uploaded file, directly detect by file extension and return null for non-standard extensions.

I think that our conversation is going a little in the wrong direction. I do not need help to solve my issues, I just want to convey that the parse-server returns the wrong content type in the headers, which is based only on the file extension from the file name, and not on the real type.

As I wrote above, the best option would be to get the mime info of an uploaded file and return it, but it isn't a little work and I have no desire to do it because we can't even agree on a simple hotfix in 1 line that solves 99% cases.
I think we are not using our time rationally If you don't want to merge the PR just let me know, I'll use a fork. Thanks! 🙏🏻

@igor-lemon
Copy link
Author

igor-lemon commented Oct 3, 2021

Maybe you are a little misled, this issue I created just because you request an issue for a pull request. I don’t need to solve any own issues here. :)

@mtrezza
Copy link
Member

mtrezza commented Oct 4, 2021

The issue discussion here should justify the PR, not the other way around. That's why opening an issue is a requirement. Maybe we can clarify that in the PR template.

The issue I see is that your suggestion means setting an incorrect content type in some cases if the content type cannot be determined. What is the argument for that and why wouldn't it be better to not specify a content type at all instead of an incorrect one?

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
@dblythy
Copy link
Member

dblythy commented Mar 24, 2022

Perhaps we could add the option fileUpload.forceDownload which adds the following headers:

res.set("Content-Disposition", "attachment;filename=somefile.ext");

@mtrezza
Copy link
Member

mtrezza commented Mar 24, 2022

Wouldn't that be something one wants to determine on a per-file basis?

@dblythy
Copy link
Member

dblythy commented Mar 26, 2022

Probably, which I assume would come when we have a beforeGetFile trigger

@mtrezza
Copy link
Member

mtrezza commented Mar 26, 2022

That sounds like an exciting idea!

@dblythy
Copy link
Member

dblythy commented Mar 26, 2022

What are your thoughts? I think that the file get triggers have their own considerations and concerns, but it's probably the most versatile approach to this issue

@mtrezza
Copy link
Member

mtrezza commented Mar 26, 2022

Yep, agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

3 participants