openapi_contracts icon indicating copy to clipboard operation
openapi_contracts copied to clipboard

Validation for query parameters

Open SalvatoreT opened this issue 1 year ago • 6 comments

I'd like to have my OpenAPI contracts check the query params, similar to how they now check the request body.

Example

To build on the /pets path from the fixtures, I'd imagine a spec that lets you specify what type of pet you'd like and whether or not they're a good boi (even though we know they are).

Spec

paths:
  /pets:
    get:
      operationId: pets
      summary: Pets
      parameters:
        - description: type of animal to return
          in: query
          name: type
          required: true
          schema:
            type: string
            enum: [cat, dog]
        - description: Is good boi?
          in: query
          name: good_boi
          schema:
            type: boolean
      responses:
        '200':
          description: Ok
          content:
            application/json:
              schema:
                type: array
                items:
                  oneOf:
                    - $ref: 'components/schemas/Polymorphism.yaml#/Pet'

HTTP Call

Good!

GET /pets?type=dog&good_boi=true

Bad.

GET /pets?type=bird&good_boi=never

SalvatoreT avatar Apr 03 '24 22:04 SalvatoreT

Hey @SalvatoreT, this is indeed not verified yet and it makes sense to implement it. I will see if I find time this week.

mkon avatar Apr 15 '24 06:04 mkon

Thank you!

SalvatoreT avatar Apr 15 '24 13:04 SalvatoreT

hey @SalvatoreT, do you want to test the gem from the branch validate-query-params to see if it covers your needs?

Parameter validation needs to be enabled with an option, for example

it { is_expected.to match_openapi_doc(doc, parameters: true) }

mkon avatar Apr 24 '24 18:04 mkon

Hey @mkon, I'm working with @SalvatoreT on this, so I tested it out. First of all, thank you for your work! The gem has been very helpful for us and we appreciate the update :)

For the most part, this works great. I noticed an issue with non-string query params, however. Here's some query params that we have defined in our OpenAPI spec (v3.1.0):

in: query
name: page
style: deepObject
schema:
  type: object
  properties:
    after:
      description: Object ID for pagination cursor
      type: string
      examples:
        - obj_ABC123
    before:
      description: Object ID for pagination cursor
      examples:
        - obj_ABC123
      type: string
    size:
      description: Limit on the number of objects returned
      examples:
        - 5
      type: integer

For a spec that makes this request: GET /api/v1/object?page[size]=101 We receive this failure: {"size"=>"101"} is not a valid value for the query parameter "page"

Clearly, the problem is the value of size is getting encoded as a string in the URI and deserialized as a string as well. I did some looking and I think the way we have this defined in the OAS is correct and tooling should be able to validate non-string query params (for example).

samanthawritescode avatar Apr 24 '24 22:04 samanthawritescode

Hey, thanks for trying this out. Yes indeed the issue is that query parameters always deserialise to a string which will make them invalid when comparing against the schema definition of integer. I will have to add some middle step that attempts some kind of casting based on the schema. Might take a couple of weeks time since I am soon out traveling.

mkon avatar Apr 25 '24 14:04 mkon

No worries, I hope you enjoy your travels!

samanthawritescode avatar Apr 25 '24 18:04 samanthawritescode

Hey @samanthawritescode and @SalvatoreT, sorry for the long delay. I updated the branch with a fix that utilizes ahx/openapi_parameters to cast the query parameters before validating them. Does this solve it for you?

mkon avatar Jun 13 '24 11:06 mkon

Hey @mkon no worries, thanks for getting back to it! I just tested it and it looks great :)

I did find one "issue", but it might be a better fit for ahx/openapi_parameters. Here's the stack trace and repro steps. I say "issue" in quotes because in this case I am purposefully passing in an invalid value for the param to test the error case, so it's not necessary to do contract testing with parameters: true in this case. That said, better error handling would probably be nice.

Repro with these params in the spec:

{
  filter: 'badfilter'
}

OpenAPI:

- in: query
  name: filter
  style: deepObject
  schema:
    additionalProperties: false
    type: object
    properties:
      note:
        type: string
      created-at-start:
        format: date-time
        type: string
      created-at-end:
        format: date-time
        type: string 

Stack trace:

 Failure/Error: expect(response).to match_openapi_doc(openapi_doc, parameters: true, path: '/inquiries')

 NoMethodError:
   undefined method `each_with_object' for "badfilter":String

           object.each_with_object({}) do |(key, value), hsh|
                 ^^^^^^^^^^^^^^^^^
 # /usr/local/bundle/gems/openapi_parameters-0.3.3/lib/openapi_parameters/converter.rb:47:in `convert_object'
 # /usr/local/bundle/gems/openapi_parameters-0.3.3/lib/openapi_parameters/converter.rb:34:in `convert'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/doc/parameter.rb:22:in `matches?'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/validators/parameters.rb:12:in `block in validate'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/validators/parameters.rb:9:in `each'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/validators/parameters.rb:9:in `validate'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/validators/base.rb:15:in `call'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/validators/base.rb:20:in `call'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/match.rb:24:in `valid?'
 # /usr/local/bundle/bundler/gems/openapi_contracts-4bbd0590844c/lib/openapi_contracts/rspec.rb:10:in `block (2 levels) in <main>'

samanthawritescode avatar Jun 13 '24 23:06 samanthawritescode

@samanthawritescode thanks for finding this edge case. I am now rescuing the error when conversion through ahx/openapi_parameters fails, it should lead to an error message that makes sense.

Maybe openapi_parameters should return the original value if it cannot be casted due to incompatible types.

mkon avatar Jun 14 '24 07:06 mkon

I confirmed that it errors much more nicely in this case now. Thanks again, I will get this implemented on our end as soon as it's merged :)

samanthawritescode avatar Jun 17 '24 23:06 samanthawritescode

Maybe openapi_parameters should return the original value if it cannot be casted due to incompatible types.

This is solved with v0.3.4

ahx avatar Jun 18 '24 07:06 ahx

Released v0.14.0

mkon avatar Jun 25 '24 07:06 mkon