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(transport-commons): Allow case insensitive route lookups #3353

Merged
merged 1 commit into from Nov 27, 2023

Conversation

daffl
Copy link
Member

@daffl daffl commented Nov 27, 2023

This pull request allows case insensitive route lookups by setting app.routes.caseSensitive = false.

Closes #3330

@daffl daffl merged commit a4a5ab6 into dove Nov 27, 2023
4 checks passed
@daffl daffl deleted the route-case-sensitivity branch November 27, 2023 22:58
@jeffreythomasprice
Copy link

Is it intended that when case-insensitive mode is on, all path parameters are to be lower cased as well? I suspect that it is not, given the check for the : prefix in the new code.

The behavior I see is that incoming path parameters are lower cased before they make it to our code. Our path parameters happen to be case sensitive, although the rest of the path is not. (Now that I'm seeing this, that's maybe a strange design decision we should revisit...)

I have a service like:

app.use('/foo/:bar/:baz')

The new code in packages/transport-commons/src/routing/router.ts does this:

getPath(path: string) {
  return stripSlashes(path).split('/')
  const result = stripSlashes(path).split('/')

  if (!this.caseSensitive) {
    return result.map((p) => (p.startsWith(':') ? p : p.toLowerCase()))
  }

  return result
}

If I invoke this endpoint with:

/foo/Something/SomethingElse

result will then be:

['foo', 'Something', 'SomethingElse']

None of those elements starts with :, so they will all get lowered.

@daffl
Copy link
Member Author

daffl commented Dec 4, 2023

Ah, the parameter name will be properly cased but not the value. That should indeed not be the case. The nodes themselves probably need to check it so this should be a follow-up issue to fix.

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.

Case sensitivity of path matching
3 participants