-
Notifications
You must be signed in to change notification settings - Fork 5
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
Strange partial match behaviour if you break the undocumented uniqueness constraint #9
Comments
That does seem like a bug rather than a restriction. |
I suggested intent of design purely based on the comment on the line above my link
I look forward to multi-line permalink syntax :) |
I’m afraid I won’t be able to look into it now as I’m out sick. Any help is appreciated with debugging the root cause :) |
Throwing Records the route registration changes in the following way Different endpoint function name: Same endpoint function name: I hope this helps, sorry to hear you are sick. I'd be happy to help if I had the time. |
I think I see the problem. Basically, it'll register each verb with each path which is what you are observing. This is tricky because it's related to how the internal state is stored which was done in a way that allowed using both decorators and a builder pattern. For example, it allows the following usage: class Test {
@route('/save')
@POST()
@PUT()
@PATCH()
save() {
/**/
}
} It would require a breaking change to the API in order to fix it, because I don't want to be maintaining 2 state representations 😅 |
awilix-router-core/src/state-util.ts
Line 322 in 5b6fd88
These lines of code replace a route if the target endpoint name is a duplicate.
The route then takes on some very strange characteristics.
In my example:
GET /:id endpointFn
POST /:id/query endpointFn
Using middleware to coerce the payload from body to query so the same endpoint can handle body and provide a way around the maximum URL length.
I stumbled onto this requirement when POST /:id wasn't behaving as expected for someone and I told them this is not even a route and should return 404.
It was not..
I have not dug further into the WHY of the code beyond this point as I have simply re-written my code to go around the issue.
But, the replacement appears to introduce partial route matching such that invalid routes are no longer falling through to 404.
I would suggest 2 things:
Firstly, document this restriction in the readme as this library enforces a restriction that it is not making clear to the consumer (for me that is via awilix-express)
Secondly, investigate why partial route-matching is caused by the replacement or undo the decision to replace.
Once I removed the duplicate endpoint in the builder configuration the POST /:id call returned 404 as I would have expected.
The text was updated successfully, but these errors were encountered: