search

xyncro / freya

Freya Web Stack - Meta-Package
https://freya.io
Other
330 stars 30 forks source link

Unmatched route returns 200 #194

Closed et1975 closed 7 years ago

et1975 commented 7 years ago

Running HelloWorld and asking for /hell returns OK (200).

Is there a way to configure the router to return 404 for unmatched routes instead?

neoeinstein commented 7 years ago

Yes this is a bug in Freya.Core. The OwinMidFunc.ofFreya function needs to be modified to halt when the pipeline result is halt. Check out xyncro/freya-core#6, which has the patch as part of the .NET Core conversion. (This should be merged and released as a new alpha soon.

neoeinstein commented 7 years ago

Here's a direct link: https://github.com/xyncro/freya-core/pull/6/commits/5a8891a74a5022a2ce1b6c267500feaf561d1b57

et1975 commented 7 years ago

thanks!

kolektiv commented 7 years ago

Just a comment on this one now that I look at it... I wouldn't say a router should return 404 (or return at all) for an unmatched route, but pass through to the next pipeline component. That way you can write a general 404 handler to sit at the end of a pipeline chain, compose multiple routers in sequence, etc. If a router returns halt when no routes match, that would preclude some useful functionality. I'm not sure I'm totally understanding the issue mentioned or solved though, might be worth a chat!

et1975 commented 7 years ago

+1 for more useful functionality, anything would be better than 200 though.

panesofglass commented 7 years ago

@et1975 the default status code is 200 OK. Anything else should be configured. It's possible that we could change that in the Freya.Router, which could, for example, default to 404 Not Found for anything not explicitly mounted. However, changing this in Freya.Core would break the OWIN abstraction, which is to allow a user to determine how things are handled.

panesofglass commented 7 years ago

@neoeinstein does correctly halting cause the response status code to return 404 Not Found, or does that just correctly return the completed Task at that point?

neoeinstein commented 7 years ago

So, I spent some time discussing what was happening with @kolektiv. There is a difference between the abstraction used by Kestrel and by Katana.

In Katana (and I haven't verified this), the entire application is the OwinAppFunc, and the environment is set up with 200 OK as the default. Thus, if no route matches and it falls out the bottom of a router, the result is an empty 200 OK.

In Kestrel, we use an OwinMidFunc, and Kestrel initially sets the status code to 200 OK but also provides a "final" pipeline handler that sets the status code to 404 Not Found if it is invoked. In the PR on Freya.Core, returning Halt just returns a completed task to prevent any further handlers from taking part in modifying the request. In the degenerative case, a pipeline consisting of just Halt will return 200, and Next will return 404.

et1975 commented 7 years ago

Thanks for the explanation folks! Obviously I'm not qualified to say what default should be, I'm just looking for the way to configure it.