jhthorsen / mojolicious-plugin-openapi

OpenAPI / Swagger plugin for Mojolicious
54 stars 44 forks source link

Openapi v3: As I user, I programmer like a more DWIM "Accept" header negotiation #104

Closed plk closed 5 years ago

plk commented 5 years ago

I just converted a v2 spec to v3 which validates in the latest Swagger Editor without errors. I enabled V3 parsing, installed YAML::XS and am on the version 2.11. Server started fine, no errors. Every request fails with 404 and:

No responses rules defined for type application\/xhtml+xml text\/html application\/xml *\/*.","path":"\/"

Before I delve into this, is there anything general you know of I need to change in code for v3?

NGEfreak commented 5 years ago

I have a similar problem with content negotiation. Currently, a client has to set the full mime type like application/json in the Accept header. It doesn't work if he uses wildcards like */* or application/*. Also it doesn't work when the Accept header isn't set at all.

Here is a test case:

#!/usr/bin/env perl

use 5.010;
use strict;
use warnings;
use utf8;

use Test::More;
use Test::Mojo;

use Mojolicious::Lite;

get '/test' => sub {
    shift->render( openapi => { foo => 'bar' }, status => 200 );
  },
  'test';

plugin 'OpenAPI' => {
    url    => 'data:///api.yml',
    schema => 'v3',
};

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

# Client accepts only application/json
$t->get_ok( '/test', { Accept => 'application/json' }, json => { foo => 'test' } )
    ->status_is(200)
    ->json_is( { foo => 'bar' } );

# Client accepts everything implicitly meaning Accept header isn't set or empty
$t->get_ok( '/test', json => { foo => 'test' } )
  ->status_is(200)
  ->json_is( { foo => 'bar' } );

# Client accepts everything explicitly
$t->get_ok( '/test', { Accept => '*/*' }, json => { foo => 'test' } )
  ->status_is(200)
  ->json_is( { foo => 'bar' } );

# Client accepts application/* explicitly
$t->get_ok( '/test', { Accept => 'application/*' }, json => { foo => 'test' } )
  ->status_is(200)
  ->json_is( { foo => 'bar' } );

done_testing;

__DATA__

@@ api.yml
openapi: 3.0.0
info:
  title: Test
  version: 0.0.0
paths:
  /test:
    get:
      x-mojo-name: test
      responses:
        '200':
          description: Test
          content:
            application/json:
              schema:
                properties:
                  foo:
                    type: string

The first call works as expected. The other three produce these errors:

jhthorsen commented 5 years ago

@plk: You need to simplify your setup, since your error reports are invalid or lacking in details. I don't believe you're getting 404 and you provide me no way to reproduce your error.

@NGEfreak: I think you've clarified my confusion with @plk, but I have no idea how to fix this. Can you point me to where in the spec where I can find mentioning of how to interpret "/" or "application/*"? or even where a default response type is specified? The "/paths/.../responses/.../content" part of the spec is an object (hash-ref), so I can't really prioritise any of the mime types there. I guess I can cheat if the object contains only one matching type, but I'm not sure if that's correct.

plk commented 5 years ago

I can try to get a simple setup example but I just wanted to see if there was anything you knew of with V3 that was a general change people moving from V2 needed to do.

jhthorsen commented 5 years ago

@plk: You need to specify a "Accept" header (response type) that is defined in your schema.

NGEfreak commented 5 years ago

I couldn't find any mentioning of how to handle this kind of content negotiation in the spec. However, I think the current behavior should be changed.

Currently, the check is run during the call of $c->render( openapi => ... ), i.e. most likely at the end of a controller. The main code of the controller probably run successfully up to this point and perhaps even changed something on the system. Thus, this kind of check that involves a parameter of the client should be moved to $c->openapi->valid_input so it is run at the beginning of a controller. An invalid request of a client should fail as soon as possible to prevent the execution of the main controller code.

Better: the check in $c->render( openapi => ... ) should depend on the renderer and not on the Accept header. If there is a mismatch between what my renderer is able to output and what is defined in the OpenAPI schema then it should fail with a 500 error.

Another option is probably needed to define this:

$app->plugin(
  OpenAPI => {
    renderer => sub {
      my ($c, $data) = @_;
      return Mojo::JSON::encode_json($data);
    },
    renderer_content_types => [ 'application/json' ],
  }
);

Something like this should fix the immediate problem.

However, if you really want to support the whole deal of content negotiation then you have to implement something like in HTTP::Accept which parses the Accept header and provides a prioritized list of the accepted content types. Then you can filter this list against the content types from the schema. If there is no match you can return a 406 error. Keep in mind that wildcards like */* are possible in the Accept header and also in the OpenAPI schema. I guess it's also a good idea to provide a new helper which returns the filtered list so it can be used in the controller or renderer to determine what the client wants.

jhthorsen commented 5 years ago
  1. Looks like if you want to have a spec for "*/*" then you need to define that - https://swagger.io/docs/specification/media-types/
  2. It is a very good idea to perform the Accept header check in $c->openapi->valid_input.
  3. I don't think I want to run any checks on the output from the renderer() function, since that is just a octets that is sent to the client. I think you should test that yourself.
  4. I don't understand what renderer_content_types solves.
  5. I am going to support content negotiation. I started on it earlier today, but $work came in the way.
  6. I think you misunderstand render(openapi => ...). That is just a data-structure, so you don't need to understand the negotiated value inside your controller - you only need it inside renderer(), and I think you can use https://mojolicious.org/perldoc/Mojolicious/Plugin/DefaultHelpers#accepts to figure out what to render in that callback.
NGEfreak commented 5 years ago

Suppose I write a renderer that is able to convert my $data from render(openapi => $data) to JSON, XML and text. I would write something like this, right?

$app->plugin(
  OpenAPI => {
    renderer => sub {
      my ($c, $data) = @_;
      if ( $c->accepts('json') ) {
        return Mojo::JSON::encode_json($data);
      } elsif ( $c->accepts('xml') ) {
        return encode_xml($data);
      } else {
        return encode_text($data);
      }
    },
  }
);

Then I can also advertise in my OpenAPI schema that I support JSON, XML and text.

But how does the plugin know that I support these? It could get this information from the schema, but this seems unreliable when OpenAPI supports media types with wildcards like */*.

I don't understand why OpenAPI even allows wildcards. I doubt there is a renderer that is able to provide every format.

So, basically there are three lists involved:

There could be a mismatch between each list. Two checks are needed:

Examples:

Client: */* (which implies application/json) Schema: application/json Renderer: application/json

=> No error. This is probably one of the most common cases.

Client: application/json Schema: application/json Renderer: application/json

=> No error. Another common case.

Client: application/xml Schema: application/json Renderer: application/json

=> Client error 406: Client requested something that I'm not able to provide.

Examples: Client: application/xml Schema: */* (which implies application/xml) Renderer: application/json

=> Server error 500: I advertised something that I'm not really able to provide.

martynd commented 5 years ago

Ran into this one today when switching up to OpenAPI 3.0.0 (thank you for the support!)

@NGEfreak Realistically the Accept header is purely an HTTP protocol level item which just happens to be used by OpenAPI. While I agree that it is unlikely there is a renderer which can handle all types and unlikely a client which effectively can either, with Accept being part of the higher level protocol it should be trusted implicitly regardless.

With that said, I do agree with your examples with regards to intended behavior.

@jhthorsen @NGEfreak OpenAPI is designed with any arbitrary scenario in mind so while */* might seem unrealistic, something like image/* could well not be. Likewise, text/* would be renderable on any client and could be used without it being unrealistically overreaching. The reverse such as */json could be used to handle inconsistent uses of application/json and text/json.

Use of Accept wildcards in OpenAPI would be used for routing multiple types to a single action and the Content-Type of the response would be validated against that to make sure it falls within the bounds of the wildcard. As long as it does, any response Content-Type could be used.

*/* shouldnt be needed in the OpenAPI spec to match a clients */* entry, if the client has */* it should match against the first type in the OpenAPI spec the same way as it would if there were 2 matching types in Accept. In the case of Accept not being defined/having no value, it should be assumed to be */* per the HTTP spec.


TLDR; Per the HTTP spec, a client */* should match against the first type defined in the OpenAPI spec regardless of what it is without needing a wildcard entry.

Examples in the previous comment look good but should also include Client: application/xml, application/json Schema: */* (which implies application/json) Renderer: application/json

=>No Error: An explicit match can be made

jhthorsen commented 5 years ago

Please see my comments in the updated documentation and updated tests in 8ac2bde. I hope it will answer your questions.

plk commented 5 years ago

Is there a url to get this dev version?

jhthorsen commented 5 years ago

Yes. You can use cpanm:

$ cpanm https://github.com/jhthorsen/mojolicious-plugin-openapi/archive/master.tar.gz

plk commented 5 years ago

Not sure what to make of the following simple v3 example (using M::P::OpenAPI DEV 2.12) - this is the built-in which renders the spec in JSON and which most people use as the url parameter for Swagger UI and it errors out with no response rules etc. Obviously we can't control what Accept headers arbitrary hard-coded clients pass (like many REST auto-test tools, centralised Swagger UI over which we have no control etc.). Do we need to put something in code to handle even this default in v3?

use Mojo::Base -strict;
use Mojolicious::Lite;

plugin OpenAPI => {schema => 'v3', url => 'data://main/file.yaml'};

my $ua = Mojo::UserAgent->new;
my $res = $ua->get("/v1.json")->res;

package main;
__DATA__
@@ file.yaml
openapi: 3.0.0
info:
  title: Title
  description: Description
  version: 1.0.0
servers:
  - url: 'https://server.com/v1'
paths:
  '/test':
    get:
      responses:
        "200":
          description: Success listing connections
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/TEST"
components:
  schemas:
    TEST:
      type: object
      properties:
        testprop:
          type: string