jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 58 forks source link

allOf changed behaviour somewhere after 2.05? #160

Closed skaurus closed 4 years ago

skaurus commented 5 years ago

I have a schema like this (important parts here are view path and view definition, the rest is provided for the sake of completeness):

swagger: '2.0'
basePath: "/rest/v1"
host: ...
externalDocs:
  url: ...
info:
  title: ...
  version: '1'
schemes:
- https
produces:
- application/json

paths:

  /view:
    post:
      operationId: v1_view
      x-mojo-to: rest_api-v1#view
      summary: register new view
      produces:
      - application/json
      parameters:
      - $ref: "#/parameters/apiKey"
      - name: body
        in: body
        required: true
        schema:
          $ref: "#/definitions/view"
      responses:
        '202':
          description: ''
          examples:
            application/json:
              status: ok
        '500':
          description: "Internal server error"
        '401':
          description: "Authorization failed"
        '400':
          description: "Bad Request"

parameters:

  apiKey:
    name: api_key
    in: query
    required: true
    type: string
    minLength: 6
    maxLength: 6

definitions:

  deviceInfo:
    type: object
    required:
    - deviceId
    properties:
      deviceId:
        type: string
      screenResolution:
        type: string
        pattern: "[0-9]+x[0-9]+"
      deviceType:
        type: string
      osVersion:
        type: string
      mobileOperator:
        type: string

  genericEvent:
    type: object
    required:
    - url
    - deviceInfo
    properties:
      deviceInfo:
        $ref: "#/definitions/deviceInfo"
      event:
        type: string
      ts:
        type: string
        pattern: "[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}\\.[0-9]{3}"
      url:
        type: string
        format: uri
      meta:
        type: object

  view:
    type: object
    allOf:
    - $ref: "#/definitions/genericEvent"
    - properties:
        event:
          enum:
          - view
          default: view
        rgid:
          type: string
      required:
      - rgid

And I have some tests. Tests try to use view path without rgid parameter and expect to get 400 status code. It was indeed so with Mojolicious 7.46, Plugin::OpenAPI 1.26 and JSON::Validator 2.05. After updating code to 8.17, 2.14 and 3.11 this test now gets status 202 and fails.

I'm probably missing something obvious here, could you help?

skaurus commented 5 years ago

I forgot to add that requests are validated with $self->openapi->validate, and while with the old versions this code returned some errors, with new versions I get an empty list.

skaurus commented 5 years ago

With Mojolicious 7.94, Plugin::OpenAPI 1.30 and JSON::Validator 2.18 test runs fine.

jhthorsen commented 5 years ago

Hello @skaurus.

I got some questions/comments:

  1. Where in the Swagger 2 spec does it say that you can use allOf? I'm actually pretty surprised that this goes through. I was expecting it to fail validation instead of allowing you to start the server.
  2. You write "test runs fine" - Where is the test? Providing a small test case would make it a lot easier for me to understand what's going on. Maybe you can write something that looks like https://github.com/jhthorsen/mojolicious-plugin-openapi/blob/master/t/path-parameters.t and replace your original description?
  3. I see you added "stuff" for completeness, but please provide the smallest version of spec to avoid distractions.
elcamlost commented 5 years ago

Hi! My 50 cents (cause I started to use allOf with swagger in the company).

  1. I found out of such option from this SO discussion. It mentioned here in the swagger official docs. There is also example in the swagger repo.
skaurus commented 5 years ago

Hello @jhthorsen,

I'm trying to make a test case. it turned out to be harder to replicate than I thought. Also, I don't have a lot of time for it, but I think I'll make it. Thanks for the test example!

jhthorsen commented 5 years ago

Thanks @elcamlost!

@skaurus: I'll leave it open for now, but hoping you won't be able to make a failing test, hehe

skaurus commented 5 years ago

I found the reason. We call $self->openapi->validate in under callback, so request isn't routed yet, so $c->stash('openapi.path') is empty, so local $op_spec->{parameters} in _helper_validate is empty.

The test case is coming a bit later.

skaurus commented 5 years ago

Here it is. What makes it work on older versions is that little hack with openapi.parameters. Is there more idiomatic way of checking errors in under callbacks, which is unlikely to break with updates?

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

use Mojolicious::Lite;

my $under = sub {
  my $c = shift;
  my $spec_parameters = $c->match->endpoint->pattern->defaults->{'openapi.parameters'};
  $c->stash('openapi.parameters' => $spec_parameters);

  my @errors = $c->openapi->validate;
  return @errors ? 0 : 1;
};

post '/foo' => sub {
  my $c = shift;

  $c->render(openapi => { id => $c->param('rgid') });
}, 'allOf';

plugin OpenAPI => {
  url     => "data://main/path-parameters-allof.yml",
  route   => app->routes->under('/api/allOf')->to('cb' => $under),
};

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

$t->post_ok('/api/allOf/foo' => json => { 'url' => 'https://google.com' })->status_is(400);

done_testing;

__DATA__
@@ path-parameters-allof.yml
swagger: '2.0'
basePath: "/api/allOf"
info:
  title: allOf behaviour
  version: '1'

paths:

  /foo:
    post:
      operationId: allOf
      produces:
      - application/json
      parameters:
      - name: body
        in: body
        required: true
        schema:
          $ref: "#/definitions/view"
      responses:
        '200':
          description: ''
        '500':
          description: "Internal server error"
        '401':
          description: "Authorization failed"
        '400':
          description: "Bad Request"

definitions:

  genericEvent:
    type: object
    required:
    - url
    properties:
      url:
        type: string
        format: uri

  view:
    type: object
    allOf:
    - $ref: "#/definitions/genericEvent"
    - properties:
        rgid:
          type: string
      required:
      - rgid
skaurus commented 5 years ago

Hack like this can make this work in new versions:

my $basePath = quotemeta($c->openapi->spec('/basePath'));
my $path = $c->match->endpoint->to_string;
$path =~ s/^$basePath//;
local $c->stash->{'openapi.path'} = $path;

Funnily enough, $c->openapi and $c->match->endpoint do work with not-quite-routed-yet request

jhthorsen commented 4 years ago

I don't think this is a JSON::Validator issue anymore.

Please open a new (cleaned up) issue in https://github.com/jhthorsen/mojolicious-plugin-openapi/issues/.