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

Inconsistent router behavior #2655

Closed
wants to merge 2 commits into from
Closed

Inconsistent router behavior #2655

wants to merge 2 commits into from

Conversation

za-creature
Copy link
Contributor

Fixes an inconsistency within the router:

express = require "express"

app = express()


app.param "num", (req, res, next, value) ->
  if value.match(/[0-9]+/)
    next()
  else
    next("route")


app.param "alpha", (req, res, next, value) ->
  if value.match(/[a-z]+/)
    next()
  else
    next("route")


app.get "/:alpha/:num", (req, res) ->
  # this works for /asd/123
  res.status(200).send("Matched Alpha/Num")


app.get "/:alpha/static", (req, res) ->
  # this works for /asd/static
  res.status(200).send("Matched Alpha/Static")


app.get "/:num/static", (req, res) ->
  # this works for /123/static
  res.status(200).send("Matched Num/Static")


app.get "/:num/:alpha", (req, res) ->
  # but this doesn't work for /123/asd (generic 'no route matched' 404 error)
  res.status(200).send("Matched Num/Alpha")


app.listen 1234, ->
  console.log("Ready")

This could probably be fixed by using a param regexp instead of a function. However there are cases when that's not enough, for example if you're trying to get an object from a database and fall back onto another route if you couldn't find said object.

Fixes an inconsistency within the router:

```coffeescript
express = require "express"

app = express()

app.param "num", (req, res, next, value) ->
  if value.match(/[0-9]+/)
    next()
  else
    next("route")


app.param "alpha", (req, res, next, value) ->
  if value.match(/[a-z]+/)
    next()
  else
    next("route")


app.get "/:alpha/:num", (req, res) ->
  # this works for /test/123
  res.status(200).send("Matched Alpha/Num")


app.get "/:alpha/static", (req, res) ->
  # this works for /test/static
  res.status(200).send("Matched Alpha/Static")


app.get "/:num/static", (req, res) ->
  # this works for /123/static
  res.status(200).send("Matched Num/Static")


app.get "/:num/:alpha", (req, res) ->
  # but this doesn't work for /123/asd (generic 'no route matched' 404 error)
  res.status(200).send("Matched Num/Alpha")


app.listen 1234, ->
  console.log("Ready")
```

This could probably be fixed by using a param regexp instead of a function. However there are cases when that's not enough, for example if you're trying to get an object from a database and fall back onto another route if you couldn't find said object.
@za-creature za-creature changed the title Inconsistent rounter behavior Inconsistent router behavior May 19, 2015
@dougwilson
Copy link
Contributor

Good catch! If you want to practice, please feel free to add a test, otherwise I'll add one for you :)

@za-creature
Copy link
Contributor Author

I'll give it a shot when my shift is over

@za-creature
Copy link
Contributor Author

I just ran 670 unit tests in less than 2 seconds. What kind of magic is this?

za-creature pushed a commit to planetary/kepler that referenced this pull request May 20, 2015
@dougwilson dougwilson added this to the 4.13 milestone Jun 19, 2015
@dougwilson dougwilson mentioned this pull request Jun 19, 2015
6 tasks
@za-creature za-creature deleted the patch-1 branch June 22, 2015 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants