New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenAPI inference start #43

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mohawk2
Copy link
Contributor

mohawk2 commented Dec 31, 2018

These are improvements to the inferences made from the OpenAPI spec about the API, used to hook up the API to the API controller, together with some doc tweaks.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 31, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 6e0e5e8 on mohawk2:openapi-infer-start into 7de941e on preaction:master.

@mohawk2 mohawk2 force-pushed the mohawk2:openapi-infer-start branch from a988990 to 770b51a Dec 31, 2018

@preaction
Copy link
Owner

preaction left a comment

I still like the general direction this is going in, but there are some specific changes I'd like to see before I can merge this.

Add a new item to the collection. The collection name should be in the
stash key C<collection>.
The new item should be in the request body as JSON, under parameter

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

This doesn't make sense to someone reading the docs unless they also have the OpenAPI spec open next to them (and even then, I don't really understand this) . To me, it sounds like either I need to URL-encode the JSON and POST a form body of newItem={...} or I need to post a JSON body with {"newItem": <the actual item>}, neither of which are correct.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, updated!

The item's ID field-name is in the stash key C<id_field>. The
ID itself is looked up from the hash output by
L<Mojolicious::Validator::Validation/output>.

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

We don't need to document implementation details. This is where the OpenAPI plugin puts things that it has properly validated. What we need to tell users is that there needs to be a parameter with the same name as the id_field stash.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, updated!

item should be in the request body as JSON.
key C<collection>.
The item's ID is in the stash key C<id_field>. The ID itself is looked

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

This paragraph combines both of the above problems: The validation does not need to be documented (but how the id_field works and that a parameter with that name needs to exist should be documented), and the item needs to be the entire body of the request, encoded as JSON.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed. I've updated this to just refer to get_item and add_item.

C<id>.
stash key C<collection>.
The item's ID is in the stash key C<id_field>. The ID itself is looked

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

It's the name of the item's ID field, not the ID. And this also mentions unnecessary implementation details.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed. I've updated this to just refer to get_item.

collection => $name,
id_field => $path_params[0]{name},
};
} else {

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

Please don't cuddle elses. Give the closing brace its own line.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Fair enough. This is not in line with CONTRIBUTING.md, may I suggest you update this bit?

Opening brace on the same line as the opening keyword

This comment has been minimized.

@preaction

preaction Jan 9, 2019

Owner

That's the closing brace, though. I checked CONTRIBUTING.md before I made the comment, and it doesn't mention this, so I'll add a new bullet point to that file (sorry):

  • Closing brace on its own line. Don't cuddle elsif and else (like } else { on one line)

=item a key C<x-collection> with string-value under the path

=item within the path's operations, the input (for mutations) or output type

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

This is looked up with the schema of the body parameter or the 200 response, yes? It might be clearer to explain it that way.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, updated

# mutates $spec
sub _openapi_spec_add_mojo {
my ( $self, $spec, $config ) = @_;
my $path2collection = $self->_openapi_path2collection( $spec );

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

Rather than building this entire map for all the paths, it might make more sense in the code to have a method that accepts the $path and $pathspec and returns the collection name. That would remove the need for this variable and the awkwardly-named _openapi_path2collection method (_openapi_find_collection_name could be a good name for this method).

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, much better!

my ( $self, $path, $pathspec, $method, $op_spec, $api_controller ) = @_;
my ($name) = $path =~ m#^/([^/]+)#;
die "No 'name' found in '$path'" if !length $name;
my ( $self, $path, $pathspec, $method, $op_spec, $api_controller, $collection ) = @_;

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

There is an untenable amount of arguments being passed-in to this function. Two of them ($collection, and $api_controller) are only used to put in to the hashref. Those keys could be added to the hashref after it's returned from this function, and that would get it the number of arguments back into the realm of reasonableness (4 arguments + invocant is okay, 6 is a bit much).

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, great catch.

@@ -459,6 +459,12 @@ an error. To derive which collection, these things are considered:

=back

Each operation infers from the HTTP method plus other information which
method of L<Yancy::Controller::Yancy::API> it should connect to.
The C<id_field> parameter is the C<name> of the last C<in: "path"> parameter

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

Are you sure it's the last one and not the first one?

$ perl -E'my ( $foo, $bar ) = grep $_ > 5, 1..9; say "\$foo: $foo\n\$bar: $bar"'
$foo: 6
$bar: 7

If it was the last one, I'd really prefer it to be the first one.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

My rationale is e.g. this situation (from https://github.com/gothinkster/realworld/blob/master/api/swagger.json#L654): /articles/{slug}/comments/{id}. Arguably, it's an error to even make the comment's ID be "under" the article, since a sensible relational scheme is to have each comment's ID be globally unique. However, where that uniqueness is true, the correct ID (and from the correct "collection", which is Comment) is to use the last, that is most specific, path parameter, not the first.

What do you think?

This comment has been minimized.

@preaction

preaction Jan 9, 2019

Owner

Ah, I was thinking of the case /articles/{id}/{slug} (for accessing just the article). In this case, the slug is actually worthless, but it reads well. Humph. Those are competing use-cases... But, I agree with you that the last is probably more important more often, so we'll go with this and see what happens. We may find some cases later this doesn't work for, but we'll deal with them when we get there.

method of L<Yancy::Controller::Yancy::API> it should connect to.
The C<id_field> parameter is the C<name> of the last C<in: "path"> parameter
specified in the operation's spec. Every operation type except C<get>
in "list" mode, and C<post>, needs an C<id_field> parameter.

This comment has been minimized.

@preaction

preaction Dec 31, 2018

Owner

I think it's better to say what operations do require an ID field, not which ones don't: Saying which ones don't means going to look up what all the operations are to then subtract the ones that don't. It's much easier to verify against a list of inclusions than a list of exclusions.

This comment has been minimized.

@mohawk2

mohawk2 Jan 1, 2019

Contributor

Agreed, updated

@mohawk2 mohawk2 force-pushed the mohawk2:openapi-infer-start branch 2 times, most recently from fd50c91 to 6e0e5e8 Jan 1, 2019

@preaction preaction force-pushed the preaction:master branch from ecae6fa to 800085d Jan 2, 2019

@mohawk2 mohawk2 force-pushed the mohawk2:openapi-infer-start branch from 6e0e5e8 to a4dd7a7 Jan 6, 2019

@mohawk2

This comment has been minimized.

Copy link
Contributor

mohawk2 commented Jan 6, 2019

@preaction I just rebased against new master. Please let me know if you'd like further changes :-)

@preaction

This comment has been minimized.

Copy link
Owner

preaction commented Jan 9, 2019

This was failing Travis due to the JSON::Validator changes. I rebased and Travis succeeded so I'm merging this.

@preaction preaction closed this Jan 9, 2019

preaction added a commit that referenced this pull request Jan 9, 2019

release v1.021
    [Added]

    - Added some more intelligent inferences about a configured OpenAPI
      spec to hook up the controller. In short: It's now easier to
      provide your own OpenAPI spec to work with the Yancy editor.
      Thanks @mohawk2 for continuing this work! [Github #43]

    [Fixed]

    - Fixed compatibility with JSON::Validator version 3. Thanks
      @mohawk2 for the patch! [Github #45] Thanks @eserte for the report
      [Github #44]

@mohawk2 mohawk2 deleted the mohawk2:openapi-infer-start branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment