Skip to content

Download button#2

Merged
matze merged 7 commits intomatze:masterfrom
yannickfunk:download-button
Jun 23, 2022
Merged

Download button#2
matze merged 7 commits intomatze:masterfrom
yannickfunk:download-button

Conversation

@yannickfunk
Copy link
Copy Markdown
Contributor

This PR persists the file extension into the database and adds a download button for direct file download

@matze
Copy link
Copy Markdown
Owner

matze commented Jun 20, 2022

The download feature is much appreciated but why the need for the database layout change? The whole thing could be a lot simpler by just fetching the raw data from the database and then appending the extension on the response as requested by the user. Similar to how the paste view highlights on demand.

@yannickfunk
Copy link
Copy Markdown
Contributor Author

yannickfunk commented Jun 21, 2022

My problem was that I couldn't see a way to obtain the extension after the paste was created. Initially I thought it is as easy is getting the extension from Entry because Entry is a 1:1 mapping to the db content. But it appeared that Entry doesn't 1:1 reflect the db content because extension is not persisted.

Could you point me to an alternative way to obtain the extension when download()
https://github.com/yannickfunk/wastebin/blob/4e2b143e54af3138be0c839907ade805fed78d3f/src/rest.rs#L61
is called?

One dirty workaround I could imagine would be to use javascript to parse the extension from window.location.href and then pass it along with the id to the download/ route. But I don't like that

@matze
Copy link
Copy Markdown
Owner

matze commented Jun 21, 2022

Could you point me to an alternative way to obtain the extension

In the show handler id and ext are extracted from the final path segment. You could pass Key.ext (e.g using a getter) to the Paste template and reconstruct the filename there.

@yannickfunk
Copy link
Copy Markdown
Contributor Author

Thanks a lot, totally missed that possibility. I added an updated version

Comment thread src/rest.rs

Ok(Response::builder()
.header(header::CONTENT_TYPE, content_type)
.header(header::CONTENT_DISPOSITION, content_disposition)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it should be possible to use Response::from_parts together with TypedHeaders to set the headers in a typesafe manner.

Copy link
Copy Markdown
Contributor Author

@yannickfunk yannickfunk Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried using TypedHeaders but it doesn't seem very ergonomic for Response building. Also, the header for which the Typing is the most important ContentDisposition does not support attachement (see https://docs.rs/headers/latest/src/headers/common/content_disposition.rs.html#56).

I now validated the inputs and put constraints on the static strings. Let me know if this ok.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay, I will think about it myself a bit. But strange that this old issue is still not resolved.

@matze
Copy link
Copy Markdown
Owner

matze commented Jun 21, 2022

I'll approve and run the workflow but please squash the commits, so git bisect becomes a bit easier to do in the future.

@yannickfunk
Copy link
Copy Markdown
Contributor Author

As far as I know you can just choose the squash option when merging with the github ui. Let me know if it doesn't work

@matze matze merged commit 093aca9 into matze:master Jun 23, 2022
@matze
Copy link
Copy Markdown
Owner

matze commented Jun 23, 2022

I always assume people want to craft their own commits but since you are okay with squashing :-) anyhow, thanks!

matze added a commit that referenced this pull request Jun 23, 2022
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.

2 participants