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

PathResolver can sometimes resolve to the wrong path #62

Open
pvince opened this issue Dec 7, 2018 · 3 comments
Open

PathResolver can sometimes resolve to the wrong path #62

pvince opened this issue Dec 7, 2018 · 3 comments

Comments

@pvince
Copy link
Contributor

pvince commented Dec 7, 2018

I have two different 'paths' in my application:
POST /api/v0/{grouping}/items/generate
GET /api/v0/{grouping}/items/{id}

The two paths appear in my openapi.yaml file in that order. While doing testing I noticed that when I called POST /api/v0/{grouping}/items/generate no controller was getting called.

I stepped through the exegesisRunner until I hit the PathResolver.resolvePath function and noticed that when I ran POST /api/v0/{grouping}/items/generate, resolvePath was returning GET /api/v0/{grouping}/items/{id} with 'grouping' set as a parameter, and with 'id' set as 'generate'.

When I stepped through resolvePath I noticed that the for loop @

for(const dynamicPath of this._dynamicPaths) {
has no 'early out'.

As in, it first finds the 'generate' endpoint, then keeps processing the path and the last path it processes that matches is the GET .../items/{id}, so it returns that.

As a temporary hack, I have changed my local code such that the for loop stops once it matches any path, but then it leaves the end user at the mercy of the order they define their endpoints in their specification file.

I am not sure if there is a way you can change the code to build up a list of 'matched' paths, and then prefer paths that have more 'fixed' values over paths that have more variables?

@niallmccullagh
Copy link
Contributor

Hi @pvince,

Just a quick observation.

Instead of posting to ‘POST /api/v0/{grouping}/items/generate’ why not post to ‘POST /api/v0/{grouping}/items‘ this would follow the more RESTful pattern of creating resources.

@pvince
Copy link
Contributor Author

pvince commented Dec 7, 2018

I am simplifying the use case a bit here.

The items are created based on a template, and depending on how that template is structured and the options passed to 'generate' a variable number of items can be created. Further, the generated items are tied back to the template that was used to create them. Finally, at this time we didn't want to let people directly 'create' the items, but there might be a point in time where we could let them do that, and we didn't want to change the definition of what it meant to POST to the ../items.

The templates are managed through a separate endpoint, and they can be individually created via a POST.

@jwalton
Copy link
Contributor

jwalton commented Dec 7, 2018

Yeah, it would be nice if this was a little more deterministic. I'm in the middle of crunch time over here, so I won't get to work on this until about a week from now. But I'll toss out some brainstorming here.

The relevant part of the spec says:

When matching URLs, concrete (non-templated) paths would be matched before their templated counterparts. Templated paths with the same hierarchy but different templated names MUST NOT exist as they are identical. In case of ambiguous matching, it's up to the tooling to decide which one to use.

So the good news is, we can do whatever we want! :D

Doing it based on which path matches first in the JSON/YAML would be nice, and easy to understand. The one downside to this is that, paths are stored as keys on an object in OpenAPI, and technically, according to the ECMAScript spec the keys in a JSON object are unordered. Most implementations (like node) will give you back keys in the same order you define them, but even node can violate that if you call delete on a key. That's perhaps being a bit pedantic though. Matching the "first" one also means we can break out of that loop early, which I like.

If we really want to be safe, we could go by order, and also add an extension to open-api so you can explicitly specify the order (like have an x-exegesis-path-priority: 1 to prefer path A over path B with x-exegesis-path-priority: 2, then we can sort them based on priority).

I am not sure if there is a way you can change the code to build up a list of 'matched' paths, and then prefer paths that have more 'fixed' values over paths that have more variables?

I like that. Maybe we could store a count of template variables with each path, and then when we match if we match more than one of them we pick the one with the lowest count? I was going to say "Too bad we can't break out early here", but actually we could combine this with the idea above and automatically assign priorities to paths based on the number of template variables. Then ordering largely doesn't matter. Or maybe sort by explicit priority first, then within each explicit priority sort by variable count. That gives you a fair amount of flexibility.

Or maybe that's a little too fancy, and we should just sort by template variable count for now and worry about custom priorities later if they're actually needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants