Lack of support for nested routes of the same resource

[blelump]

OpenAPI version
3

Describe the bug
Lack of support for routes like:

/resource/new
/resource/:id

To Reproduce
Prepare spec for ie:

/resource/new
/resource/:id

and try to satisfyApiSpec against /resource/new route.

Current behavior
For /resource/new route it matches /resource/:id and spec fails.

Expected behavior
If /resource/new is being tested, it matches /resource/new in the spec.
If /resource/:id is being tested, it matches /resource/:id in the spec.

Additional context

• I don't have any meaningful proof (RFC or smth similar) in hand that defining routes in such way is good or bad practice. What I can tell is that never had a problem with such approach with any WEB server.
  EDIT: It seems that this is more like a convention in some environments rather than a standard itself (ie. route /resource/new).
• Internally the "problem" is in OpenApi3Spec.js:

    const openApiPath = this.paths().find(openApiPath =>
      doesOpenApiPathMatchPathname(openApiPath, pathname, matchingServerBasePaths)
    );
  

  I'm still not sure whether it should be fixed so we should discuss it further.
• Workaround: Use satisfySchemaInApiSpec("...");

[rwalle61]

Good spot! I agree that this is a genuine bug, and that the bug is in that matching logic.

The desired behaviour from that logic:
/new should match /new if we have a /new in our OpenAPI spec. If there isn't a /new in our OpenAPI spec, then it can try to match /{id}.

Current behaviour of that logic:
/new matches both /new and /:id, and we choose one of them (the first or the last, I can't remember). This should be easy to recreate in a test

Very happy to take a PR on this as I’m short on time at the moment. Otherwise I will see when I can fix it

[rwalle61]

I've created a branch with new tests that expose this bug, see the diff https://github.com/RuntimeTools/chai-openapi-response-validator/compare/57-match-path-without-params-over-path-with-params?expand=1

Are we agreed that the new tests capture the bug?

If so, feel free to PR a fix that satisfies those tests :)

[blelump]

@rwalle61 ,
essentially the problem is with https://github.com/troch/path-parser used internally to match routes. So when you'll prepare a matcher like:

p = new Path('/users/:param<new>/');

you'll get correct results:

> p.test('/users/345')
null
> p.test('/users/new')
{ param: 'new' }

but in order to achieve that, you need to change the route every time you iterate over the paths array, ie /test/responseBody/referencesSchemaObject needs to become /test/responseBody/:smth<referencesSchemaObject>, which already sounds like a workaround.

Another approach I see to tackle this problem is to #filter instead of #find here https://github.com/RuntimeTools/chai-openapi-response-validator/blob/master/lib/classes/OpenApi3Spec.js#L75 . Assume you get /a/b/c and /a/b/{c} and then first try to match for equality among found candidates (ie. "/a/b/c" == "a/b/c"). If matched, the path is found and if not (ie. /a/b/d is given), it should match a route with path parameter {c} at the end, so the latter will match.

[rwalle61]

Thanks for looking into this. I prefer the second approach (filter) because it makes nice conceptual sense: find all matching paths, and if there are multiple matches then choose the path that matches directly over the path that matches using a path parameter.

I think filter would work for that.

Do we have to consider efficiency? If so, instead of iterating over all paths (as filter would do) we could stop iterating once we’ve found a path that matches directly. So instead of find or filter it might be faster to do something like (pseudocode):

let matching Path;
paths.forEach(path => {
    if (match) {
        matchingPath = path;
        if (pathMatchesDirectly) {
            return;
        }
    }
});

I don’t know if that would actually be faster. It probably wouldn’t make a significant difference unless the OpenAPI spec were enormous.

Anyway, I don’t have strong opinions about this. I think your approach is spot on and I’m happy to see what you come up with!

[rwalle61]

To confirm, this is a genuine bug according to OpenAPI 3 spec:

A relative path to an individual endpoint. The field name MUST begin with a slash. The path is appended (no relative URL resolution) to the expanded URL from the Server Object's url field in order to construct the full URL. Path templating is allowed. 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.

[rwalle61]

Quite a lot of code has changed since I initially recreated the failure in a branch.

So here's a new branch recreating the failure https://github.com/RuntimeTools/OpenAPIValidators/compare/master...rwalle61:57-match-non-templated-path-instead-of-templated-path?expand=1

[rwalle61]

I don't have time to fix this immediately, so am happy if anyone wants to take over :)

[rwalle61]

@blelump and @Nitwel, I've merged a fix into master, would you mind pulling master and checking it works? I'll release if so

[rwalle61]

published in 0.9.4 (#111), please close this when you've verified it now works for you :)

[mojito317]

@rwalle61 could you please check this fix with a path like:
/resourceA/:id/resourceB?
I'm afraid it fails for me because of the same reason and matches to the similar /:id1/:id2/:id3 endpoint.