Path matching based on regex

[jandouwebeekman]

It is possible to have endpoints where the path parameters determine which specific endpoint is called, for example:

     get(/\/items\/(\d+)/) do |id|
        content_type :json
        JSON.generate(items[id.to_i])
      end

     get(/\/items\/([^\d]+)/) do |name|
        content_type :json
        JSON.generate(items_map[name])
      end

Currently OpenapiFirst cannot handle these weird cases as every path parameter is replaced with %r{([^/?#]+)} in lib/openapi_first/router/path_template.rb meaning that calls to both endpoints will be validated against the same endpoint form the openapi definition.

I suggest (and have already implemented in a fork) that you have an optional setting which uses the regex provided with the pattern of a path parameter to match paths.
This needs to be optional since people might use these patterns to validate the user input.

[ahx]

Hey @jandouwebeekman, thanks for the issue and the interest in this gem.

I don't understand the use case. Can you share a snipped of an OAD (OpenAPI API description) that uses parameter patterns to point to different operations?

It is possible to have endpoints where the path parameters determine which specific endpoint is called […]
[…] calls to both endpoints will be validated against the same endpoint form the openapi definition.

This sounds like something that at least is not supported by OpenAPI 3.0 itself right now, because the path templating is rather limited. I've read about an idea for OpenAPI 4.0 some months where it should be possible to have the same path definition (like /stuff/{name}) being routed to different operations based on matching parameters. Your case sounds like something related to that

That aside, I think I get your use-case right now. 🤔 … But can you show a little example OAD, just to be sure?

[ahx]

This needs to be optional since people might use these patterns to validate the user input.

The path parmeter pattern is always used to validate the path parameter. So what do you mean by "might"? Sorry if I act clueless.

[jandouwebeekman]

first of all:
our use case is definitely not supported by the OpenAPI standard because we allow empty path parameters like this:

paths:
    /foo{format}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^(.(json|xml))?$
            operationId: foo
    /foobar{format}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^(.(json|xml))?$
            operationId: foobar

but in the example I gave it is indeed about the case which you say might be supported in OpenAPI 4.0
so if i would write the specification for the example it would be

paths:
    /items/{id}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^\d+$
            operationId: get_items_id
    /items/{name}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^[^\d]+$
            operationId: get_items_name

here if you get a request for /items/some_item, this library will validate it against the get_items_id specification
then it will check if the paramaters match the pattern which fails and thus you might throw an error (if you have that enabled of course)
however, it should just be able to be validated get_items/name

with regards to your second comment:
if my suggestion is not optional and you have the specification:

paths:
    /items/{id}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^\d+$
            operationId: get_items_id

and have a request for /items/some_item, it will not match with any path and will be seen as an unknown path instead of a failing path parameter pattern validation

does this make it more clear what I mean?

[ahx]

Thanks for the explanation.
So your OAD (or your already existing API which you want to define an OAD for?) looks like this:

paths:
    /items/{id}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^\d+$
            operationId: get_items_id
    /items/{name}
        get:
            parameters:
                -  in: path
                    name: format
                    required: true
                    schema:
                      type: string
                      pattern: ^[^\d]+$
            operationId: get_items_name

This is not supported by OpenAPI 3, but you want to make it work by augmenting the routing logic in openapi_first.

Can you solve this by using a schema with a oneOf like this instead? This should validate that one of the patterns must match, but should still support your existing (Sinatra?) code.

/items/{id}
    get:
        parameters:
            -  in: path
                name: id
                required: true
                schema:
                  oneOf:
                    - type: string
                      pattern: ^\d+$
                    - type: string
                      pattern: ^[^\d]+$
        operationId: get_items

If that is an option for you I would like to keep the routing as is or make it work if you find an issue with this approach.

[jandouwebeekman]

no, the {format} example is what our OAD looks like

I was just looking for an alternative example which I could use as an argument for the PR as our OAD just isn't supported by OpenAPI 3 (as it is an optional path parameter which id not directly after a / )

but the oneOF would indeed be a solution in the other example
(even though things like a description or even the responses would be unintuitive for end users)

[ahx]

Hi. I think in general a path like /foo{format} should be allowed in OpenAPI. But this currently cannot be handled by openapi_first. I have added a failing test to show the issue. I have no idea how to make that pass right now. This would probably require changing the routing logic quite a bit.

If we would solve that, would this plus using oneOf in the parameter schema solve your use case?
I would like to stick with what OpenAPI is supporting right now or at least does not forbid to do.

[jandouwebeekman]

I have no idea how to make that pass right now. This would probably require changing the routing logic quite a bit.

I am not sure if this is alllowed by OpenAPI, but a solution could be to change lib/openapi_first/router.rb:

    def find_path_item(request_path)
      found = @static[request_path]
      return [found, {}] if found

      @dynamic.find do |_path, path_item|
        params = path_item[:template].match(request_path)
        return [path_item, params] if params
      end
    end

to not return the first value but to get a list of all the matches and then return the match that has the least amount of characters to be interpreted as a path parameter, so something like:

def find_path_item(request_path)
      found = @static[request_path]
      return [found, {}] if found

      matches = @dynamic.filter_map do |_path, path_item|
        params = path_item[:template].match(request_path)
        next unless params

        [path_item, params, params.values.sum(&:length)]
      end

      matches&.min_by { |_path_item, _params, length| length }
             &.then { |path_item, params, _length| [path_item, params] }
    end

But again: I'm not sure if this solution actually makes sense or if it is a weird hack for something that is not supported by OpenAPI

Btw, this solution still behaves weird if you have the path "/{id}{format}" since it would match "/123.json" to id="123.jso" an format="n", but i guess it is an improvement?

[ahx]

But again: I'm not sure if this solution actually makes sense or if it is a weird hack for something that is not supported by OpenAPI

Linters, like redocly for example are complaining about ambigous paths like /{some}{id} and /{foo}{id}:

Paths should resolve unambiguously. Found two ambiguous paths: `/{some}/{id}` and `/{foo}/{id}`.

But paths like /some{format} and /something{format} are fine.

Btw, this solution still behaves weird if you have the path "/{id}{format}" since it would match "/123.json" to id="123.jso" an format="n", but i guess it is an improvement?

Agreed. The router would say 🆗, but the request validation would probably complain about an unknown format.

About the code example: I think other solutions would consist of a trie like datastructure, but so far I did not get my head around that. I would be open to merge a PR with a simple solution like you have outlined to make that failing test pass. I think that this is a valid use-case and it is supported by OpenAPI.

[jandouwebeekman]

Linters, like redocly for example are complaining about ambigous paths like /{some}{id} and /{foo}{id}:

Haha, right, i completely forgot that I turned that warning off

I think other solutions would consist of a trie like datastructure

Yeah, such a solution would mean a complete overhaul of the routing logic, but I'm not sure if it is a complete solution since you might have trouble with paths like /{format}thing and /{format}something so I think it will be very difficult to find an elegant solution for these weird cases.

But since I forgot the no-ambiguous-paths rule it means that for our code we need to use a monkey patch anyway so for me it doesn't really matter if my last code example gets into this repository.
I do think it is an improvement but I also agree it is not the best solution so I'm fine with opening a PR for that code.
Would you like that or do you think it is better to keep it as it is until a better solution is found?

[ahx]

Would you like that or do you think it is better to keep it as it is until a better solution is found?

I would totally appreciate a solution for this and I don't think the outlined one is a bad solution. So if you want to work on this, that would super cool! 👍🏼

Notes:
Related to #155

[jandouwebeekman]

I have opened #389 for this solution