feat(rest): add openapi schema consolidation

[dougal83]

Add openapi schema consolidation to rest server as core OAS Enhancer

Implements #4328
See also #4290 (as follows)

For longer term, I would like our REST layer to be smarter and able to automatically extract schemas used in multiple places into /components/schemas and replace them with $ref at usage points.

This logic can work as follows:

1. When building OpenAPI schema, we need to scan the entire OpenAPI spec tree and look for all known places where a schema object can be provided. Those places include endpoint parameters, request bodies, response schemas, and so one. I think it's ok to start with few commonly-used places first and add more of them later, as we learn more.
2. For each schema object encountered, if the object has a title, then add it to a look-up table (title -> schema) and increase the reference counter.
3. After we scanned the spec tree, iterate over all entries in the look-up table, pick entries with two or more references and move them to /components/schemas.
4. Scan the spec tree again, replace all schemas that were moved to /components/schemas in step 3 with $ref. (I think this could be optimized to avoid the second spec tree scan, but that could be done later.)

Originally posted by @bajtos in #4290 (comment)

Checklist

• 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

[bajtos]

Thank you for the contribution, this will be an awesome improvement! ❤️

I have few comments to address and discuss, see below.

[jannyHou]

@dougal83 Great effort! The consolidation logic looks reasonable to me.

And for Miroslav's comment https://github.com/strongloop/loopback-next/pull/4365/files#r363800058, I believe creating an enhancer could be a solution. See example in https://github.com/strongloop/loopback-next/pull/4258/files#diff-1903ce348e29b37ce20637fdc02f9d39R45

[dougal83]

Sorry for taking so long to review your updates.

Not a problem, we all have finite bandwidth.

[bajtos]

👏

[bajtos]

@emonddr can you PTAL and check if your comments have been addressed by now?

[emonddr]

Thanks @dougal83

[dhmlau]

@dougal83, good to merge? I'll leave the honor to you :)

[dougal83]

@dhmlau 👍 I just need to address the following (and ensure test completes as expected): #4365 (comment)

[raymondfeng]

@dougal83 This will remove spec.paths as we try to build openapi spec from the handler. See https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest.server.ts#L800.

You probably have to add a route with the expected api spec.

[raymondfeng]

@dougal83 The following code works for me.

1. We need to call server.route() to set up a real route
2. server.route() needs to be called before start.

  context('options', () => {
    it('disables consolidator if consolidate is set to false', async () => {
      const options: {rest: RestServerConfig} = {
        rest: {openApiSpec: {consolidate: false}},
      };
      options.rest = givenHttpServerConfig(options.rest);
      app = new Application(options);
      app.component(RestComponent);
      server = await app.getServer(RestServer);

      const EXPECTED_SPEC = anOpenApiSpec()
        .withOperation(
          'get',
          '/',
          anOperationSpec().withResponse(200, {
            description: 'Example',
            content: {
              'application/json': {
                schema: {
                  title: 'loopback.example',
                  properties: {
                    test: {
                      type: 'string',
                    },
                  },
                },
              },
            },
          }),
        )
        .build();

      server.route('get', '/', EXPECTED_SPEC.paths['/'].get, () => {});

      await server.start();
      const spec = await server.getApiSpec();
      expect(spec).to.eql(EXPECTED_SPEC);

      await server.stop();
    });

    it('runs consolidator if consolidate is set to true', async () => {
      const options: {rest: RestServerConfig} = {
        rest: {openApiSpec: {consolidate: true}},
      };
      options.rest = givenHttpServerConfig(options.rest);
      app = new Application(options);
      app.component(RestComponent);
      server = await app.getServer(RestServer);

      const EXPECTED_SPEC = anOpenApiSpec()
        .withOperation(
          'get',
          '/',
          anOperationSpec().withResponse(200, {
            description: 'Example',
            content: {
              'application/json': {
                schema: {
                  $ref: '#/components/schemas/loopback.example',
                },
              },
            },
          }),
        )
        .withComponents(
          aComponentsSpec().withSchema('loopback.example', {
            title: 'loopback.example',
            properties: {
              test: {
                type: 'string',
              },
            },
          }),
        )
        .build();

      server.route(
        'get',
        '/',
        anOperationSpec()
          .withResponse(200, {
            description: 'Example',
            content: {
              'application/json': {
                schema: {
                  title: 'loopback.example',
                  properties: {
                    test: {
                      type: 'string',
                    },
                  },
                },
              },
            },
          })
          .build(),
        () => {},
      );

      await app.start();
      const spec = await server.getApiSpec();
      expect(spec).to.eql(EXPECTED_SPEC);

      await app.stop();
    });
  });

[raymondfeng]

BTW, this is not really a unit test anymore. We probably should move it to integration.

[jannyHou]

thank you @raymondfeng . In getApiSpec, spec.paths and spec.components.schemas are not preserved, is it intended?
I remember we discussed this before, if the paths and schemas should be kept I can submit a PR to fix it.
(the discussion here is not related to PR I can open a new issue if we need more opinions)

[jannyHou]

For the rest configurations:
the unit tests are added in tests/unit/rest.server/rest.server.unit.ts
and integration tests are in tests/integration/rest.server.integration.ts
links point to the existing options tests.

[dougal83]

EDIT: I will address The following code works for me. onward as missed.

[dougal83]

@raymondfeng Thank you very much for your guidance! 👍

@jannyHou Thank you for your help! Could you please check that the tests are located in the correct place now? I've no formal training on tests so I'm really just hacking.

[dougal83]

@jannyHou Thanks for pointers. Could you please check changes to packages/rest/src/__tests__/unit/rest.server/rest.server.open-api-spec.unit.ts and if it is OK then I think we are good to merge pending tests passing. 👍

[dougal83]

I've just noticed comments I've missed via notifications. Will revisit pr tomorrow to double check. (I've not ignored them)

[mschnee]

This looks fantastic.

[raymondfeng]

Great effort and patience. Please squash all commits into one before merge.

[dougal83]

Great effort and patience. Please squash all commits into one before merge.

@raymondfeng @bajtos Quick query. Should I drop the following commit as it was not used? a0a6c22

[raymondfeng]

I would keep it.