Provide a reason when route cannot be found.

[maciejtrybilo]

Currently when the router cannot find an endpoint for a requested resource the reason in the response is "Not Found". This PR makes the reason more specific which allows the 404 to be easier do diagnose.

Checklist

• Circle CI is passing (code compiles and passes tests).
• There are no breaking changes to public API.
• New test cases have been added where appropriate.
• All new code has been commented with doc blocks ///.

[0xTim]

+1 on this, might be less noisy to just print the path rather than the whole string in the reason though - open to suggestions

[maciejtrybilo]

Thanks @0xTim. I don't mind either way. I'm happy to change if the reviewer prefers.

BTW, any idea why circle ci is stuck on one of the tests? It's been pending for days...

[0xTim]

@maciejtrybilo at a guess it's waiting for a review approval to be kicked off

[calebkleveter]

The compatibility suite needs to be run manually.

[maciejtrybilo]

@0xTim Updated to only print the path.

[MrLotU]

LGTM. CC @tanner0101

[tanner0101]

Hmm I'm not sure I understand the reasoning behind this. As a user, why would I need to see the path that I just typed in to request the resource?
I could see this being useful in logs, but I'm not sure about being returned to the user.

[0xTim]

@tanner0101 its to bring parity with Vapor 2 and for logs as well. Especially in a browser there are lots of 404s that confuse people (mainly favicons and Apple touch icons) so it would be good to surface those with the 404s.

[maciejtrybilo]

@0xTim Thanks! Do you have the credentials to rerun the failed test? It looks like it timed out.

[0xTim]

@maciejtrybilo I do not unfortunately (although I think I should do @tanner0101 ?)

Anyway, I don't think rerunning would help, it looks like the error is a package dependency issue with that package in the compatability suite, though it was last run a while ago so might work now.

[0xTim]

@tanner0101 bump

[kylebrowning]

This is much more useful for logs than it is the browser.

Hosting on AWS you just see abort 404 in cloud watch so you get nothing.

[MrLotU]

IIRC by adding it to Abort it'll both appear in logs & in the HTTP response, correct?

[tanner0101]

I still don't fully understand the benefit behind this. In the browser, the console shows the request for a given error which includes the path.

[kylebrowning]

@tanner0101 I can provide some context here.

on AWS, this is all you get to see when there is a 404.

I wrote my own middleware to solve this, but it would be nice to have OOB

[tanner0101]

@kylebrowning ah okay, that context helps a lot. I agree that could be improved. That should be fixed by improving ErrorMiddleware which has access to the incoming request and can get the path that way.

[kylebrowning]

@tanner0101 I agree with that approach, that's why I copied ErrorMiddleware and just modified it.

[0xTim]

@tanner0101 do you have permissions to kick the CI or shall I just merge it?

[MrLotU]

I'll see if I can give the CI a kick

[MrLotU]

@0xTim CI is being weird, but builds are green, so it's fine by me.

[tanner0101]

From my previous comment, I think this should be fixed by logging the path from either ErrorMiddleware or in the ApplicationResponder. I don't think putting the path in the reason make sense.

That should be fixed by improving ErrorMiddleware which has access to the incoming request and can get the path that way.

[0xTim]

@maciejtrybilo do you want to do the update?

[maciejtrybilo]

@0xTim Yep, I should have some time soon.

[maciejtrybilo]

Closing and opening a new PR instead following @tanner0101 suggestions: #2170