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

Missing action Function Returns 500 Instead of 405 #5507

Closed
1 task done
joshboley opened this issue Feb 17, 2023 · 9 comments · Fixed by #6320
Closed
1 task done

Missing action Function Returns 500 Instead of 405 #5507

joshboley opened this issue Feb 17, 2023 · 9 comments · Fixed by #6320
Labels
bug Something isn't working feat:routing

Comments

@joshboley
Copy link

What version of Remix are you using?

1.13.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

  • Make any Remix route (page, resource, or api route) with an exported loader, but no action
  • Make a GET request to the route, notice success
  • Make a POST (or PUT, etc) request to the route, notice a 500 response

Example Route:

// routes/api/health.tsx
import {json} from '@remix-run/node'

export async function loader() {
  return json({
    status: 'success',
  })
}

Expected Behavior

During the non-GET request, POST in this example, a 405 status code should be returned, based on this discussion.

Actual Behavior

The returned status code is 500. Note however, that the Remix server outputs 405, but that is not the status code actually received by the caller.

Example curl:

curl -X POST http://localhost:3000/api/health -v

*   Trying 127.0.0.1:3000...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> POST /api/health HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.79.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< content-type: text/plain; charset=utf-8
< Vary: Accept-Encoding
< Date: Fri, 17 Feb 2023 22:03:04 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked
<
Unexpected Server Error

* Connection #0 to host localhost left intact
[object Object]%

But note the output in the terminal from the running Remix server:

POST /api/health 500 - - 159.584 ms
ErrorResponse {
  status: 405,
  statusText: 'Method Not Allowed',
  internal: true,
  data: 'Error: You made a POST request to "/logout" but did not provide an `action` for route "routes/__app/logout", so there is no way to handle the request.',
  error: Error: You made a POST request to "/logout" but did not provide an `action` for route "routes/__app/logout", so there is no way to handle the request.
@machour machour added the needs-response We need a response from the original author about this issue/PR label Mar 5, 2023
@machour
Copy link
Collaborator

machour commented Mar 5, 2023

@joshboley Can't reproduce on my side.
Would it be possible to share a reproduction on http://remix.new/ ?

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Mar 5, 2023
@jazanne
Copy link

jazanne commented Mar 20, 2023

I am also seeing this issue in 1.14.0

@jazanne
Copy link

jazanne commented Mar 20, 2023

@machour Have done a bit of digging into this and it seems like what's happening is that the 405 error response is not be identified as a valid Response so a "last resort" error response is returned with 500 code.

Perhaps related to this change from December #4782

} catch (error: unknown) {
if (isResponse(error)) {
// Note: Not functionally required but ensures that our response headers
// match identically to what Remix returns
error.headers.set("X-Remix-Catch", "yes");
return error;
}
return returnLastResortErrorResponse(error, serverMode);
}

@joshboley
Copy link
Author

@machour sorry for the late replay. I was able to reproduce this by using npx create-remix@latest with just the bare bones app. This only occurs when there is no default component being exported, meaning this only occurs for an API/Resource Route. The example in my original post is enough to reproduce the issue, but here's a link to a reproduction.

@jazanne thanks for digging into this and confirming!

@jazanne
Copy link

jazanne commented Mar 28, 2023

Any thoughts on whether or not this will be prioritized? It's a pretty disruptive issue for us since it skews the number of unexpected errors / 500s we see in monitoring.

@mfreundlich
Copy link

This is still reproducible on 1.15.0. Any word on this being prioritized? Like @jazanne mentioned, this leads to a large increase in 500s in monitoring.

@kiliman
Copy link
Collaborator

kiliman commented Apr 27, 2023

Here's a patch to fix the issue in Remix v1.15.0
https://gist.github.com/kiliman/a7d7e90d30b0d01e5ffe5426c9dfab69

diff --git a/node_modules/@remix-run/server-runtime/dist/responses.js b/node_modules/@remix-run/server-runtime/dist/responses.js
index d042d61..1e6d5c6 100644
--- a/node_modules/@remix-run/server-runtime/dist/responses.js
+++ b/node_modules/@remix-run/server-runtime/dist/responses.js
@@ -47,7 +47,7 @@ function isDeferredData(value) {
   return deferred && typeof deferred === "object" && typeof deferred.data === "object" && typeof deferred.subscribe === "function" && typeof deferred.cancel === "function" && typeof deferred.resolveData === "function";
 }
 function isResponse(value) {
-  return value != null && typeof value.status === "number" && typeof value.statusText === "string" && typeof value.headers === "object" && typeof value.body !== "undefined";
+  return value instanceof router.ErrorResponse || (value != null && typeof value.status === "number" && typeof value.statusText === "string" && typeof value.headers === "object" && typeof value.body !== "undefined");
 }
 const redirectStatusCodes = new Set([301, 302, 303, 307, 308]);
 function isRedirectStatusCode(statusCode) {
diff --git a/node_modules/@remix-run/server-runtime/dist/server.js b/node_modules/@remix-run/server-runtime/dist/server.js
index fd10bc5..b924771 100644
--- a/node_modules/@remix-run/server-runtime/dist/server.js
+++ b/node_modules/@remix-run/server-runtime/dist/server.js
@@ -110,6 +110,9 @@ async function handleDataRequestRR(serverMode, staticHandler, routeId, request,
     return response;
   } catch (error) {
     if (responses.isResponse(error)) {
+      if (error.headers === undefined) {
+        error.headers = new Headers()
+      }
       error.headers.set("X-Remix-Catch", "yes");
       return error;
     }
@@ -251,6 +254,9 @@ async function handleResourceRequestRR(serverMode, staticHandler, routeId, reque
     return response;
   } catch (error) {
     if (responses.isResponse(error)) {
+      if (error.headers === undefined) {
+        error.headers = new Headers()
+      }
       // Note: Not functionally required but ensures that our response headers
       // match identically to what Remix returns
       error.headers.set("X-Remix-Catch", "yes");

@brophdawg11 brophdawg11 added bug Something isn't working feat:routing and removed bug:unverified labels May 4, 2023
@brophdawg11 brophdawg11 self-assigned this May 4, 2023
@brophdawg11 brophdawg11 linked a pull request May 5, 2023 that will close this issue
@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label May 23, 2023
@brophdawg11 brophdawg11 removed their assignment May 23, 2023
@brophdawg11
Copy link
Contributor

This is fixed by #6320 and should be available in the next release (which should be 1.17.0)

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-c90c16e-20230524 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat:routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants