jhthorsen / mojolicious-plugin-openapi

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

`in: body` parameter can only be JSON and I'm not sure it's right #31

Closed skaurus closed 7 years ago

skaurus commented 7 years ago

During call to $c->openapi->validate existense of in: body parameter is currently verified via $c->req->json, and json method returns something only if $c->req is a valid JSON.

Relevant code lines: https://github.com/jhthorsen/json-validator/blob/master/lib/JSON/Validator/OpenAPI.pm#L58 https://github.com/jhthorsen/json-validator/blob/master/lib/JSON/Validator/OpenAPI/Mojolicious.pm#L11

But Swagger 2.0 spec doesn't seem to enforce for body parameters to be JSON. Body parameter must be defined using Schema Object, but

Unlike previous versions of Swagger, Schema definitions can be used to describe primitive and arrays as well.

http://swagger.io/specification/#fixed-fields-45 http://swagger.io/specification/#primitive-sample-86

jhthorsen commented 7 years ago

Reason behind this is that I don't have any personal interest to support anything else. It is already supported to respond with any format you like though: https://metacpan.org/pod/Mojolicious::Plugin::OpenAPI#RENDERER

I don't have time to implement the "input" part, but I will take a pull request. Please come to #swagger in irc.perl.org if you want to discuss it first.

skaurus commented 7 years ago

I think I was not clear enough, sorry.

It is not about response part, it is about request part. If I have a request with some mandatory parameter in request body, and this body is not a valid JSON (simple string for example), then call to $c->openapi->validate in my controller will report that this mandatory parameter is not provided at all.

And given my quote from Swagger 2.0 spec I'm not sure that body parameter really must be a valid JSON.

jhthorsen commented 7 years ago

Can you show me a minimal version of the schema that doesn't work?

jhthorsen commented 7 years ago

Closing this, because I think your schema is wrong. See the test in the commit above for a minimal schema.

skaurus commented 7 years ago

Thanks for showing a test, it is now much easier for me to piggyback on it and create failing one :)

Here it is: http://pastebin.com/KSAncSsB

All there is to make it pass is to change $c->req->json in this line https://github.com/jhthorsen/json-validator/blob/master/lib/JSON/Validator/OpenAPI/Mojolicious.pm#L11 to $c->req->body.

Things I've changed from your test:

  1. in: body instead of in: formData
  2. parameter is required: true
jhthorsen commented 7 years ago

I don't understand what you're talking about. A plain string cannot be body, it needs to be formData. A plain string in JSON is not just a string, it need to be quoted.

I can't help you if you're trying to enforce an invalid schema. Please refer to a specification or rephrase your question/statement.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameter-object

skaurus commented 7 years ago

A plain string cannot be body, it needs to be formData.

why you think so? As I told in first message, per Swagger 2.0 spec body must be described by an Schema object (http://swagger.io/specification/#fixed-fields-45). And Schema definitions can be used to describe primitive and arrays as well. (link http://swagger.io/specification/#primitive-sample-86).

skaurus commented 7 years ago

As told in the title of the issue, I'm not actually sure that body not must be a valid JSON. I reported something that I thought might be an issue. Either body MUST be a JSON indeed, either there is a mistake that you requiring it to be a JSON. It seems that we are finally understanding each other and I'm just hoping that you'll either explain to me why I'm wrong and why body must be a JSON either we'll agree that it's not a requirement.

edit: I just grepped up Swagger spec again by word body and I don't see anything saying that it must be a JSON.

jhthorsen commented 7 years ago

"body" should be a datastructure (something that can be validated), meaning XML, JSON or something similar. If you want arbitrary data, then you must use formData+file instead.

This module only supports JSON and binary data (image data, binary documents, ...) as input. This means that if you want to post a string using "in":"body", it must be a JSON string. Look at the two lines below. The first is a JSON string, the second line is not:

"Correct"
Invalid
mrenvoize commented 7 years ago

To add to that, following through the links you've provided: http://swagger.io/specification/#fixed-fields-45 leads me to http://swagger.io/specification/#schemaObject from the 'body' section.. which in tern states that the schema is based on http://json-schema.org/ and it does not state that primitives are treated differently.

So.. that leads me to exactly the same conclusion as @jhthorsen and to prove the point using a well respected third party validator:

https://jsonschemalint.com/#/version/draft-04/markup/json?gist=64d5d42f9f11441610ad0efffc86aee6 - Passes https://jsonschemalint.com/#/version/draft-04/markup/json?gist=0ef40ac82d246170f7666a542361fe2c - Fails

webron commented 7 years ago

Apologies for butting in (blame @mrenvoize) but just a few clarifications:

webron commented 7 years ago

Oh, and of course, @jhthorsen can choose to support whatever he wants, given it's his tool :)

leejo commented 7 years ago

@skaurus - that test case is bad, here's a correct one: http://pastebin.com/MW1zwfie

Note the differences:

1) you're posting JSON so you need to stipulate that otherwise the Content-Type header isn't going to be set 2) you don't need to faff about with encoding

If you're trying to get text/plain accepted for validation that's never going to work. This module works with JSON, you need to supply JSON.

jhthorsen commented 7 years ago

@webron: Thanks for butting in! 👍

@leejo: It kind of works fine with "binary data" as well. It's just that the actual body part cannot be validated, so you simply slurp in the data. At least that is my understanding.

jhthorsen commented 7 years ago

Please open a new issue if someone wants this module to validate consumes/produces.

skaurus commented 7 years ago

Ok, double quotes, I see. Thanks everyone :-)

webron commented 7 years ago

@jhthorsen you're right about RFC7159, however, JSON Schema Draft 4 was published before RFC7159 (and before that, primitives were not a legal JSON). V2 of Swagger/OAS relies on Draft 4, even though it was released after RFC7159. So the situation is a bit tricky. I'd say it's open for interpretation.

FWIW, OASv3 depends on what's is sort of known as Draft 5, so that will take this issue out of the equation.

jhthorsen commented 7 years ago

@webron: Ah! That is interesting about Draft 4 released before RFC7159... I will keep on allowing it, since it's hard for me to /not/ allow it, hehe.

We're quite excited about OASv3. Do you know if there's a schema available? We're waiting for it here https://github.com/jhthorsen/json-validator/issues/41, but I've only found https://github.com/OAI/OpenAPI-Specification/releases

Sorry for messing up the history on this issue. Could you please respond on https://github.com/jhthorsen/json-validator/issues/41 if you have any information @webron ?

webron commented 7 years ago

@jhthorsen just did. If there are spec-specific questions (in general), feel free to ping me. I'll do my best to assist.

MikeRalphson commented 7 years ago

Do you know if there's a schema available?

It may (or may not) become the official OpenAPI 3 schema, but there is one here which has been tested against thousands of converted swagger 2.0 definitions.