Redocly / redocly-cli

⚒️ Redocly CLI makes OpenAPI easy. Lint/validate to any standard, generate beautiful docs, and more.
https://redocly.com/docs/cli/
MIT License
957 stars 148 forks source link

Join command needs to join multiple spec with server prefix #1403

Open ootkin opened 10 months ago

ootkin commented 10 months ago

Is your feature request related to a problem? Please describe.

Currently the join cli command does not resolve conflicts based on paths and operationIds if we got different servers information.

Describe the solution you'd like

The join command needs to resolve this kind of conflicts using the server base url or using a custom prefix.

Describe alternatives you've considered

The alternative is to prefix manually every path and operationId to prevent conflicts, which is very verbose. The operationId is usefull if we need to generate an SDK from the specs.

Additional context

Considering different teams that are working on different microservices. Every microservices have their own servers, paths and operationId in the openapi.yaml, for example:

user.yaml for user microservice:

openapi: 3.1.0
info:
  title: User microservice
  version: 1.0.1
servers:
  - url: https://api.server.com/users
paths:
  /healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - users

product.yaml for product microservice:

openapi: 3.1.0
info:
  title: Product microservice
  version: 1.1.1
servers:
  - url: https://api.server.com/products
paths:
  /healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - products

If I run:

redocly join user.yaml product.yaml

I get the following error messages:

Conflict on paths => /healthz : get in files: user.yaml,product.yaml 
Conflict on paths => operationIds : healthz in files: user.yaml,product.yaml 
Please fix conflicts before running join.

The error is very descriptive, however in the servers section I specified different prefixes.

This behaviour limits the flexibility on every teams because they need to know if a operationId is already used in other specifications.

tatomyr commented 10 months ago

Hi @ootkin! Thanks for bringing this up. What output would you expect in the joined file in this example?

ootkin commented 10 months ago

For the operationId, I'm not entirely certain. However, when generating an SDK (which is based on operationId for naming methods and tags for namespacing them), I would expect something like this:

sdk.user.healthz();
sdk.product.healthz();

For the paths and servers, I anticipate a merge similar to the following:

servers:
  - url: https://api.server.com
paths:
  /products/healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - products
  /users/healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - users

In this setup, the join command will assign the appropriate prefix to the paths to avoid conflicts, and the prefix would be derived from the servers section.

tatomyr commented 10 months ago

Thanks! I have to think this through. I'm not sure what logic should apply if there are several servers (e.g. test ones) that could contain different pathnames (or even have no pathname). Also, server could contain server variables, so they should be put somewhere into parameters instead.

Meanwhile, you can use a custom plugin decorator as a workaround. The idea is to go through each OpenAPI description and remove the pathname from the server url (assuming that they contain only one server or it's agreed to use e.g. the first one) and prepends each path item with it, something like this:

      const DecoratePaths = () => {
        let serverPathname
        return {
          Server: {
            enter(server) {
              serverPathname = new URL(server.url).pathname
              server.url = new URL(server.url).origin
            },
          },
          Root: {
            leave(root) {
              const newPaths = {}
              for (let path in root.paths) {
                newPaths[serverPathname + path] = root.paths[path]
              }
              root.paths = newPaths
            }
          }
        };
      },

And then you can join the bundled files into one.

More examples of the decorators in the Redocly CLI Cookbook.

Please let me know if that helps.

ootkin commented 10 months ago

Thanks for the quick response.

It's the first time that I use redocly and it seems very interesting.

Do you have some examples/resources that i can consult in order to use some best practice to document multiple services into one centralized spec? Maybe i'm missing something...

Thanks!

tatomyr commented 10 months ago

When it comes to API best practices, you can certainly refer to our blog: https://redocly.com/blog/. Maybe @lornajane or @TaylorKrusen have something else to add?

lornajane commented 10 months ago

I don't have any specific resources to recommend (there's a blog post coming but it's not published yet), but I think this is a really valuable discussion, thanks @ootkin !

The join command should not transform the OpenAPI descriptions, it should just combine them. We do have the tools for you to make the changes to the specs with some decorators, and it would make sense to put it all in one pipeline and decorate and then join the updated API descriptions.

The custom plugins are the right way to do this - I wonder if we need to create and share a "prefix-all-paths" decorator in the cookbook, which would solve half of this problem. The operationIds would still need to be made unique across the whole API surface though. And I think either the path prefix or some tagging could be used to prompt the SDK generators to group things into modules like in the original example.

ootkin commented 10 months ago

For the operationId, I think it would be useful to lauch a decorator before the join that checks if there are conflicts in the operationId field and then update them with a unique prefix or something like that.

ootkin commented 10 months ago

Thanks! I have to think this through. I'm not sure what logic should apply if there are several servers (e.g. test ones) that could contain different pathnames (or even have no pathname). Also, server could contain server variables, so they should be put somewhere into parameters instead.

Meanwhile, you can use a custom plugin decorator as a workaround. The idea is to go through each OpenAPI description and remove the pathname from the server url (assuming that they contain only one server or it's agreed to use e.g. the first one) and prepends each path item with it, something like this:

      const DecoratePaths = () => {
        let serverPathname
        return {
          Server: {
            enter(server) {
              serverPathname = new URL(server.url).pathname
              server.url = new URL(server.url).origin
            },
          },
          Root: {
            leave(root) {
              const newPaths = {}
              for (let path in root.paths) {
                newPaths[serverPathname + path] = root.paths[path]
              }
              root.paths = newPaths
            }
          }
        };
      },

And then you can join the bundled files into one.

More examples of the decorators in the Redocly CLI Cookbook.

Please let me know if that helps.

As you said, It's a complicated logic if you have multiple servers and servers vars. So I tried to think another "custom" solution and this is my idea:

First of all, I created a main.yaml file that contains all the shared info:

openapi: 3.1.0
info:
  title: JoinedSpec
  version: 1.0.0
servers:
  - url: "http://{baseUrl}"
    variables:
      baseUrl:
        default: api.production.example.com
        enum:
          - api.production.example.com
          - api.development.example.com
paths: {}

After that each service spec should specify a custom prefix with an extension like this:

user.yaml

x-ingress-prefix: user # <-- this
openapi: 3.1.0
info:
  title: User microservice
  version: 1.0.1
paths:
  /healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - users

product.yaml

x-ingress-prefix: product # <-- this
openapi: 3.1.0
info:
  title: Product microservice
  version: 1.1.1
paths:
  /healthz:
    get:
      operationId: healthz
      summary: Get the status of the service
      tags:
        - products

Then, use a preprocessor like this:

module.exports = addIngressPrefix;

/** @type {import('@redocly/cli').OasDecorator} */
function addIngressPrefix() {
  return {
    Root: {
      leave(root) {
        const prefix = root["x-ingress-prefix"];
        if (!prefix) {
          return;
        }

        const newPaths = {};
        for (let path in root.paths) {
          newPaths[`/${prefix}${path}`] = root.paths[path];
        }
        root.paths = newPaths;
      },
    },
  };
}

And then: npx @redocly/cli join main.yaml user.yaml product.yaml --preprocess.

tatomyr commented 10 months ago

I wouldn't recommend using preprocessors both when bundling and joining (and we're going to deprecate them in the join command anyway) as they interfere with linting and are intended to be used only in rare specific cases.

checks if there are conflicts in the operationId field and then update them with a unique prefix

I'd prefix them anyway as there's no guarantee they won't change later. And if your intent is to use the joined API description in an SDK, it's probably safer (as there will be fewer changes when the necessity of the namespacing appears).

As @lornajane said, it's generally better to bundle and decorate each API description in isolation, and only then join.

ootkin commented 10 months ago

I modified my pipeline like this:

      - name: Bundle specs
        run: npx @redocly/cli bundle -o dist openapi/main.yaml openapi/services/*

      - name: Join specs
        run: npx @redocly/cli join ./dist/main.yaml ./dist/*.yaml -o openapi/openapi.yaml

The first step is to bundle all the openapi spec in order to use decorators, and then marge them together.

After that i created 2 decorators: `addIngressPrefix.js´:

module.exports = addIngressPrefix;

/** @type {import('@redocly/cli').OasDecorator} */
function addIngressPrefix() {
  console.log("Updating paths with prefix...");
  return {
    Root: {
      leave(root) {
        let prefix = root["x-ingress-prefix"];
        if (!prefix) {
          return;
        }

        prefix = prefix.replace(/^\/+/g, "");

        const newPaths = {};
        for (let path in root.paths) {
          newPaths[`/${prefix}${path}`] = root.paths[path];
        }
        root.paths = newPaths;
      },
    },
  };
}

addOperationIdPrefix.js:

module.exports = addOperationIdPrefix;

/** @type {import('@redocly/cli').OasDecorator} */
function addOperationIdPrefix() {
    console.log("updating OperationIds ... ");
    let prefix;
    return {
        Root: {
            enter(root) {
                let ingressPrefix = root["x-ingress-prefix"];
                if (!ingressPrefix) {
                    return;
                }
                prefix = ingressPrefix.replace(/^\/+/g, "");
            }
        },
        Operation: {
            leave(target) {
                if (!prefix) {
                    return;
                }

                const {operationId: id} = target;
                if(id) {
                    const capitalized = id.charAt(0).toUpperCase() + id.slice(1)
                    target.operationId = `${prefix}${capitalized}`;
                }
            }
        },
    }
};

And this is my redocly.yaml:

apis:
  myapi:
    root: openapi/openapi.yaml
decorators:
  plugins/add-ingress-prefix: on
  plugins/add-operation-id-prefix: on
extends:
  - recommended
plugins:
  - "./plugins/plugin.js"
tatomyr commented 10 months ago

Good job @ootkin! Could you add your case to our Cookbook repo? Do you still think there would be a benefit of extending the join command functionality?