Skip to content

Conversation

@spencewenski
Copy link
Contributor

@spencewenski spencewenski commented Aug 6, 2025

Problem

When a server function returns an error, the content-type header is not set.

Solution

In order to set the content-type header in the response, the FromServerFnError::Encoder::CONTENT_TYPE associated constant needs to make its way to where the response object is constructed. This PR achieves this with the following changes:

  • Add content_method to the Res trait to allow setting the Content-Type header for server function error responses.
  • Call the Res::content_method from the server function handler when an error is returned from the app's server fn impl.

Note: As mentioned in #4209, this issue also affects Leptos's middleware handlers. Unfortunately, resolving this for middleware requires a semver-incompatible change. The following PR contains a potential approach: #4249

Closes #4209

Problem
-------
When a server function return an error, the content type header is not
set.

Solution
--------
Use the `FromServerFnError::Encoder::CONTENT_TYPE` associated constant
to set the content type header on the returned API response.

Note: A similar issue occurs if an error is [returned in Leptos's
middleware
chain](https://github.com/leptos-rs/leptos/blob/b986fe11dc63a803d026f9ccae804e880c2a702b/server_fn/src/middleware/mod.rs#L77).
However, I believe resolving that will require adding a field to the
`BoxedService`, which would be an API-breaking change.
)
);
let content_type =
<Self::Error as FromServerFnError>::Encoder::CONTENT_TYPE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This formatting is kind of weird, but it's what cargo fmt wants.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer to modify error_response() to take and set the content type from the codec, then we wouldn't need to define a content_type function on Response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also prefer that, but I was trying to avoid introducing a breaking change. Would you prefer to modify error_response even though it would be a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it'll be better to have the better api and do the breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, sounds good. I’ll update when I get a chance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR, please take a look when you get a chance. I made a few additional breaking changes that I thought made sense and also resolve the middleware issue I originally tried fixing in #4216.

@benwis
Copy link
Contributor

benwis commented Aug 6, 2025

Have you looked at how the Errors are typically returned? If I recall correctly, they're usually not encoded/decoded using the standard logic and may not be valid JSON

@spencewenski
Copy link
Contributor Author

spencewenski commented Aug 6, 2025

Have you looked at how the Errors are typically returned? If I recall correctly, they're usually not encoded/decoded using the standard logic and may not be valid JSON

I think they're encoded by the FromServerFnError::Encoder associated type now, right? At least for errors returned in the body of the server function.

fn ser(&self) -> Bytes {

Copy link
Contributor

@benwis benwis left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to look into this, it can certainly get a bit tricky parsing the different layers of server fn code.

I think I'd prefer to modify error_response() here to take the codec's content_type and set it on the response. This way we shouldn't need a new associated fn on the Request and Response types

@spencewenski spencewenski changed the title fix: Set content type header for server function errors fix!: Set content type header for server function errors Aug 8, 2025
@spencewenski spencewenski requested a review from benwis August 8, 2025 00:13
"futures",
], optional = true, workspace = true, default-features = true }
thiserror = { workspace = true, default-features = true }
bon = { workspace = true }
Copy link
Contributor Author

@spencewenski spencewenski Aug 8, 2025

Choose a reason for hiding this comment

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

I'd be open to using typed-builder instead, or manually writing the necessary builder code.

@spencewenski
Copy link
Contributor Author

I think the following semver error is a false positive; I was able to implement it in one of the example crates.

--- failure trait_newly_sealed: pub trait became sealed ---

Description:
A publicly-visible trait became sealed, so downstream crates are no longer able to implement it
        ref: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.42.0/src/lints/trait_newly_sealed.ron

Failed in:
  trait server_fn::middleware::Service in file /home/runner/work/leptos/leptos/server_fn/src/middleware/mod.rs:46

https://github.com/leptos-rs/leptos/actions/runs/16836198701/job/47702412558?pr=4215#step:4:320

@spencewenski
Copy link
Contributor Author

Hey, @benwis , can you take a look at the updated PR when you get a chance? Thanks!

@benwis
Copy link
Contributor

benwis commented Aug 14, 2025

Sorry it's been a few days, had some work to tackle. I'm somewhat puzzled by the direction this took, I didn't think another trait like ServerFnErrorResponseParts should be needed. In the previous Iteration I believe you attached the content-type header by referencing the already set value from the currently selected codec.

    let content_type =
                    <Self::Error as FromServerFnError>::Encoder::CONTENT_TYPE;

Why not just pass the above as a value to error_response() here: 9ac211f
It's quite possible I've missed something though.

@spencewenski
Copy link
Contributor Author

spencewenski commented Aug 14, 2025

Sorry it's been a few days, had some work to tackle. I'm somewhat puzzled by the direction this took, I didn't think another trait like ServerFnErrorResponseParts should be needed. In the previous Iteration I believe you attached the content-type header by referencing the already set value from the currently selected codec.

    let content_type =
                    <Self::Error as FromServerFnError>::Encoder::CONTENT_TYPE;

Why not just pass the above as a value to error_response() here: 9ac211f It's quite possible I've missed something though.

