jhthorsen / mojolicious-plugin-openapi

OpenAPI / Swagger plugin for Mojolicious
53 stars 41 forks source link

Breaking changes for ->route method in Mojo > 9.11 breaks a bunch of tests #205

Closed aimass closed 3 years ago

aimass commented 3 years ago

Hi there!

Seems Mojo 9.11 forward has changed the route method name to _route so it breaks all these tests. Maybe Mojolicious should support both methods and issue a deprecation notice because this is a breaking a lot of stuff.

find . -type f -exec grep --color -nH --null -e '->route' {} + ./plugin-spec-renderer-doc.t:10: my $under = $app->routes->under('/my-api' => sub {1}); ./plugin-spec-renderer-doc.t:56: $app->routes->get('/url' => sub { $[0]->render(text => $[0]->url_for($name)\ ) }); ./plugin-spec-renderer-doc.t:57: $app->routes->get( ./plugin-spec-renderer-doc.t:63: $app->routes->post('/pets', sub { shift->render(openapi => {}) })->name('addP\ et'); ./basic-under-route-authenticate.t:6:my $auth = app->routes->under('/api')->to( ./basic-register-plugin.t:14:my $obj = plugin OpenAPI => {route => app->routes->any('/one'), url => 'data://mai\ n/one.json'}; ./basic-register-plugin.t:92:ok $obj->route->find('cool_api'), 'found api endpoint'; ./basic-register-plugin.t:93:isa_ok($obj->route, 'Mojolicious::Routes::Route'); ./basic-autorender.t:9: app->routes->namespaces(['MyApp::Controller']); ./basic-coerce.t:16: $t->app->routes->post( ./plugin-spec-renderer-standalone.t:17:$app->routes->get('/my-unknown-doc' => sub { shift->openapi->render_spec\ }); ./plugin-spec-renderer-standalone.t:18:$app->routes->get( ./basic-mojo-route-names.t:6:app->routes->namespaces(['MyApp::Controller']); ./basic-mojo-route-names.t:10:my $r = $t->app->routes; ./basic-mojo-route-names.t:25:$r = $t->app->routes->namespaces(['MyApp::Controller']);

jberger commented 3 years ago

Maybe Mojolicious should support both methods and issue a deprecation notice because this is a breaking a lot of stuff.

It was deemed a security issue and a change happened per the security issue policy. It certainly was unfortunate

jberger commented 3 years ago

Well wait, now I'm confused. There were breaking changes to the route method at 9.0, there was an additional security related breakage at 9.11 with format detection. I'm not sure which issue you are referring to

aimass commented 3 years ago

I think you are correct. I am probably confused with the format detection breaking change in 9.11 I've had lots of issues deploying Mojo these past few days so I'm losing track fast LOL

jhthorsen commented 3 years ago

The tests run with Mojolicious 9.14, so I don't think this is a M::P::OpenAPI issue

https://github.com/jhthorsen/mojolicious-plugin-openapi/runs/2180846470?check_suite_focus=true

I'm confused about your find . -type f -exec grep --color -nH --null -e '->route' {} + output, so please reopen if I missed out on anything.

aimass commented 3 years ago

Yeah the grep was from Emacs, below is a more careful one. Not sure how these tests are passing or the module itself is working with 9.14 is the "route" method name is no longer supported and was changed to _route way back in Dec 20th 2020.

https://github.com/mojolicious/mojo/blob/master/lib/Mojolicious/Routes/Route.pm#L217

https://github.com/mojolicious/mojo/commit/8814bb8c80191256be6dfcc2319c64851bed4cde#diff-7be7af96e5651a8d90f2f63cd66dc40eb7053d94d08ddca8a97d433c9b5a960e

Oddly enough I did do a fresh install with 9.14 and this plugin and it seemed to work. But honestly unsure how. I will investigate more and report back.

lib/Mojolicious/Plugin/OpenAPI.pm:      $r = $self->route->root->find($name);
lib/Mojolicious/Plugin/OpenAPI.pm:      $self->route->add_child($r)                              if $r;
lib/Mojolicious/Plugin/OpenAPI.pm:      $r = $self->route->$http_method($route_path);
lib/Mojolicious/Plugin/OpenAPI.pm:  $self->route($route);
lib/Mojolicious/Plugin/OpenAPI/Security.pm:  return $openapi->route(
lib/Mojolicious/Plugin/OpenAPI/Security.pm:    $openapi->route->under('/')->to(cb => $self->_build_action($openapi, $handlers)));
lib/Mojolicious/Plugin/OpenAPI/SpecRenderer.pm:    my $spec_route = $openapi->route->get(
lib/Mojolicious/Plugin/OpenAPI/SpecRenderer.pm:      = $openapi->route->options($route->pattern->unparsed, {'openapi.default_options' => 1});
t/basic-register-plugin.t:ok $obj->route->find('cool_api'), 'found api endpoint';
t/basic-register-plugin.t:isa_ok($obj->route,     'Mojolicious::Routes::Route');
aimass commented 3 years ago

nvm, my bad.

These route seem OpenAPI route not related to Mojolicious::Routes::Route

I also confirm is working on Mojo 9.14

Thank you for responding to the issue!!