jhthorsen / mojolicious-plugin-openapi

OpenAPI / Swagger plugin for Mojolicious
53 stars 41 forks source link

Array parameters in schema don't handle single values #220

Closed briandfoy closed 2 years ago

briandfoy commented 2 years ago

This doesn't apply to the 4.x line, but it may be useful to anyone stuck on the on 3.x. When I get a chance to update to 4.x I'll see if I replicate the same behavior.

I think I've run into another path where arrays aren't promoted properly, as in #154.

I have a parameter list list like this, where all the form parameters are treated as an object with properties. This allows the schema to constrain various things about the properties, such as excluding properties not specified in the schema. That all works unless, as in #154, there's only one item for the array:

  parameters:
    - in: query
      name: values
      schema:
        type: object
        properties:
          flags:
            description: 'Some unused param in the code'
            type: array
            items:
              type: string
      style: form
      explode: true

I think this comes down to _coerce_object_by_style in JSON::Validator::OpenAPI::Mojolicious. If the style is form, it simply hashifies the query and immediately returns it. The param which should be an array is just a string.

I kludgy hot fixed this by expanding the if branch, and this takes care of the cases I care about.

if ($style eq 'form' && $explode) { my $hash = $c->req->url->query->to_hash;

if( exists $p->{schema} ) {
  my $properties = $p->{schema}{properties};
  foreach my $name ( keys %$hash ) {
    next if ref $hash->{$name};
    next unless exists $properties->{$name};
    next unless $properties->{$name}{type} eq 'array';
    $hash->{$name} = [ $hash->{$name} ];
  }
}

return $hash;

}

jhthorsen commented 2 years ago

How is this an issue if it does not apply to the 4.x versions?

briandfoy commented 2 years ago

I've had a chance to test this with the latest version, and it's still a problem in 4.x. I've made a minimal working example in briandfoy/openapi_test and shown by running perl -Ilib t/basic.t there.

jhthorsen commented 2 years ago

I'm unable to reproduce this. Could you please provide a test case or at least an URL that fails the validation?

briandfoy commented 2 years ago

I don't know what you did to try to reproduce this, but I just tried it again with 5.0 and it's still a problem. In the example repo I provided in my previous message, try the steps in the README or just run perl -Ilib t/basic.t. If that's what you did and everything passed, then there's something else for me to track down.

 $ perl -MMojolicious::Plugin::OpenAPI -le 'print Mojolicious::Plugin::OpenAPI->VERSION'
 5.00
 $ perl -MJSON::Validator -le 'print JSON::Validator->VERSION'
 5.02
 $ pwd /Users/brian/Desktop/openapi_test
 $ perl -Ilib t/basic.t
 # Mojolicious::Plugin::OpenAPI 5.00
 ok 1 - GET /search
 ok 2 - Missing q is bad request
 ok 3 - GET /search?q=abc
 ok 4 - 200 OK
 ok 5 - GET /search?q=abc
 ok 6 - 200 OK
 ok 7 - GET /search?q=abc&flags=123
 not ok 8 - Single flags succeeds
 #   Failed test 'Single flags succeeds'
 #   at t/basic.t line 15.
 #          got: '400'
 #     expected: '200'
 ok 9 - GET /search?q=abc&flags=123&flags=xyz
 ok 10 - Multiple flags succeeds
 1..10
 # Looks like you failed 1 test of 10.
jhthorsen commented 2 years ago

The repo you linked to is not a minimal working example. I have not counted the number of lines and files, but there's a lot 😞

Even https://github.com/jhthorsen/mojolicious-plugin-openapi/blob/master/t/v3-style-object.t can be argued being a very long test-file, but at least it's all in one file. Please provide a test case like that, or maybe even better: A patch to v3-style-object.t with your failing case.

briandfoy commented 2 years ago

Here you go:

use Mojo::Base -strict;
use Test::Mojo;
use Test::More;

use Mojolicious::Lite;

get '/pets' => sub {
  my $c = shift->openapi->valid_input or return;
  $c->render(openapi => $c->validation->output);
  },
  'getPets';

plugin OpenAPI => {url => 'data:///parameters.json'};

my $t = Test::Mojo->new;

# style: deepObject
$t->get_ok('/api/pets')->status_is(200);
$t->get_ok('/api/pets?flags=123&flags=abc')->status_is(200);
$t->get_ok('/api/pets?flags=xyz')->status_is(200);

print $t->tx->res->to_string, "\n";

done_testing;

__DATA__
@@ parameters.json
{
  "openapi": "3.0.0",
  "info": {
    "license": {
      "name": "MIT"
    },
    "title": "Swagger Petstore",
    "version": "1.0.0"
  },
  "servers": [
    {
      "url": "/api"
    }
  ],
  "paths": {
    "/pets": {
      "get": {
        "operationId": "getPets",
        "parameters": [
          {
            "name": "do",
            "in": "query",
            "style": "form",
            "explode": true,
            "schema": {
              "type": "object",
              "properties": {
                "flags": {
                  "type": "array",
                  "items": {
                    "type": "string"
                  }
                }
              }
            }
          }
        ],
        "responses": {
          "200": {
            "description": "pet response",
            "content": {
              "*/*": {
                "schema": {
                  "type": "object"
                }
              }
            }
          }
        }
      }
    }
  }
}

And, the output:

ok 1 - GET /api/pets
ok 2 - 200 OK
ok 3 - GET /api/pets?flags=123&flags=abc
ok 4 - 200 OK
ok 5 - GET /api/pets?flags=xyz
not ok 6 - 200 OK
#   Failed test '200 OK'
#   at test2.pl line 20.
#          got: '400'
#     expected: '200'
HTTP/1.1 400 Bad Request
Date: Wed, 03 Nov 2021 08:05:27 GMT
Server: Mojolicious (Perl)
Content-Length: 89
Content-Type: application/json;charset=UTF-8

{"errors":[{"message":"Expected array - got string.","path":"\/do\/flags"}],"status":400}
1..6
# Looks like you failed 1 test of 6.
jhthorsen commented 2 years ago

Thanks for the smaller example. It should be fixed in the upcoming release of JSON::Validator.