ServerFnErrorResponseParts is a struct returned by FromServerFnError::ser that contains the data required to return the error response, including the serialized error as Bytes and the content type. I wanted to create a struct to contain this data instead of adding a content_type parameter to Res::error_response so that more data could be returned by FromServerFnError::ser and passed to Res::error_response in the future without needing another API-breaking change. The content type is still coming from <Self::Error as FromServerFnError>::Encoder::CONTENT_TYPE, it's just coming from the FromServerFnError::ser directly in the ServerFnErrorResponseParts struct instead of needing to get it separately.

Returning the data from FromServerFnError::ser also resolves the related issue with the server fn middleware from my other PR in a cleaner way.

Copy link
Contributor

@benwis benwis left a comment

Choose a reason for hiding this comment

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

A few minor nits and a couple deeper questions as we mess with these traits

@benwis
Copy link
Contributor

benwis commented Aug 22, 2025 via email

@gbj
Copy link
Collaborator

gbj commented Aug 24, 2025

In general I'm fine with this. I would prefer to use typed-builder rather than adding bon to the dependency tree, since typed-builder is already used in any leptos project.

It can't be merged until 0.9, given the breaking changes, and there are no plans for 0.9 in the near future, since releasing new breaking versions causes churn that can take the ecosystem a while to catch up to and is therefore kind of annoying for users.

I do think it's possible to do it with relatively fewer changes, by using default trait methods. See #4247 for a non-breaking approach, which we could merge into 0.8. (Assuming cargo-semver-checks doesn't disagree with me, which is always possible.)

@spencewenski
Copy link
Contributor Author

spencewenski commented Aug 24, 2025

In general I'm fine with this. I would prefer to use typed-builder rather than adding bon to the dependency tree, since typed-builder is already used in any leptos project.

It can't be merged until 0.9, given the breaking changes, and there are no plans for 0.9 in the near future, since releasing new breaking versions causes churn that can take the ecosystem a while to catch up to and is therefore kind of annoying for users.

I do think it's possible to do it with relatively fewer changes, by using default trait methods. See #4247 for a non-breaking approach, which we could merge into 0.8. (Assuming cargo-semver-checks doesn't disagree with me, which is always possible.)

Thanks, @gbj , the non-breaking approach you mentioned is essentially what I did originally. I would like for this to be able to be released relatively soon, so if 0.9 is not planned any time soon I’d prefer to take the non-breaking approach. The breaking approach could be done later when if/when 0.9 is planned. Any objections on that, @benwis ?

@benwis
Copy link
Contributor

benwis commented Aug 24, 2025

In general I'm fine with this. I would prefer to use typed-builder rather than adding bon to the dependency tree, since typed-builder is already used in any leptos project.
It can't be merged until 0.9, given the breaking changes, and there are no plans for 0.9 in the near future, since releasing new breaking versions causes churn that can take the ecosystem a while to catch up to and is therefore kind of annoying for users.
I do think it's possible to do it with relatively fewer changes, by using default trait methods. See #4247 for a non-breaking approach, which we could merge into 0.8. (Assuming cargo-semver-checks doesn't disagree with me, which is always possible.)

Thanks, @gbj , the non-breaking approach you mentioned is essentially what I did originally. I would like for this to be able to be released relatively soon, so if 0.9 is not planned any time soon I’d prefer to take the non-breaking approach. The breaking approach could be done later when if/when 0.9 is planned. Any objections on that, @benwis ?

It makes sense to me, sorry to make this a bit complicated. I'm hoping to get a patch release out soon and would love to have at least this partial release in it.

@spencewenski spencewenski changed the title fix!: Set content type header for server function errors fix: Set content type header for server function errors Aug 25, 2025
@spencewenski
Copy link
Contributor Author

spencewenski commented Aug 25, 2025

I updated the PR to go back to the original semver-compatible approach and included some (reworded) comment's from @gbj's sample PR. I also opened a separate PR for the semver-incompatible approach. It's based on main so it will need an update to remove the Res::content_type method. Feel free to take that PR and update it or let me know if you start working on 0.9 and I'd be happy to update it. Or, ignore it and take a different approach to solving the issue. 😂

If things look good please merge when you get the chance because I do not have merge permissions.

@spencewenski
Copy link
Contributor Author

spencewenski commented Aug 25, 2025

The semver check is failing due to a different change that’s already in main.

variant ReloadWSProtocol:Auto in /home/runner/work/leptos/leptos/leptos_config/src/lib.rs:286
semver requires new major version

https://github.com/leptos-rs/leptos/actions/runs/17198851352/job/48793426173?pr=4215

@spencewenski
Copy link
Contributor Author

Looks like the breaking change was reverted in main, so I synced my branch and all checks are passing now.

@spencewenski
Copy link
Contributor Author

Hey @benwis , can you merge this PR when you get a chance? I don't have merge permissions.

@gbj gbj merged commit 3b058e7 into leptos-rs:main Aug 29, 2025
283 checks passed
@gbj
Copy link
Collaborator

gbj commented Aug 29, 2025

Thanks for all your work, and for your patience and flexibility!

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.

Server functions do not set content-type header when an error is returned

3 participants