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

Lack of support for nested routes of the same resource #2 #164

Closed
mojito317 opened this issue Sep 18, 2020 · 9 comments
Closed

Lack of support for nested routes of the same resource #2 #164

mojito317 opened this issue Sep 18, 2020 · 9 comments
Assignees

Comments

@mojito317
Copy link

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

OpenAPI version
3

Describe the bug
Lack of support for routes like:

/:id1/:id2/:id3
/resource/:id/resource

To Reproduce
Prepare spec for ie:

/:id1/:id2/:id3
/resource/:id/new

and try to satisfyApiSpec against /resource/:id/new route.

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

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

Additional context
This issue is almost the same as #57. I commented with my problem under that thread too.

@mojito317 mojito317 added the bug Something isn't working label Sep 18, 2020
@JonnySpruce JonnySpruce self-assigned this Sep 18, 2020
@JonnySpruce
Copy link
Collaborator

Thanks for raising this issue. @rwalle61 and I have been able to reproduce it and it's definitely something we need to discuss the expected behaviour for, since both paths technically do match.

Looking at the relevant part of the OpenAPI3 spec, it's not clear whether your example would fall into the ambiguous category or whether because /resource/:id/new has more concrete segments it should be preferred over the /:id1/:id2/:id3 path. I'm assuming this is the behaviour you expected?

Over the next few days I will do some deeper research into what should happen in situations like your example, and share my findings here.

If we do decide that this is a bug, we would need to implement a ranking system for each path that matches to decide which is preferred. (Additionally, it would be easy to extend this to warn if two paths appear to be ambiguous e.g. same number of concrete segments).

@JonnySpruce
Copy link
Collaborator

JonnySpruce commented Sep 21, 2020

To update, from my research I found nothing concrete. Aside from a comment from a collaborator which hints at the preferred ordering being whatever path contains more concrete segments, there seems to be no real reference to the correct way to order paths like in your example.

So to get a better answer, I've raised this issue in the OpenAPI Specification repo. Hopefully they can give us the answers we need so that we know how to handle cases like this in the future.

@mojito317
Copy link
Author

mojito317 commented Sep 22, 2020

Looking at the relevant part of the OpenAPI3 spec, it's not clear whether your example would fall into the ambiguous category or whether because /resource/:id/new has more concrete segments it should be preferred over the /:id1/:id2/:id3 path. I'm assuming this is the behaviour you expected?

Answering the question: I am trying to validate a path with more concrete segments that should be preferred over the /:id1/:id2/:id3 path like in CouchDB (which is not following strictly the OpenAPI guidance, unfortunately): /_node/{node-name}/_stats (link) over /{db}/{docid}/{attname} (link)

Edit: And thanks @JonnySpruce for opening the issue in the OpenAPI specification repo.

@rwalle61
Copy link
Collaborator

rwalle61 commented Sep 25, 2020

Thanks for clarifying, @mojito317 . Just to confirm, when you say that using these CouchDB paths

is not following strictly the OpenAPI guidance, unfortunately

is that according to some official OpenAPI guidance, or your (reasonable) assumption?

If the former, I guess we couldn't support this use case until it is compatible with OpenAPI. If the latter, hopefully the official OpenAPI people will clarify for us :)

@mojito317
Copy link
Author

is that according to some official OpenAPI guidance, or your (reasonable) assumption?

It is only my assumption, sorry if I made some confusion in the heads.

@rwalle61
Copy link
Collaborator

rwalle61 commented Dec 14, 2020

Unfortunately, the OpenAPI folks haven't answered this. I guess it's a pretty rare edge case.

Until we hear from them, I think this is a case of ambiguous matching, [and] it's up to the tooling to decide which one to use.

It would be easiest for this tool not to commit one way or another. @mojito317 do you definitely need our matching to stay consistent?

For reference:

image

@mojito317
Copy link
Author

Hi @rwalle61! Thanks for the feedback!

Unfortunately, I could not use the validator for my whole descriptor. I used a workaround that commented out the parts that were in conflict to check the descriptors are valid for all the endpoints manually. I could not put it in our pipeline. But since I had to manually validate the endpoints it is not necessary for me right now, but I can imagine we are going to have a similar validation at automation. If that will be the case I think we cannot use this validator in this form.

@rwalle61
Copy link
Collaborator

rwalle61 commented Mar 3, 2021

The OpenApi folks haven't got back to us on this, so I guess it's not one of their priorities. Unfortunately I think this plugin should avoid committing to unofficial behaviour, unless lots of users request it.

Sorry @mojito317 we can't currently support your use case, but thanks for your interest and bringing this to our attention!

@GershyRbi
Copy link

GershyRbi commented May 16, 2023

I'm encountering this same issue when using toSatisfyApiSpec. I have 2 suggestions:

  • Consider allowing the path to be explicitly provided:
const res = await requestNewResource();
expect(res).toSatisfyApiSpec({ specificPath: '/resource/:id/new' });
  • Generate a failure only if no endpoint matches, as opposed to the current behaviour, where "the first endpoint stumbled across" determines failure (i.e. if /:id1/:id2/:id3 is "stumbled across" first and fails, keep checking for more matching paths, resulting in finding /resource/:id/new).

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

4 participants