Validation errors when paths have multiple path parameters

[bfreuden]

Which package are you using?
chai-openapi-response-validator

OpenAPI version
3

Describe the bug
When finding the OAPath matching a request path, the validator does not take into account the number of path parameters.
For the moment it only checks the existence of path parameters: OAPaths having no path parameters are favored over templated OAPaths (paths with path parameters).

To Reproduce

Create an API like this:

POST /shops/{shop}/pets/_search
GET /shops/{shop}/pets/{pet}

Depending on the declaration order of paths in the spec, a request like POST /shops/12345/pets/_search, might unexpectedly raise a validation error like this:
Request operations found for path '/shops/{shop}/pets/{pet}' in API spec: POST
Indeed: there is only a GET here.
That's because the current strategy based on the existence of path parameters does not work here since both paths are templated paths.

Expected behavior
I expect the library to realize that /shops/{shop}/pets/_search was called and not to throw any validation error.

Are you going to resolve the issue?
Yes: I have already written 2 tests (one for chai-openapi-response-validator and one for jest-openapi) based on the preferNonTemplatedPathOverTemplatedPath.test.ts tests and I have corrected the code.

[bfreuden]

The pull request number is #230
As it is my first contribution to this repository, I might need some guidance.

[rwalle61]

Thanks @bfreuden! Taking a look at this. On first glance your PR seems great :) Just checking though, is this issue different to #164?

[bfreuden]

Thank you @rwalle61 for the lightning fast feedback!
TBH I took me quite some time to come up with a (hopefully?) no-nonsense answer to your question. I went through:

1. Lack of support for nested routes of the same resource #2 #164
2. Lack of support for nested routes of the same resource #57
3. Ordering of matches when no path is concrete OAI/OpenAPI-Specification#2356
4. https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#pathsObject

Short answer
It is the same issue.
My PR is invalid and, as is, you should reject it since I'm going to do a second version :)
In the long answer I'll explain why and how (IMHO) the PR could be made valid.

Long answer
It's too bad you didn't get any answer from the authors of the spec to your question in bullet point 3 above (what is the official definition of paths with "same hierarchy"?).
I think the point made by jchook here is invalid and might have discouraged any action from you guys that get our developer backs every day :). He is saying:

/resource/{id}
/resource/new

This seems ambiguous, even with smart matching. What if you used short codes for IDs and had a resource with id = new?

Although the spec says (and it already said so when jchook wrote):

Assuming the following paths, the concrete definition, /pets/mine, will be matched first if used:

/pets/{petId}
/pets/mine

Those are exactly the same examples and the spec is very clear: this is no ambiguity (so jchook is wrong).

There are plenty of APIs having this non-ambiguity like Elasticsearch (look for "/deployments/{deployment_id}" and "/deployments/_search" in https://cloud.elastic.co/api/v1/api-docs/spec.json). Even the official petstore example has this non-ambiguity (look for "/pet/{petId}" and "/pet/findByStatus" in https://petstore3.swagger.io/).

Now, getting back to your question (what is the official definition of paths with "same hierarchy"?), I believe the answer is:

function sameHierarchy(oapth1, oapath2) {
  const pathParamRegex = /\{[^}]+\}/ 
  return oapth1.replaceAll(pathParamRegex, "{}") == oapth2.replaceAll(pathParamRegex, "{}")
}

But I agree with you: the spec is unclear.
At first I was tempted to consider that the author of the spec were thinking the problem in a "left-to-right greedy iterative way": if 2 paths can match (so they have the same number of /-separated "elements"), then compare each element of the paths starting from the left: if one is template and the other is not, then favor the concrete one and... end of the story (no need to go look at the rest of the paths).

But it is not the case, otherwise they wouldn't have written this:

The following may lead to ambiguous resolution:

/{entity}/me
/books/{id}

To be honest (and not so humble) I'm not sure it is very clear to the authors of the spec. Otherwise they would have been able to state it clearly (and to answer your question) :).

