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

fix: undefined HTTP method handlers not responding with 405 #1041

Closed
wants to merge 20 commits into from

Conversation

edivados
Copy link
Contributor

@edivados edivados commented Sep 1, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

The docs state the following:

 If a handler is not defined for a given HTTP method, SolidStart will return a 405 Method Not Allowed response.

This is not the case.

  • Defined and undefined routes return 200 for undefined HTTP method handlers
  • Wildcard route placed at the root of routes directory exporting a POST method handler breaks server functions (i.e. [...404])

What is the new behavior?

  • Defined routes return 405 for undefined HTTP method handlers
  • Undefined routes return 200 for undefined HTTP method handlers (same as before)
  • inlineServerFunctions middlware gets called before apiRoutes to prevent wildcard routes from mistakenly handle requests for server functions
  • In island mode POST method handler of a route is skipped since it is used for navigation.

404 Page

[...404] wilcard route should look like this if it's desired to return 404 for all HTTP methods otherwise 405 will be returned.

const notFound = () => new Response(null, { status: 404 });

export const POST = notFound;
export const PUT = notFound;
export const PATCH = notFound;
export const DELETE = notFound;

export default function NotFound() {
  ...
}

Other information

@edivados edivados marked this pull request as ready for review September 1, 2023 21:28
@edivados edivados marked this pull request as draft September 1, 2023 21:42
@edivados edivados marked this pull request as ready for review September 1, 2023 21:46
@edivados edivados marked this pull request as draft September 1, 2023 22:04
Comment on lines 23 to +25
composeMiddleware([
apiRoutes,
inlineServerFunctions,
apiRoutes,
Copy link
Contributor Author

@edivados edivados Sep 2, 2023

Choose a reason for hiding this comment

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

Without changing the order, requests for server functions will be mistakenly handled by wildcard routes (like 404) that now have a handler for all methods.

This can be reproduced in the latest version 0.3.5 by adding a POST method handler to the [...404] page. It will break server functions.

@edivados edivados changed the title fix: undefined route handlers not responding with 405 fix: undefined HTTP method handlers not responding with 405 Sep 2, 2023
@ryansolid
Copy link
Member

Thank you for all your work on this. However, in setting up for SolidStarts next Beta Phase built on Nitro and Vinxi we are closing all PRs/Issues that will not be merged due to the system changing. If you feel your issue was closed in mistake. Feel free to re-open it after updating/testing against 0.4.x release. Thank you for your patience.

See #1139 for more details.

@ryansolid ryansolid closed this Dec 18, 2023
@edivados edivados deleted the method-not-found branch December 19, 2023 19:21
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