jhthorsen / mojolicious-plugin-openapi

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

Get rid of x-bundled in spec output #119

Closed augensalat closed 5 years ago

augensalat commented 5 years ago

This PR tries to fix the second issue of #117. I'm absolutely unsure if my solution is right - at least the results seem correct now. Please review thoroughly. In case you accept the PR please note that the first commit adds breaking test cases, so you might want to squash-merge.

jhthorsen commented 5 years ago

Your PR shows that there’s some missing tests in M::P::OpenAPI. I’m leaving this PR open to remind me to add more tests, but unfortunately I cannot merge it. It seems like you completely ignore the bundled spec, meaning that what I mentioned in #117 will not work: A client will receive an invalid spec, in the case where a $ref points to a file on disk, which is not accessible from web.

jhthorsen commented 5 years ago

What we could do however, is to add an option to register() which allows the raw spec to be used, instead of the bundled. I think in many cases this will work, but I can’t change the default, since it will break certain cases. Maybe a flag such as “serve_unbundled_spec”.

augensalat commented 5 years ago

Hm, I extended test register.t to use a remote ref: https://gist.github.com/augensalat/6c9372fbb035a6ba9229a243f5e93c24

And now I'm a little surprised: It seems that $self->validator->get([paths => $path]) and $self->validator->get([paths => $path => $method]); do the dereferencing to the remote file, but $self->validator->get([]) does not. Is this a bug in the validator, or desired behaviour?

jhthorsen commented 5 years ago

I think you're experiencing this behaviour:

The idea is that the JSON path that you give to get() will recursively resolve the $ref, so something like the code below returns "Default response." instead of undef.

# Using JSON::Validator's "fork" of Mojo::JSON::Pointer's get()
$validator->get("/paths/$path/get/responses/400/description"); # Default response.

# Using Mojo::JSON::Pointer::get() directly:
$validator->spec->get("/paths/$path/get/responses/400/description'); # undef()

If it did not resolve the "$ref", then you had to do all the steps manually yourself, since it's hard to know where the author of the spec placed a $ref or not.

augensalat commented 5 years ago

If it did not resolve the "$ref", then you had to do all the steps manually yourself, since it's hard to know where the author of the spec placed a $ref or not.

I'm not able to follow, and I have no idea in which way I could change my code to make it DWIM.

Simple question: Is JSON::Validator::get supposed to dereference (remote file) $refs? If yes, then it is currently broken. If not, then please explain, under which circumstances it dereferences $ref, and in which situation it does not.

jhthorsen commented 5 years ago

I’m not sure how to explain I’m in a different way, but maybe reading the code helps?

https://github.com/mojolicious/json-validator/blob/master/lib/JSON/Validator.pm#L250

$data is tied, in the case where it’s a {'$ref' => ...}.

jhthorsen commented 5 years ago

Closing this in favour of #126. Please review it @augensalat.