Still, I have the feeling that a reasonable approach to this problem has to be implemented.

What is wrong in my PR? I'm counting template path elements and not considering their position.
In this example I would favor the second path (and it is a dubious choice):

/pets/{petId}/fur/{attribute}
/pets/mine/{a}/color

I think there might be an unopinionated solution though (I mean: not "betraying" the spirit of the spec or "over-interpreting" it) and I would be grateful if you could give me your opinion.

Proposed solution

That solution consists in generalizing this example coming from the spec:

Assuming the following paths, the concrete definition, /pets/mine, will be matched first if used:

 /pets/{petId}
 /pets/mine

I think, for the sake of conciseness, they should have written this (and it would have been enough to make their point):

 /{otherPlace}
 /here

But instead spec authors chose to add a common prefix (/pets) and I propose to extend this notion of common prefix.

 /shops/{shopId}/pets/{petId}
 /shops/{shopId}/pets/tenants_pet

In that example both paths have a common prefix (/shops/{shopId}/pets) and those prefix are equal in the pretty strict sense defined above (ignoring all path parameter names).

Since both path share a common prefix, we are back to the only example clearly stated as non-ambiguous by the spec (a template path vs a concrete path):

/{petId}
/tenants_pet

And that's it. With this new solution the behavior for this example will still be undecided (but the current PR has an opinion about it):

/pets/{petId}/fur/color
/pets/mine/{a}/{b}

Since the following paths are unambiguous as well (they are both concrete with a common prefix):

/prefix/a
/prefix/b

I also propose to extend the algorithm to support this:

 /shops/{shopId}/pets/{petId}/color
 /shops/{shopId}/pets/tenants_pet/color

But anything beyond (additional path parameters) will lead to unpredicted behavior.

What do you think?

[rwalle61]

Hi @bfreuden, thanks for your careful research, thought, and implementation - that's the type of contribution we're pleased to see! Here are my thoughts, let me know what you think :)

As you say, the official guidance is not precise:

In case of ambiguous matching, it's up to the tooling to decide which one to use.

So should we decide which one to use? So far our tool has only implemented official behaviour. This has some benefits:

• users, maintainers, contributors can just need use the official OpenAPI Specification as the sole source of truth.
• The official spec is thought out by the entire OpenAPI community, rather than just us
• the less behaviour we commit to, the less code we have to maintain

It could be worth giving up some of these benefits if many users request a particular behaviour. But I'm not sure we know what users want yet. For example, you've suggested 2 ways our tool could behave:

1. match path with fewest templated parameters (your first PR, and @mojito317 suggestion in Lack of support for nested routes of the same resource #2 #164)
2. match path with no templated parameters, after common prefixes (your second PR)

I think both suggestions are reasonable, and probably prefer option 1 because it's simpler - e.g. your first PR is much simpler than the second. @mojito317 originally suggested option 1. And you've suggested both, and now prefer option 2 to 1.

If we're not sure which is best, perhaps we shouldn't commit to any of them. That said, I'm very glad you raised this discussion and contributed two great PRs :) I suggest leaving this issue open and seeing if people upvote it or comment, to swing towards any of the suggestions. What do you think?

[bfreuden]

Hi @rwalle61, thanks again for the fast and thorough reply! Very appreciated :)

I really understand your point: sticking to the spec is a sound position.
However, to be honest, I believe there is a problem with OpenAPIValidators.
I can only believe because I cannot prove my claim (since the spec is not precise enough).

Let me show you this: https://github.com/bfreuden/openapi-validation-tests
That repo is containing 3 OpenAPI-based web services implemented with 3 different stacks:

• Java Vert.x Web API Contract
• NodeJS express-openapi
• Python fastapi

Each of them is implementing this API:

GET  /shops/{shop}/pets/{pet}
POST /shops/{shop}/pets/_search

The repo is also containing mocha tests based on chai-openapi-response-validator:

• 3 "manual" spec tests: one for each web service
• 3 chai-openapi-response-validator tests: one for each service

Here is the result:

  Test all servers
    ✓ Python fastapi server must satisfy basic manual API spec tests
    1) Python fastapi server must satisfy API spec
    ✓ Java Vertx Web API Contract server must satisfy basic manual API spec tests
    2) Java Vertx Web API Contract server must satisfy API spec
    ✓ NodeJS express-openapi server must satisfy basic manual API spec tests
    3) NodeJS express-openapi server must satisfy API spec

I agree this is no proof... but it sound surprising that 3 frameworks (implemented by different persons) are able to leverage this spec (indeed: manual spec tests do succeed) while chai-openapi-response-validator has troubles with it.

I pushed the experiment a little further with Vert.x: as stated in the OpenAPI specification, it is indeed exhibiting unpredictable behavior with this:

The following may lead to ambiguous resolution:

/{entity}/me
/books/{id}

Depending on the declaration order in the spec file it will use the first or the second handler.
But the behavior with get and search pets is very sound and does not depend on declaration order in the spec file.

In the end I am left wondering: have we failed to understand the OpenAPI specification (I believe it is the case)? And how come other people seem to have gotten it right (I don't know)?

I will push the experiment a little further (to validate my intuition that the second PR is better) and keep you posted.

[bfreuden]

Sorry: I have not answered (good) questions you asked in your last message because I feel I can't for the moment.
But they're still on my mind :)

[bfreuden]

Asked the question on Stackoverflow and got an answer from the 4th top contributor of the OpenAPI-Specification github repo.
This is the opinion of one person though, so it should be handled with care.

[rwalle61]

Thanks for raising this on Stackoverflow and in the OpenAPI repo, it'd be awesome if you can get an official OpenAPI answer as to what behaviour we should implement 👍

[bfreuden]

@rwalle61 thanks for commenting my issue in the OpenAPI repo. It would be great to get an official answer indeed!

I'm sorry I've been busy these days.
I've been thinking about that "ambiguous" case though (in a background thread of my mind :)).

I'm putting quotes around "ambiguous" because it is (according to the OpenAPI specification, sensu stricto), but it does not seem to be (according to the tools I've tested so far, and according to an OpenAPI contributor).

For the sake of the argument, let's stick to the spec and consider it ambiguous though :).

In this case I've come to the conclusion that OpenAPIValidators is opinionated (compared to the OpenAPI spec) since it is definitely choosing a path: it is choosing the last matching OA path appearing in the OpenAPI spec of the API.
Not the first one one, not a random one... the last one.
See findOpenApiPathMatchingPossiblePathnames in https://github.com/openapi-library/OpenAPIValidators/blob/master/packages/openapi-validator/lib/utils/common.utils.ts

Why is it so? :)

OpenAPIValidators could, for instance, refuse to validate the response and report the ambiguity of the spec of the API: this would probably be the most unopinionated and sanest approach.

Let's get back to goal the the library (I'm quoting the readme):

These test plugins let you automatically test whether your server's behaviour and documentation match.

