jhthorsen / mojolicious-plugin-openapi

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

openapi.spec broken in 2.10 #107

Closed mohawk2 closed 5 years ago

mohawk2 commented 5 years ago

It seems that 2.09 worked fine, but 2.10 has broken Yancy. Now the openapi.spec helper is returning a Mojo::Collection of two different response specs, which then blows up the plugin on line 282 which wants the $op_spec to be a hash, not that.

Possibly connected is that there are two different string constants used here, for_current vs for_path:

sub _helper_get_spec {
  my $c    = shift;
  my $path = shift // 'for_current';
  my $self = _self($c);

  return $self->validator->get($path) if ref $path or $path =~ m!^/! or !length $path;

  my $jp;
  for my $s (reverse @{$c->match->stack}) {
    $jp ||= [paths => $s->{'openapi.path'}];
  }

  push @$jp, lc $c->req->method if $jp and $path ne 'for_path';    # Internal for now
  return $jp ? $self->validator->get($jp) : undef;
}

See https://travis-ci.org/preaction/Yancy/jobs/487286044#L698 for an example.

@preaction FYI.

jhthorsen commented 5 years ago

Can you link to the test that blows up in Yancy? Also, what is the actual JSON pointer? Sure it's valid?

mohawk2 commented 5 years ago

The test, which is shown in the Travis log I linked, is (even with the line that fails): https://github.com/preaction/Yancy/blob/master/t/plugin/auth/basic.t#L119

The "actual JSON pointer" is set to the array [ 'paths', undef, 'get' ]. It seems the problem is that $s->{'openapi.path'} is undef. The request that fails is for /yancy/api, which I infer leaves that openapi.path as undef. Should the above should actually read:

($s->{'openapi.path'} // '') ? (or possibly openapi.path should be set to '' in that scenario)

You have not addressed my point about for_current vs for_path in your code: are they supposed to be different?

mohawk2 commented 5 years ago

This is the commit that broke Yancy: b2667efd3e8f04381e8d8d33a378a25179f1c2d8

mohawk2 commented 5 years ago

@preaction Good news, #108 should sort this out, assuming it is found suitable by @jhthorsen!