jhthorsen / mojolicious-plugin-openapi

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

Fix for v3 defaults #125

Closed plk closed 5 years ago

plk commented 5 years ago

This PR does the following:

The default response handling issue explains the problems I was having with v3 as I use the plugin default response machinery a lot and it was not finding the default responses because the path was assumed to be e.g. #/definitions/... which doesn't exist in v3.

plk commented 5 years ago

I'll do the pertidy and add a few more tests I think. The _add_default_response() change addresses the v3 issue with the location of the default_response_name schema. I included it as it's an issue with defaults in general with v3.

plk commented 5 years ago

perltidy using your .rc is done on the two changed files. Added array element default handling and added associated tests. Also abstracted some v3 conditionals to generalise v3 support.

jhthorsen commented 5 years ago

Code is still not run through perltidy. At least not with the config in this project.

plk commented 5 years ago

Are you sure? I checked and using your .rc, the two files in my fork return empty diffs with perltidy versions of the same files ...

plk commented 5 years ago

Just occurred to me that I hadn't run perltidy on the new test script - perhaps this is what you meant. Now done.

plk commented 5 years ago

Do you have any idea if you might get to this soon? I ask only as I have to upgrade some projects to v3 and these projects use defaults quite a lot.

jhthorsen commented 5 years ago

I've been reluctant to merge this, since it does not feel like the right solution. I'm leaving it open though, since the tests will be useful in one way or another.

Please look at https://github.com/mojolicious/json-validator/issues/155

plk commented 5 years ago

I am not convinced either but the current situation with v3 is completely broken. On the other hand, it's not a bad solution unless someone wants to fix this in core JSON::Validator which doesn't seem to be the case from the discussion mentioned earlier.

jhthorsen commented 5 years ago

"completely broken" is not very constructive.

And if you could look at the link I posted above, then you would see that I'm pretty sure I've figured out an API that will work for all parties.

This PR is very messy and I'm convinced the solution is in the wrong place, so I'm closing this for now.

plk commented 5 years ago

Alright, "does not feel like the right solution" isn't too helpful either. This PR fixes a bug in v3 $ref resolution for top-level defaults and makes defaults work in most places. I would see this as at least a temporary improvement on not working at all. However, understand that this is your jurisdiction so I'll stop bothering you about it.

jhthorsen commented 5 years ago

does not feel like the right solution

I tried to give a hint that this should be solved in J::V above. I'll be more explicit next time.

Anyhow...

I've now added support for "default" in JSON::Validator 3.08. I think you can test it by doing this:

my $openapi  = $app->plugin(OpenAPI => \%params);
$openapi->validator->coerce('bool,def,num,str');

Note that you need JSON::Validator 3.08 for that to work. I will try to find time to test it with your excellent unit tests in this PR, but unfortunately that won't be today.

plk commented 5 years ago

Regardless, you will still need the fix in _add_default_response() I made as otherwise, default responses won't work in v3 because the reference path is hard-coded for v2 paths. This accounts for the confusion in some of the v3 tickets I think as this bug only happens when default responses are used.

I tried quickly the 3.08 but I couldn't get the test file to work. I noticed that 2.13 throws a deprecation warning for coerce() though.