Skip to content

PathFinder finds all patterns, who looks like my request path, and he return worst case #226

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

Closed
mrkovalchuk opened this issue Mar 23, 2020 · 8 comments · Fixed by #245
Closed

Comments

@mrkovalchuk
Copy link

mrkovalchuk commented Mar 23, 2020

Good day. Please start from comment in code:
dcb7161#r37991936

In my case PathFinder find three path patterns (with all him staff like a response and more...):
path_pattern (from request): api/some/reourse/{key}/this/path/
Matched patterns with important arg:

  1. api/some/reourse/{key}/ mimetype: json
  2. api/some/reourse/{key}/this/ mimetype: json
  3. api/some/reourse/{key}/this/path/ mimetype: text/csv

Ok. Could be worse. We can see, number three - best match. But code from link say: I'll return first of them. Why? Why we use iterators everywhere, but still return one response for validation? Not the most accurate pattern. Just first. Please look like it be in 0.13.2 version. Radical changes. And I don't understand new logic =(

This behaviour produce error in schema validation process. Schema right. I checked it.

Thanks for help.

@p1c2u
Copy link
Collaborator

p1c2u commented Mar 23, 2020

Hi @mrkovalchuk

thanks for reporting the issue. It should find exactly one path. Please check #220 .

Fix #222 is already merged to master.

@mrkovalchuk
Copy link
Author

@p1c2u Thanks for answer!

Problem isn't solved =(

image

Still three patterns.

playpauseandstop added a commit to playpauseandstop/rororo that referenced this issue Apr 13, 2020
To ensure proper logic behind matching current request to OpenAPI operation
to use.

`openapi-core==0.13.3` version ships with `openapi_core.templating.paths.finders.PathFinder`
class, which finds too many paths if OpenAPI schema contains multiple paths with
variables.

Given commit customize path finder logic to not return first result of
`PathFinder._get_servers_iter(...)` call, but to match that current request
full URL pattern ends with `Path.name`.

After fixing python-openapi/openapi-core#226, this code might be
not needed anymore.

Also provide docstrings for reason behind providing custom classes for validation,
unmarshalling data, etc.
@playpauseandstop
Copy link

Hi,

I ran into similar issue, when upgrading openapi-core to 0.13.3 for my projects.

First, I attempted to change OpenAPI Schema files to put paths with variables on the top, but unfortunately it didn't help much, as openapi-core==0.13.3 still matches way too many paths and I cannot ensure that request (and response) will be validated against necessary operation.

This leads me to use custom PathFinder class and define next logic in find method,

        servers_iter: Iterator[PathTuple] = self._get_servers_iter(
            request.full_url_pattern, operations_iter_peek
        )
        for server in servers_iter:
            path = server[0]
            if request.full_url_pattern.endswith(path.name):
                return server

While base PathFinder tries to return first servers_iter match instead,

        servers_iter = self._get_servers_iter(
            request.full_url_pattern, operations_iter_peek)

        try:
            return next(servers_iter)
        except StopIteration:
            raise ServerNotFound(request.full_url_pattern)

My OpenAPI schema contains next paths,

  • /repositories
  • /repositories/{owner}
  • /repositories/{owner}/env
  • /repositories/{owner}/{name}
  • /repositories/{owner}/{name}/env

And change above allows BaseValidator to pick proper OpenAPI path tuple for every request.

Hope, this is helpful

@stephenfin
Copy link
Contributor

stephenfin commented Apr 16, 2020

I've seen this same issue, but the above solution doesn't work when validating Django responses. I ended up monkey patching the search function in openapi_core.templating.util like so:

# ...
from openapi_core.templating import util

# HACK! Workaround for https://github.com/p1c2u/openapi-core/issues/226
def search(path_pattern, full_url_pattern):
    p = util.Parser(path_pattern)
    p._expression = p._expression + '$'
    result = p.search(full_url_pattern)
    if not result or any('/' in arg for arg in result.named.values()):
        return None

    return result


util.search = search

# ... rest of code

I suspect this could be done in a far less hacky fashion if our pattern used for lookup included the type information for any parameters, as extracted from the schema. I looked into doing this with parse.Parser but I wasn't able to do so easily. I might have more luck with re but I just haven't gotten to trying this yet.

stephenfin added a commit to getpatchwork/patchwork that referenced this issue Apr 18, 2020
We've done the necessary work here already so this is a relatively easy
switchover. However, we do have to work around an issue whereby the
first possible matching route is used rather than the best one [1]. In
addition, we have to install from master since there are fixes missing
from the latest release, 0.13.3. Hopefully both issues will be resolved
in a future release.

[1] python-openapi/openapi-core#226

Signed-off-by: Stephen Finucane <[email protected]>
@stojan-jovic
Copy link

stojan-jovic commented Apr 24, 2020

I think that I have same issue like described here (with version 0.13.3), so I skipped opening new issue.

Test swagger file:

paths:
  /v1/test/{test_param}:
    get:
      tags:
        - Test
      summary: Test
      description: Test
      parameters:
        - name: test_param
          in: path
          required: true
          description: Test path parameter
          schema:
            type: string
            enum:
              - test1
              - test2
      responses:
        200:
          description: Successful
  /v1/test/{test_param}/{test_param_2}:
    get:
      tags:
        - Test
      summary: Test
      description: Test
      parameters:
        - name: test_param
          in: path
          required: true
          description: Test path parameter
          schema:
            type: string
            enum:
              - test3
              - test4
        - name: test_param_2
          in: path
          required: true
          description: Test path parameter
          schema:
            type: string
            enum:
              - test5
              - test6
      responses:
        200:
          description: Successful

By making call to the second endpoint:

$ curl http://localhost:8000/v1/test/test3/test5

Request being validated against first path (i.e. /v1/test/{test_param}):

Value test3/test5 not valid for schema of type SchemaType.STRING: (<ValidationError: \"'test3/test5' is not one of ['test1', 'test2']\">,)

Simple workaround would be to change places of conflicted paths in swagger file, but I think that that will not work always (in above simple example it works).

@p1c2u Does #222 solves this? If not, I'm willing to contribute with one PR to solve issue, but not sure are you (or anybody else) already on this?

@p1c2u
Copy link
Collaborator

p1c2u commented Apr 25, 2020

@stojan-jovic it should be fixed by #245 . Feel free to test it.

@stojan-jovic
Copy link

@p1c2u I can confirm that fix working fine! I verified it pretty much time ago, but totally forgot to respond here.

When we can expect new release? Can I somehow help with that? This is really important fix...

@timabbott
Copy link

Yeah, I'd also love to see this in a release; one has to do weird gymnastics in the OpenAPI configuration file to work around this bug.

timabbott pushed a commit to zulip/zulip that referenced this issue Jul 14, 2020
…egex.

openapi-core, the request validator has a bug due to which data type
of path parameters is not checked. Hence `/messages/{message_id}`
can match with `messages/matches_narrow`. So change the position of
of `message/{message_id}` after all such possible matches to avoid
errors.

See python-openapi/openapi-core#226 for details.
timabbott pushed a commit to zulip/zulip that referenced this issue Jul 14, 2020
openapi-core, the request validator has a bug due to which data type
of path parameters is not checked. Hence `/users/{user_id}` can match
with `users/me`. So change the position of`/user/{user_id}` after all
such possible matches to avoid errors.

See python-openapi/openapi-core#226 for details.
stephenfin added a commit to getpatchwork/patchwork that referenced this issue Dec 13, 2020
In commit b7f3c3d ("tests: Switch to openapi-core 0.13.x") we added
support for 'openapi-core' 0.13.x. However, we needed to use a commit
from master since an important fix [1] was not included in the latest
release at the time, 0.13.3. 0.13.4 has since been released so let's
move on and use that.

[1] python-openapi/openapi-core#226

Signed-off-by: Stephen Finucane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants