deepObject passes validation when it's an array

[jan-bw]

I am not sure whether that's on purpose or whether that's a bug, but I've just spotted a (imo) surprising behaviour during deep object validation. Given schema definition like this:

paths:
  /foo:
    get:
      parameters:
        - name: filters
          in: query
          required: false
          style: deepObject
          explode: true
          schema:
            type: object
            properties:
              first:
                type: array
                items:
                  type: string
              second:
                type: array
                items:
                  type: string

When making a request with following params:

• filters=foo
• filters[]=1&filters[]=2
  the validation passed.

This surprised me because I'd expect it to fail as neither string nor array should be allowed, should they? Since in rails app you can then access the params[:filters] and it's a string or an array (respectively) it creates a risky precedence.

It's caused by the OpenapiParameters::Query#parse_deep_object where prop_key = key.match(prop_regx)&.[](1) returns nil because the regex won't match keys like filters nor filters[].

[ahx]

Hi. This is is sort of on purpose. It acts that way because "filters", "filters[first]" and "filters[]" are considered three different parameters. Your example describes effectively to parameters "filters[first]" and "filters[second]", but it does not describe "filters" nor "filters[]", so those are treated as unknown query parameters, which are (currently) generally allowed.

To avoid API drift, OpenapiFirst::Test raises an error if it sees an unknown query parameter during a request test.

I think this is the correct implementation of the spec, but I am open to discuss practicability.I think one clear way forward would be to fail validation if there are any unknown query parameters not just in tests, but in production as well, but I am not sure if we want this, so I am open for suggestions / opinions.

[jan-bw]

"filters", "filters[first]" and "filters[]" are considered three different parameters

I think that's the point I am not so sure and convinced about :) It seems to me that they are considered separate parameters in the openapi_first's implementation while they are not considered separate by the Openapi spec. In openapi spec to have them considered separate I'd have to define something like this:

paths:
  /foo:
    get:
      parameters:
        - name: filters
          in: query
          required: false
          schema:
            type: string
        - name: filters[]
          in: query
          required: false
          style: form
          explode: true
          schema:
            type: array
            items:
              type: string
        - name: filters[first][]
          in: query
          required: false
          style: array
          explode: true
          schema:
            type: array
            items:
              type: string

For reference, I've checked how the validation is handled by express-openapi-validator and it works as I described - it fails for both filters=foo and filters[]=bar with:

OpenAPI validation error:
  request/query/filters must be object
  - /query/filters: must be object

Having that said, it looks like it all gets sorted out in version 3.2 with introduction of the querystring so might not be worth touching anyways, especially if you think it's correct :)

fail validation if there are any unknown query parameters not just in tests, but in production as well,

I don't think that's safe :D

[ahx]

I don't think that's safe :D

I think I agree. Some colleagues were arguing with me about that, but nobody backed that up with an implementation. ;)

Thanks for the pointer to the querystring parameter location. I've added that to #469 ♥️

[ahx]

I think that's the point I am not so sure and convinced about :) It seems to me that they are considered separate parameters in the openapi_first's implementation while they are not considered separate by the Openapi spec.

I had some doubts about this as well, but I will stick to it until convinced otherwise =). I could not find a definitive answer to that in the OAS at the time so I decided for something that did not seem to contradict the OAS as I understood it and what does not add a lot of complexity to the implementation.

That being said, finding other implementations that went for a different implementation might be a strong pointer to change this (in a major version).