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(server): prevent 404 error in fetch adapter when endpoint is set to '/' #4893

Closed
wants to merge 12 commits into from
Closed

Conversation

maddsua
Copy link
Contributor

@maddsua maddsua commented Oct 6, 2023

🎯 Changes

Fixed:

Setting endpoint property to '/' in fetchRequestHandler function call will result in url path being trimmed and in a "NOT_FOUND" error in all request attempts

worker.ts:

export default {

  async fetch(request: Request): Promise<Response> {

    return fetchRequestHandler({
      endpoint: '/',
      req: request,
      router: appRouter,
      createContext,
    });
  }
};

Issue

Expected behavior: request succeeds

Actual behavior (error log):

_TRPCClientError: No "mutation"-procedure on path "reateUser"
    at _TRPCClientError.from (C:\Users\maddsua\Downloads\examples-minimal-main\client\index.js:269:16)
    at C:\Users\maddsua\Downloads\examples-minimal-main\client\index.js:677:48  
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  meta: {
    response: Response {
      [Symbol(realm)]: null,
      [Symbol(state)]: [Object],
      [Symbol(headers)]: [HeadersList]
    },
    responseJSON: [ [Object] ]
  },
  shape: {
    message: 'No "mutation"-procedure on path "reateUser"',
    code: -32004,
    data: {
      code: 'NOT_FOUND',
      httpStatus: 404,
      stack: 'TRPCError: No "mutation"-procedure on path "reateUser"\n' +       
        '    at callProcedure (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:372:11)\n' +
        '    at inputToProcedureCall (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:516:24)\n' +
        '    at file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:628:49\n' +
        '    at Array.map (<anonymous>)\n' +
        '    at resolveHTTPResponse (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:628:28)',
      path: 'reateUser'
    }
  },
  data: {
    code: 'NOT_FOUND',
    httpStatus: 404,
    stack: 'TRPCError: No "mutation"-procedure on path "reateUser"\n' +
      '    at callProcedure (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:372:11)\n' +
      '    at inputToProcedureCall (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:516:24)\n' +
      '    at file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:628:49\n' +
      '    at Array.map (<anonymous>)\n' +
      '    at resolveHTTPResponse (file:///C:/Users/maddsua/AppData/Local/Temp/tmp-14488-OzljTyWav0KP/worker.js:628:28)',
    path: 'reateUser'
  },
  [cause]: undefined
}

Got this error while trying to setup trpc on cloudflare workers. Actually it's my first interaction with trpc, so please don't judge me too much

Upd: having a trailing slash in endpoing path also causes 404 error

@maddsua maddsua requested a review from a team as a code owner October 6, 2023 18:45
@vercel
Copy link

vercel bot commented Oct 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
trpc-next-app-dir ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2023 11:18pm
www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2023 11:18pm

@vercel
Copy link

vercel bot commented Oct 6, 2023

@maddsua is attempting to deploy a commit to the trpc Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@KATT KATT left a comment

Choose a reason for hiding this comment

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

Please add a regression test 🙏

@maddsua
Copy link
Contributor Author

maddsua commented Oct 6, 2023

Please add a regression test 🙏

I have one more thing in mind to check, but at the same time I'm not sure how to add a regression test in this project

@maddsua
Copy link
Contributor Author

maddsua commented Oct 6, 2023

9b82c5242a250d832549e9f424099a03901be67c Ensures that this change won't have any effect in case user has defined a procedure that starts with forward slash, like this:

'/getUserById': t.procedure.input(z.string()).query((opts) => {
  return users[opts.input]; // input type is string
}),

@maddsua
Copy link
Contributor Author

maddsua commented Oct 6, 2023

It looks like it won't be possible to just add another test inside https://github.com/trpc/trpc/blob/main/packages/tests/server/adapters/fetch.test.ts, as the server there starts once and it only uses empty endpoint path

@maddsua
Copy link
Contributor Author

maddsua commented Oct 6, 2023

9b82c5242a250d832549e9f424099a03901be67c Ensures that this change won't have any effect in case user has defined a procedure that starts with forward slash, like this:

'/getUserById': t.procedure.input(z.string()).query((opts) => {
  return users[opts.input]; // input type is string
}),

Although, I should probably revert this one, as it actually breaks things. '/proc' procedures work fine without it

@maddsua
Copy link
Contributor Author

maddsua commented Oct 6, 2023

Sorry for too many messages, the tests in this project are a bit overwhelming for me. I've done basic testing locally, but still not sure how to do this properly
@KATT

@KATT
Copy link
Member

KATT commented Oct 9, 2023

I can't push to your branch, here's a patch:

0001-add-tests.patch

But I tried with that patch and I couldn't get the test to fail when I reverted your change:

0002-hmm-this-doesn-t-change-anything.patch

@KATT
Copy link
Member

KATT commented Oct 9, 2023

you can run the tests with

pnm vitest fetch.test

@maddsua
Copy link
Contributor Author

maddsua commented Oct 11, 2023

I can't push to your branch, here's a patch:

0001-add-tests.patch

But I tried with that patch and I couldn't get the test to fail when I reverted your change:

0002-hmm-this-doesn-t-change-anything.patch

The test is not gonna fail unless there is a procedure with a URL-like name. Thanks for the patch, I'll try to bring it to my branch

Copy link
Member

@KATT KATT left a comment

Choose a reason for hiding this comment

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

Sorry for the slow response on this - could you join our Discord and @ me there and we'll push this over the line?

Had a bit too many open stale PRs and my GitHub notifications are a mess so I just forgot about this

@@ -20,7 +20,7 @@ export async function fetchRequestHandler<TRouter extends AnyRouter>(
};

const url = new URL(opts.req.url);
const path = url.pathname.slice(opts.endpoint.length + 1);
const path = url.pathname.slice(opts.endpoint === '/' ? 1 : opts.endpoint.length + 1);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is a bit too implicit - isn't it possible to look at the url defined above? Is the problem that .pathname starts with a /?

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've joined discord, no response there yet. Anyway. The problem is that without this change trpc assumes that if endpoint path is not empty it starts with forward slash and has characters after it. Which doesn't work if endpoint path is set to a single slash with no other characters after. It's a slight oversight in algorithm, as this length + 1 in fact implies that endpoint path has two slashes needed to be removed and here he have just one, so it just "eats" first character of procedure name instead

# Conflicts:
#	packages/server/src/adapters/fetch/fetchRequestHandler.ts
@KATT KATT changed the title Fix: Prevent 404 error with fetchRequestHandler when endpoint is set to '/' fix(server): Prevent 404 error with fetchRequestHandler when endpoint is set to '/' Nov 20, 2023
@KATT KATT changed the title fix(server): Prevent 404 error with fetchRequestHandler when endpoint is set to '/' fix(server): prevent 404 error in fetch adapter when endpoint is set to '/' Nov 20, 2023
This reverts commit 74b72e6.
KATT
KATT previously approved these changes Nov 20, 2023
# Conflicts:
#	packages/tests/server/adapters/fetch.test.ts
KATT
KATT previously approved these changes Nov 20, 2023
@maddsua
Copy link
Contributor Author

maddsua commented Nov 20, 2023

Extended this pr with a fix for trailing slash: #5067.
That's why I have a ton of regexes in my code 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants