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

One url make 2 action call #220

Closed
soulski opened this issue Jul 18, 2017 · 17 comments
Closed

One url make 2 action call #220

soulski opened this issue Jul 18, 2017 · 17 comments
Labels
status: fixed Issues with merged PRs, but not released yet. type: fix Issues describing a broken feature.

Comments

@soulski
Copy link

soulski commented Jul 18, 2017

Hi, I have one Controller which have 2 method like below

import { JsonController, Get } from 'routing-controllers'

@JsonController('/api')
export class APIController {

    @Get('/user')
    public getByUser() {
        console.log('output2')
        return Promise.resolve({ say: 'user' })
    }

    @Get('/:id')
    public getById() {
        console.log('output1')
        return Promise.resolve({ say: 'hello1' })
    }

}

I run server and then send http request in path '/user'. The result in console log print follow below

output1
output2

and I receive response as json { say: 'user' }. I take a look at source code and found that after action was called, routing-controller call next method which I suspect it is the reason why 2 action which match path url call. I test by insert after after middleware that didn't call next. The result is only getByUser was called. Is this intend behavior? It use invoke only getByUser, is't it?

@NoNameProvided
Copy link
Member

NoNameProvided commented Jul 18, 2017

By default, route variables (aka your :id) will match alphanumeric characters, so your /:id endpoints matches the /user route. See #213 (comment) for details.

@marshall007
Copy link

@NoNameProvided this still seems like a bug though. It shouldn't be possible for controller methods to fall through like middleware. Once a controller method has handled the request that should be the end of it.

@NoNameProvided
Copy link
Member

Yeah, it's not mentioned in the docs and this is what makes it seems like a bug the most, also that you can only return response from the first route which was hit. PR-s are welcomed for this as for any other issue, but first lets ask @pleerock if it was intentional or not?

@ArcanoxDragon
Copy link

I'm running into this issue as well. I have an action which listens on /js/common.js and one which listens on /js/:path*, with /js/common.js being first. In Express, if a route were to hit /js/common.js, it would immediately stop propagation and run that action. With routing-controllers, it's hitting /js/common.js first and then falling through to /js/:path*, causing an error when setting some headers, because they've already been sent by /js/common.js. Routes should not fall through.

@ArcanoxDragon
Copy link

ArcanoxDragon commented Jul 19, 2017

I just made a pull request with a solution to this problem that I have now been using in my application (see #222). Place @Terminates() on your most specific route (and make sure to define it first).

If you don't feel like waiting for the PR to be accepted (assuming it is), clone my issue-220 branch, run npm install && gulp package, and replace the contents of your node_modules/routing-controllers folder with the new contents of build/package in the cloned repo (tedious, but it works).

Scratch this...it works, but after further consideration, I don't think that falling through should really be the default (after all, in Express routes, you have to explicitly fall through by accepting a next parameter and calling it in a route handler...falling through should be explicit in this library as well).

@NoNameProvided
Copy link
Member

NoNameProvided commented Jul 19, 2017

Actually I don’t think anyone takes advantage of falling through requests. So I think we can make this the default behavior without adding an extra option to re-enable it.

I would like to see this implemented as well but as this is a breaking change let’s wait until @pleerock let’s us know if it’s a bug or a feature :D

@ArcanoxDragon
Copy link

I just made a pull request (#223) which changes the default behavior to no-fallthrough, with an option to re-enable it. So, technically it's a breaking change, but it can be reverted per-server with a global option.

@MichalLytek MichalLytek added the type: fix Issues describing a broken feature. label Aug 7, 2017
@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

okay, again we have issue with our next call after action execution. I think this will be fixed once we decide what to do with next call and most probably remove it. Anyone from contributors can take initiative to implement those fixes?

@MichalLytek
Copy link
Contributor

When we remove next call, we have to build our own midleware stack handler to make after middlewares work. As now we are discussing the need of after middlewares in classic (req, res, next) way - it could be replaced with enhanced interceptors that have access also to action metadata and returned value.

@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

If we remove next call then I dont think we need after middleware...

@Diluka
Copy link
Contributor

Diluka commented Sep 30, 2017

@soulski
There are something wrong with your API design. You should obey the RESTful patterns. APIs should distinguish from each other.

api/user and api/:id have ambiguity if the request uri is api/user. The server doesn't know the :id can't be user. And the order of method definition is not reliable.

@AntSworD
Copy link

AntSworD commented Jun 5, 2018

I think koa router style is a good solution, according their code order, unless the first route has next

@The-Firexx
Copy link

As anyone knows how to solve it? I'm facing the same problem and I don't know how to tell routing-controllers to not run both methods.
Although this isn't technically a major bug cause the response is send correctly, the log shows that he is trying to run both method but he fails entering the second time cause the headers were already changed.

@jotamorais
Copy link
Member

@The-Firexx, It's been a really busy year-end so far and I've been tied up but as earlier as the 2nd week of 2020, I will create a milestone and organize the project board and we will start the issues and PRs prioritization and hopefully clean up at least the PRs queue which will allow us to set a baseline and start working on the open issues and feature requests.

@github-actions
Copy link

Stale issue message

@github-actions github-actions bot added the status: awaiting answer Awaiting answer from the author of issue or PR. label Feb 16, 2020
@NoNameProvided NoNameProvided added status: fixed Issues with merged PRs, but not released yet. and removed status: awaiting answer Awaiting answer from the author of issue or PR. labels Aug 9, 2020
@NoNameProvided
Copy link
Member

This has been fixed in #568.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: fixed Issues with merged PRs, but not released yet. type: fix Issues describing a broken feature.
Development

Successfully merging a pull request may close this issue.

10 participants