Loopback-next: wiki: validate and fix all OpenAPI Spec snippets

[bajtos]

Many wiki pages (e.g. Thinking in LoopBack) show snippets of Swagger/OpenAPI spec documents, some of them are not valid.

We should find all OpenAPI spec snippets in our wiki, run then against a validator (e.g. http://editor.swagger.io/) and fix any syntax/validation errors

[b-admike]

Routing page revision
Controllers page revision

[bajtos]

@b-admike I think it was a very valuable exercise to let somebody with a fresh mind to read through our wiki. The changes you made show how easy it is to misunderstand the real data formats when all the reader has is the text in the wiki. Let's make the wiki more clear to prevent this kind of confusion for future readers!

https://github.com/strongloop/loopback-next/wiki/Routing/_compare/9ef503e13acd7d1797148bee82c8f688166c7357...0b631f5bb8ac06b23bfc2ce0afa3d797d125409a#diff-c4a8fb233b8f9be2a14d7151f7d57f22R31

The Route constructor is accepting an OperationObject, not the full API spec.

Please rework.

https://github.com/strongloop/loopback-next/wiki/Routing/_compare/9ef503e13acd7d1797148bee82c8f688166c7357...0b631f5bb8ac06b23bfc2ce0afa3d797d125409a#diff-c4a8fb233b8f9be2a14d7151f7d57f22R71

-        'x-operation': greet,
+        'x-operation-name': greet,

The field x-operation contains a function (the handler function), while x-operation-name contains a string (Controller method name).

Please revert this change, consider adding a short note explaining the different between these two fields.

https://github.com/strongloop/loopback-next/wiki/Controllers/_compare/262f688ff43bfc9cba8780aaaa4ed50fce9ffbb1...43aa38422dc9306f555197091fcea2220f8db708#diff-2113b58ef5d0093238964e5752bae8e1R27

Again, routes use OperationObject, not the full API spec.

Please rework.

Perhaps the text needs to be clarified to make it clear to the reader what entity from OAI/Swagger spec we are using for different spec objects?

https://github.com/strongloop/loopback-next/wiki/Controllers/_compare/262f688ff43bfc9cba8780aaaa4ed50fce9ffbb1...43aa38422dc9306f555197091fcea2220f8db708#diff-2113b58ef5d0093238964e5752bae8e1R80

-    '/greet': {
+    '/': {

What was wrong with /greet path? Can we preserve it?

https://github.com/strongloop/loopback-next/wiki/Controllers/_compare/262f688ff43bfc9cba8780aaaa4ed50fce9ffbb1...43aa38422dc9306f555197091fcea2220f8db708#diff-2113b58ef5d0093238964e5752bae8e1R102

The @api decorator does not require a full Swagger spec - see https://github.com/strongloop/loopback-next/blob/0eb8d049af8252c2ae5b58c42ccc10644b9fa6bf/packages/core/src/router/metadata.ts#L22-L34

Please rework, consider explaining the difference between full Swagger spec and what is passed to @api decorator.

[b-admike]

@bajtos I've applied your feedback; PTAL
Routing page revision
Controllers page revision

[Setogit]

@b-admike
Rounting spec:

• basePath: '/', can be removed?
  Controllers spec
• Why these lines deleted?

paths: {
-    '/': {
-      get: {
-        'x-operation-name': greet,

Clean and best approach is to actually verify with https://editor.swagger.io/
When use the editor, the preamble portion needs to be there:

  "swagger": "2.0",
  "basePath": "/",
  "info": {
    "title": "LoopBack Application",
    "version": "1.0.0",
  },

[b-admike]

@Setogit

• basePath is part of ControllerSpec interface see here
• I deleted the the lines because the Route constructor accepts OperationObject
• Yes I checked with https://editor.swagger.io/, and it does complain if the preamble is not there, but does it make sense to include it even though we're looking for PathsObject or OperationObject only in the route constructor /@api decorator?

[bajtos]

Routing: I made few last corrections myself, see https://github.com/strongloop/loopback-next/wiki/Routing/_compare/5592c2c63bf013fe06aef2d2af1459c670bec3b4...6b96b57bad982385d29d3a2eaf4082a8b1b4ca7f

[bajtos]

The @api decorator is intentionally using ControllerSpec type instead of OpenApiSpec type, with the main difference that ControllerSpec does not contain preamble and basePath is optional.

Most applications will have more than one controller, it does not make sense (at least to me) to repeat Swagger pre-amble in every controller specification. It also simplifies our implementation, because we don't have to worry about merging possibly different preambles from each controller.

Last but not least, when the controller decorated with @api is defined in a 3rd-party component, then this component does not have information about the target application title, version, etc., therefore it cannot fill the preamble.

[bajtos]

Controllers: I made few last corrections myself too, see https://github.com/strongloop/loopback-next/wiki/Controllers/_compare/af3a795271cbc90f83678a26014c31516abdf04b...54a36899fec9d165ae716b6279e3301e2709264d.

[bajtos]

As far as I am concerned, these two wiki pages are good enough now.

@b-admike have you reviewed and validated specs in this page too? https://github.com/strongloop/loopback-next/wiki/Thinking-in-LoopBack

[b-admike]

Thanks @bajtos; yes, I am looking at that page now. I was working on #471.

EDIT: PTAL at changes here.

[b-admike]

@bajtos @Setogit @virkt25 One last page PTAL: Sequences - Advanced page

[bajtos]

One last page PTAL: Squences - Advanced page

LGTM 👍

[bajtos]

As for Thinking in LoopBack - the current specs don't seem correct to me. I have fixed the obvious mistakes myself https://github.com/strongloop/loopback-next/wiki/Thinking-in-LoopBack/_compare/a6e8480574e153021bb5935d526bb1dd4aec0e48...e5781e6c3f3842d9c4dcdcc6c516a1a5110cb3ca.

There is one case that's more involved and I'd like you to fix it yourself (and verify the fix):

-            type: 'array',
-            items: resource.searchItemSchema
+            description: "successful response",
+            schema: resource.searchItemSchema

The operation is returning an array of search items. Your spec is saying that a single search item is returned.

PTAL

[b-admike]

Thanks, @bajtos! I have made the change on return type. Here is the final full OpenAPI spec for that page. The swagger editor complains that the in key for slug parameter cannot be path.

Schema error at paths['/deal/{slug}'].get.parameters[0].in
should be equal to one of the allowed values
allowedValues: body, header, formData, query

However, based on the OpenAPI specification, it is possible. Thoughts?

On top of that there are 2 more errors:

Schema error at paths['/deal/{slug}'].get.parameters[0]
should NOT have additional properties
additionalProperty: type, name, in
Jump to line 52
Semantic error at paths./category/{slug}/products
Declared path parameter "slug" needs to be defined as a path parameter at either the path or operation level
Jump to line 89

Need to look into them, but maybe they can be ignored because for the slug needs to be defined error, I was able to verify that it does read it from the path.

[bajtos]

Schema error at paths['/deal/{slug}'].get.parameters[0].in
should be equal to one of the allowed values
allowedValues: body, header, formData, query

You must annotate the parameter as required: true. Optional parameters are not allowed in the path (in Swagger Spec v2).

Semantic error at paths./category/{slug}/products
Declared path parameter "slug" needs to be defined as a path parameter at either the path or operation level
Jump to line 89

Your operation defines CategoryResource parameter in: query, but does not define any parameter for the {slug} part of the path.

[b-admike]

@bajtos I have made latest change according to your feedback, PTAL.

[bajtos]

+// Since the `searchPath` for CategoryResource contains {slug} placeholder,
+// and is not accounted for by our pattern as the other two resources,
+// let's add the {slug} parameter to its get operation object.
+
+const slugParam = [
+  { name: "slug", type: "string", in: "path", required: true }
+];
+CategoryResource.paths['/category/{slug}/products'].get.parameters = slugParam;

I find this rather ugly. Why cannot we define slug parameter directly inside the definition of the search operation?

[b-admike]

I've talked with @bajtos on how to best approach this. We both find the design of the example confusing because CategoryResource is different than the other two resources and should not use the helper function. I've made a change to add the query parameter to the search operation for now, but will talk to @ritch on how to fix up CategoryResource.

[b-admike]

@bajtos We found that lodash's merge function is more robust than deep-assign so I've changed that in the Wiki as well. I have defined the path in CategoryResource which doesn't follow the template manually. PTAL https://github.com/strongloop/loopback-next/wiki/Thinking-in-LoopBack/_compare/178227d150f096c37498bff9df7b89421a46df1a...1f599ca32e7acd1924820e8cf5feab9de36c476b

[bajtos]

LGTM 👍

[b-admike]

@bajtos Thank you; I am going to close this story as done, but feel free to re-open if you feel otherwise.