fix(rest): assign all component properties to target spec

[nabdelgadir]

When using mountExpressRouter to mount a LoopBack 3 application, the explorer complains when trying to do a POST request (for LB3 models), e.g.:

And this doesn't allow a POST (or similar) request to be made through the explorer:

By adding all of component's properties, components.requestBodies will be added to the spec and this allows requests to be made through the explorer:

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[jannyHou]

@nabdelgadir The request body specification should be added in the operation spec, instead of under the components field.

See the doc for OpenAPI 3 operation object.

Usually, we add referenced schema in components, and the request body spec refers to it.
A good example you can look at is the OpenAPI spec for a pet store app, it's an official example:

Spec to define the request body
Spec to define schema in components

[nabdelgadir]

@jannyHou thanks, Janny! The reason I was copying it to components object (which also includes a requestBodies field) is because it seems like the explorer looks for /components/requestBodies instead of /operations/requestBodies for LoopBack 3 models. In the LoopBack 3 application's spec, it also puts it in /components/requestBodies.

I'm not too familiar with LoopBack 3, but I don't believe the spec includes /operations object, but do you know if there's a way to modify the explorer to look for it instead of /components?

[jannyHou]

@nabdelgadir IIRC LoopBack 3 is using swagger 2.x spec to describe the REST API, which means you may need to convert it to OpenAPI 3.x spec then add the spec to route.

If the spec is already converted or already on OpenAPI 3.0, the missing part could be generating OpenAPI schema for LB3 models and add it to components.schemas

[nabdelgadir]

@jannyHou They're already added to components.schemas (like AccessToken here):

The problem is that the explorer is looking for components.requestBodies:

[jannyHou]

@nabdelgadir I see thank you for posting the spec here.

The problem is that the explorer is looking for components.requestBodies:

This looks suspicious to me. I think the correct place for searching should be components.schemas.

[bajtos]

I believe ref: '#/components/requestBodies is a valid reference, see OpenAPI spec: https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md#componentsObject

Components Object

Holds a set of reusable objects for different aspects of the OAS. All objects defined within the components object will have no effect on the API unless they are explicitly referenced from properties outside the components object.

Field Name	Type	Description
(...)
requestBodies	Map[string, Request Body Object | Reference Object]	An object to hold reusable Request Body Objects.

In general, endpoints can use different schemas for the request body. For example, POST endpoint for create method typically does not allow id (primary key) property in the request, therefore it needs to use a different schema. LoopBack3 applications have two schemas: $new_CoffeeShop describing a request body to create a new CoffeeShop instance, and CoffeeShop used everywhere else.

I am going to use the example app from #2803 to review the generated schema and better understand what's going on.

[bajtos]

Note that entries in requestBodies are not the same as in schemas, this is what I see in result.openapi.components.requestBodies.CoffeeShop:

{
  "content": {
    "application/json": {
      "schema": {
        "$ref": "#/components/schemas/CoffeeShop"
      }
    },
    "application/x-www-form-urlencoded": {
      "schema": {
        "$ref": "#/components/schemas/CoffeeShop"
      }
    },
    "application/xml": {
      "schema": {
        "$ref": "#/components/schemas/CoffeeShop"
      }
    },
    "text/xml": {
      "schema": {
        "$ref": "#/components/schemas/CoffeeShop"
      }
    }
  },
  "description": "Model instance data"
}

[bajtos]

I am proposing to fix the problem by changing assingRouterSpec to copy all components, not only the schemas. The following patch seems to work well for me:

diff --git a/packages/rest/src/router/router-spec.ts b/packages/rest/src/router/router-spec.ts
index 31b8aa2e..32f71b94 100644
--- a/packages/rest/src/router/router-spec.ts
+++ b/packages/rest/src/router/router-spec.ts
@@ -8,10 +8,13 @@ import {OpenApiSpec} from '@loopback/openapi-v3-types';
 export type RouterSpec = Pick<OpenApiSpec, 'paths' | 'components' | 'tags'>;

 export function assignRouterSpec(target: RouterSpec, additions: RouterSpec) {
-  if (additions.components && additions.components.schemas) {
+  if (additions.components) {
     if (!target.components) target.components = {};
-    if (!target.components.schemas) target.components.schemas = {};
-    Object.assign(target.components.schemas, additions.components.schemas);
+
+    for (const key in additions.components) {
+      if (!target.components[key]) target.components[key] = {};
+      Object.assign(target.components[key], additions.components[key]);
+    }
   }

[bajtos]

Let's make sure we all understand the problem in full depth and then find a robust solution. See my comments above to get the discussion started.

[nabdelgadir]

@bajtos thanks for chiming in! In my first commit, I transferred over components.requestBodies, but do you think we should transfer over the whole components object?

I don't know if you introduced assignRouterSpec, but do you know why originally only components.schemas was assigned to the target spec instead of the full components?

[bajtos]

@bajtos thanks for chiming in! In my first commit, I transferred over components.requestBodies, but do you think we should transfer over the whole components object?

Well, I suppose we can work incrementally and add components.requestBodies only, but I suspect it will be a matter of time until we discover another components entry that needs to be copied.

I don't know if you introduced assignRouterSpec, but do you know why originally only components.schemas was assigned to the target spec instead of the full components?

I think it was me who introduced assignRouterSpec, or at least the idea of such function. I copied over components.schema only because that was the quickest solution in my spike. I did not know about other components fields like requestBodies at that time.

Now that I have a better understanding, it makes most sense to me to copy (assign) all components entries/fields. This is important because there may be custom extensions too, e.g. components.x-auth-strategies.

[nabdelgadir]

Now that I have a better understanding, it makes most sense to me to copy (assign) all components entries/fields. This is important because there may be custom extensions too, e.g. components.x-auth-strategies.

@bajtos thanks, I agree. I'll update it to assign all of components then.

[nabdelgadir]

I'm not sure why it's saying coverage dropped:

[bajtos]

I'm not sure why it's saying coverage dropped:

I am not sure either. Since the drop is -0.002%, I think it's ok to ignore it.

[jannyHou]

@bajtos Thank you for looking into the issue. I thought the explorer is looking for a missing schema, didn't realize we do generate reusable requestBodies.

Now that I have a better understanding, it makes most sense to me to copy (assign) all components entries/fields. This is important because there may be custom extensions too, e.g. components.x-auth-strategies.

+1 I am not aware of any fields in https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md#componentsObject shouldn't be copied in a LB4 app, so I am good with the patch @bajtos posted above.

I am going to review the code details asap.

@nabdelgadir you can click here to check why the coverage decreases:

In you case I think it's some code in the hello word example causes the drop, see https://coveralls.io/jobs/48100920/source_files/566824703

And feel free to ignore, "Since the drop is -0.002%, I think it's ok to ignore it." +1

[b-admike]

Came from #2803 to see what this PR was about, learned some stuff :-).

[jannyHou]

👏 LGTM good job!