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.

Sign up for GitHub

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

Jump to bottom

Add a File type to axum-extra #2768

Closed
1 task done
joeydewaal opened this issue Jun 4, 2024 · 5 comments
Closed
1 task done

Add a File type to axum-extra #2768

joeydewaal opened this issue Jun 4, 2024 · 5 comments
Labels
A-axum-extra C-feature-request Category: A feature request, i.e: not implemented / a PR. E-easy Call for participation: Experience needed to fix: Easy / not much

Comments

@joeydewaal
Copy link
Contributor

  • I have looked for existing issues (including closed) about this

Feature Request

Would it be useful to add a response type for a file in Axum? This response type would set headers like Content-Type, Content-Disposition and Content-Length. This could be a File type just like the Html and other types but one that is more generic. I would suppose that this File type would live in the axum-extra crate.

Motivation

I think this would make Axum more user-friendly. Currently the way to respond with a file is through:

  • The Html, JavaScript, Css and Wasm types.
    • This could be too specific for a user.
  • Through the Body type and setting the right headers.
    • This is good but could be made easier.

Proposal

I was thinking of something like this:

pub struct File {
    file_name: String,
    mime_type: Mime,
    source: Body,
}


impl<F, M, B> From<(F, M, B)> for File
where
    F: Into<String>,
    M: Into<Mime>,
    B: Into<Body>,
{
    fn from((file, mime, source): (F, M, B)) -> Self {
        Self {
            file_name: file.into(),
            mime_type: mime.into(),
            source: source.into(),
        }
    }
}

impl IntoResponse for File {
    fn into_response(self) -> Response {
        let mut headers = HeaderMap::new();

        headers.append(
            header::CONTENT_TYPE,
            self.mime_type.to_string().parse().unwrap(),
        );
        headers.append(
            header::CONTENT_DISPOSITION,
            format!("attachment; filename=\"{}\"", self.file_name)
                .parse()
                .unwrap(),
        );

        if let Some(content_length) = self.source.size_hint().exact() {
            headers.append(header::CONTENT_LENGTH, content_length.into());
        }
        (headers, self.source).into_response()
    }
}

// example 1
async fn file_handler1() -> impl IntoResponse {
    let bytes = [0; 4096].to_vec();
    File::from(("main.rs", mime::TEXT_PLAIN_UTF_8, bytes))
}

// example 2
async fn file_handler2() -> impl IntoResponse {
    use tokio::fs::File as TokioFile;

    let stream = TokioFile::open("./main.rs")
        .map_ok(|file| FramedRead::new(file, BytesCodec::new()))
        .try_flatten_stream();
    File::from(("main.rs", mime::TEXT_PLAIN_UTF_8, Body::from_stream(stream)))
}

This does mean that axum-extra has to export the Mime type somewhere. Also this implementation could be too simple. Any feedback would be appreciated!

Alternatives

I already saw this issue which is kind of the same but also not really. A multipart response type seems more specific than the File type for me. #2624
@jplatte jplatte added C-feature-request Category: A feature request, i.e: not implemented / a PR. E-easy Call for participation: Experience needed to fix: Easy / not much A-axum-extra labels Jun 14, 2024
@jplatte
Copy link
Member

jplatte commented Jun 14, 2024 •
edited
Loading

Sounds like a neat idea, I would call this Attachment after the Content-Disposition header value that it sets. Here's my idea for the public API:

pub struct Attachment<T> {
    inner: T,
    filename: Option<HeaderValue>,
    content_type: Option<HeaderValue>,
}

impl<T: IntoResponse> Attachment<T> {
    pub fn new(inner: T) -> Self { /* set other fields to None */ }
    pub fn filename<H: TryInto<HeaderValue>>(self, value: H) -> Self { /* ... */ }
    pub fn content_type<H: TryInto<HeaderValue>>(self, value: H) -> Self { /* ... */ }
}

impl<T: IntoResponse> IntoResponse for Attachment<T> { /* ... */ }

I suppose if the try_into fails, that could just be logged and that part of the response omitted as if it wasn't specified. I think TryInto makes sense because we already use it for headers here. If the filename is not set, Content-Disposition would just be attachment (MDN says this is possible here). If the content_type is not set, no Content-Type header is sent.

Content-Length is set by hyper automatically if it knows the length AFAIK, and can be easily set independently otherwise (as part of the inner T, or by wrapping Attachment in a tuple).

@joeydewaal
Copy link
Contributor Author

joeydewaal commented Jun 16, 2024 •
edited
Loading

The name change sounds good. Nice that hyper sets the content-length header, that would make this type easier to work with. Your public API looks handy, however why is the filename an Option<HeaderValue> and not an Option<String>? I would think a HeaderValue would be created in the IntoResponse impl.

@jplatte
Copy link
Member

jplatte commented Jun 16, 2024 •
edited
Loading

So the reason HeaderValue is a different type from String is that HTTP header values have a different allowed character set from (UTF8) Strings. The TryFrom allows passing &str or String, but also a non-UTF8 string. I suppose this would usually be a pretty bad idea for attachment filenames, and content types, but usage with &str wouldn't be made more complicated by having the API flexible enough to also support non-UTF8.

The impl will be a little bit more involved as HeaderValues don't have convenient concatenation functions available, but you can do what I did in tower-https cors middleware: https://github.com/tower-rs/tower-http/blob/a1e3f8a28bdb118c98627b8fa2d05180049dc981/tower-http/src/cors/vary.rs#L29

joeydewaal added a commit to joeydewaal/axum that referenced this issue Jun 16, 2024
@joeydewaal
Refs: tokio-rs#2768 Add an Attachment type to axum-extra
07b6f15
@joeydewaal
Copy link
Contributor Author

Ahh I understand, thanks for your clarification. I have written a PR for this.
https://github.com/joeydewaal/axum-attachment/blob/main/axum-extra/src/response/attachment.rs

Now that I think about it, would it be useful to make the Content-Disposition header optional just like Content-Type and the filename. This could also mean that this this could be renamed back to File but I don't know if this would be desired.

Also how should the TryInto<HeaderValue> be handled? Should I not set the header or should this be a tracing log.

@jplatte
Copy link
Member

jplatte commented Jun 17, 2024

I think logging it with tracing and setting the field to None on error makes the most sense.

joeydewaal added a commit to joeydewaal/axum that referenced this issue Jun 17, 2024
@joeydewaal
Refs: tokio-rs#2768 log Attachment errors using tracing
0a9b52a
@joeydewaal joeydewaal mentioned this issue Jun 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-axum-extra C-feature-request Category: A feature request, i.e: not implemented / a PR. E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jplatte @joeydewaal