The library is not claiming to be an "OpenAPI specification file" validator (it wouldn't need to validate HTTP responses to serve this purpose anyway). It's meant to ease the validation of server behavior.

Since we both agree on the fact that the OpenAPI specification is not clear enough (and is openly leaving room for interpretation to implementations), and since the library is meant to help the validation of servers (that do have their own interpretation of the OpenAPI spec), it seems to me that the library should provide ways to mitigate those corner cases.

So I'm wondering: could OpenAPIValidators support different ambiguity resolution strategies? For the moment there is only one of them: last.
Could there be first and most-concrete for instance (like in my first PR)?

[rwalle61]

Thanks @bfreuden, sorry I've been too busy to reply recently. Sounds like we're not getting a strong official OpenAPI answer. Mike's reply to your Stack Overflow question suggests we should match most-concrete. But to adapt your example, given this spec

/pets/{petId}/fur/color
/pets/mine/{a}/{b}

A request /pets/mine/fur/color would match /pets/{petId}/fur/color, but I think some people would expect it to match /pets/mine/{a}/{b}.

I think your idea of same hierarchy may cover most cases intuitively. For example, given this spec:

POST /shops/{shop}/pets/_search
GET /shops/{shop}/pets/{pet}

Request POST /shops/1/pets/_search would match POST /shops/{shop}/pets/_search. And request GET /shops/1/pets/_search would match it too, throwing the 'Wrong method: GET' error

I still think that users needing different matching strategies (most-concrete and same-hierarchy) should probably design their API more simply. We could allow the user to configure different matching strategies for our validator, but it feels a lot of complexity for little gain.

But how about this - if anyone can list 3 popular OpenAPI APIs (e.g. Swagger PetStore, Elasticsearch) that our validator doesn't match correctly (with current resolution strategy last), then I'll accept we need to change our validator to whatever resolution strategy satisfies the APIs.

[bfreuden]

Hi @rwalle61
No worries, I've been very busy as well :)

Well... to be honest I've suggested the multiple matching strategies approach because you seem very concerned about regressions for existing OpenAPIValidators users.

I don't think it is necessary though: the same prefix strategy will perform precisely the same way as the current latest strategy for people having a "simple" API (I'm quoting "simple" because I'm referring to your "more simply" statement). And that's logical: the concrete vs abstract case mentioned in the spec is just a specific case of same prefix.

Don't get me wrong: I don't think the same prefix strategy is better (in terms of spec compliance) than latest, but I don't think the lastest strategy is better than same prefix neither. However, as suggested by Mike Ralphson's reply, I strongly believe that same prefix is more useful. On top of that it is not that complex (it's only 20 lines of code that are already in my second pull request :)).

Concerning the 3 popular APIs, I accept the challenge ! :)
Here they are:

Elasticsearch
Below I'm referring to spec files of this list:
https://github.com/elastic/elasticsearch/tree/master/rest-api-spec/src/main/resources/rest-api-spec/api

---------------------------------
/{index}/_rollup/{rollup_index}    in rollup.rollup.json
/{index}/_rollup/data              in rollup.get_rollup_index_caps.json
---------------------------------
/_snapshot/{repository}/{snapshot}     in snapshot.get.json
/_snapshot/{repository}/_cleanup       in snapshot.cleanup_repository.json
/_snapshot/{repository}/_analyze       in snapshot.repository_analyze.json
/_snapshot/{repository}/_verify        in snapshot.verify_repository.json
/_snapshot/{repository}/_status        in snapshot.status.json
---------------------------------
/_security/privilege/{application}/{name}         in security.get_privileges.json
/_security/privilege/{application}/_clear_cache   in security.clear_cached_privileges.json

Kairntech Sherpa
That's the company I'm working for, very famous machine learning API :)
https://sherpa.kairntech.com/swagger-ui/

/projects/{projectName}/documents/{docId}
/projects/{projectName}/documents/_search

As you can see our API is inpired from Elastic's.

Joke API
I'm quoting https://rapidapi.com/blog/most-popular-api/

Out of all 10,000+ APIs on RapidAPI’s API marketplace, these are the APIs that over 1,000,000 developers use the most.

Joke API is in the top 50 at position 35 :)

https://rapidapi.com/LemmoTresto/api/joke3/specs

/v1/joke/{id}/upvote
/v1/joke/{id}/{property}

To conclude on a more serious tone: you're the boss here and I accept that but I won't be able to argue more than I did.
If you think my second pull request is unacceptable, so be it: feel free reject it and I'll find another solution (I won't change my API though... because I can't: it is used in production and my customers are happy with it).
In any case, I firmly believe you should reject my first PR though.

Up to you :)

[graemeberry-esgglobal]

It looks like it's been some time since this has been looked at and I just wanted to see if it had been fixed or planned to be fixed?