From 003b0e7a4e13c6d2e95707b876afa79a8d851b46 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 6 Oct 2022 17:01:12 +0200 Subject: [PATCH] #1391: ensure all tags are valid (#2124) ## What This PR does two things: 1. Add tests to ensure that all OpenAPI tags are listed in the root-level `tags` property of the spec. 2. Changes the tags for endpoints that use any of the deprecated tags (specifically `admin`). ## Why When we moved to using the docusaurus OpenAPI doc generator, we made some changes to existing tags. Moving away from having just 'admin', 'client', and one or two more, we now use a more granular system, which makes it easier to find an endpoint by category. However, there's currently nothing enforcing this and this bit of knowledge hasn't been clearly communicated to anyone. Because of this, we have some endpoints that are currently just grouped under a general 'admin' tag. This has two drawbacks: - In Swagger UI, all these endpoints are grouped together. This means that they break with the expectation that each endpoint has its own category. Further, being lumped together means that the 'admin' group is hard to read. - The 'admin' tag is (on purpose) not included in the root-level `tags` property of the generated OpenAPI spec. This means that the OpenAPI docusaurus generator doesn't pick up those endpoints, which means that they're not included in the docs on docs.getunleash.io. ## How By implementing tests that: 1. Check that the tags included in the spec are a subset of the "approved" tags 2. By checking each path in the OpenAPI spec and making sure that all its tags are listed in the root-level tag list. If this is not the case, the test fails and it logs an error explaining what is wrong to the console. An example of such a message is: ``` The OpenAPI spec contains path-level tags that are not listed in the root-level tags object. The relevant paths, operation ids, and tags are as follows: POST /api/admin/feedback (operation id: createFeedback) has the following invalid tags: "admin" PUT /api/admin/feedback/{id} (operation id: updateFeedback) has the following invalid tags: "admin" For reference, the root-level tags are: "Addons", "Admin UI", "API tokens", "Archive", "Auth", "Client", "Context", "Edge", "Environments", "Events", "Features", "Import/Export", "Metrics", "Operational", "Playground", "Projects", "Public signup tokens", "Strategies", "Tags", "Unstable", "Users" ``` ## Commits * fix: ensure that all root-level tags in the spec are 'approved tags' * fix: test that all listed path tags exist in the root-level tags * fix: use "API tokens" tag for PAT endpoints * fix: add comment explaining why tags test is there * fix: Update snapshot * fix: ensure that spec tags are a subset of the approved Tags * fix: improve error message when tags don't match * fix: further tweaks in log format --- src/lib/openapi/util/openapi-tags.ts | 7 +- src/lib/routes/admin-api/user/pat.ts | 6 +- .../__snapshots__/openapi.e2e.test.ts.snap | 6 +- src/test/e2e/api/openapi/openapi.e2e.test.ts | 91 +++++++++++++++++++ 4 files changed, 103 insertions(+), 7 deletions(-) diff --git a/src/lib/openapi/util/openapi-tags.ts b/src/lib/openapi/util/openapi-tags.ts index 0647955969..e04a5ed4a7 100644 --- a/src/lib/openapi/util/openapi-tags.ts +++ b/src/lib/openapi/util/openapi-tags.ts @@ -1,3 +1,6 @@ +// The "official" OpenAPI tags that we use. These tags are added to the OpenAPI +// spec as the root-level "tags" list. Consider creating a new entry here when +// creating a new endpoint. const OPENAPI_TAGS = [ { name: 'Addons', @@ -90,4 +93,6 @@ export const openApiTags = [...OPENAPI_TAGS].sort((a, b) => a.name.localeCompare(b.name), ); -export type OpenApiTag = typeof OPENAPI_TAGS[number]['name']; +export type OpenApiTag = + // The official OpenAPI tags that we use. + typeof OPENAPI_TAGS[number]['name']; diff --git a/src/lib/routes/admin-api/user/pat.ts b/src/lib/routes/admin-api/user/pat.ts index bb3dde1999..c255fb63c8 100644 --- a/src/lib/routes/admin-api/user/pat.ts +++ b/src/lib/routes/admin-api/user/pat.ts @@ -39,7 +39,7 @@ export default class PatController extends Controller { permission: NONE, middleware: [ openApiService.validPath({ - tags: ['admin'], + tags: ['API tokens'], operationId: 'getPats', responses: { 200: createResponseSchema('patsSchema') }, }), @@ -52,7 +52,7 @@ export default class PatController extends Controller { permission: NONE, middleware: [ openApiService.validPath({ - tags: ['admin'], + tags: ['API tokens'], operationId: 'createPat', requestBody: createRequestSchema('patSchema'), responses: { 200: createResponseSchema('patSchema') }, @@ -68,7 +68,7 @@ export default class PatController extends Controller { permission: NONE, middleware: [ openApiService.validPath({ - tags: ['admin'], + tags: ['API tokens'], operationId: 'deletePat', responses: { 200: emptyResponse }, }), diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 4acf2c079a..d47e435f29 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -7018,7 +7018,7 @@ If the provided project does not exist, the list of events will be empty.", }, }, "tags": [ - "admin", + "API tokens", ], }, "post": { @@ -7047,7 +7047,7 @@ If the provided project does not exist, the list of events will be empty.", }, }, "tags": [ - "admin", + "API tokens", ], }, }, @@ -7070,7 +7070,7 @@ If the provided project does not exist, the list of events will be empty.", }, }, "tags": [ - "admin", + "API tokens", ], }, }, diff --git a/src/test/e2e/api/openapi/openapi.e2e.test.ts b/src/test/e2e/api/openapi/openapi.e2e.test.ts index 9f4ee011a1..732d270156 100644 --- a/src/test/e2e/api/openapi/openapi.e2e.test.ts +++ b/src/test/e2e/api/openapi/openapi.e2e.test.ts @@ -4,6 +4,7 @@ import getLogger from '../../../fixtures/no-logger'; import SwaggerParser from '@apidevtools/swagger-parser'; import enforcer from 'openapi-enforcer'; import semver from 'semver'; +import { openApiTags } from '../../../../lib/openapi/util/openapi-tags'; let app; let db; @@ -100,3 +101,93 @@ test('the generated OpenAPI spec is valid', async () => { expect(enforcerWarning ?? enforcerError).toBe(undefined); }); + +test('all root-level tags are "approved tags"', async () => { + const { body: spec } = await app.request + .get('/docs/openapi.json') + .expect('Content-Type', /json/) + .expect(200); + + const specTags = spec.tags; + const approvedTags = openApiTags; + + // expect spec tags to be a subset of the approved tags + expect(approvedTags).toEqual(expect.arrayContaining(specTags)); +}); +// All tags that are used for OpenAPI path operations must also be listed in the +// OpenAPI root-level tags list. For us, there's two immediate things that make +// this important: +// +// 1. Swagger UI groups operations by tags. To make sure that endpoints are +// listed where users would expect to find them, they should be given an +// appropriate tag. +// +// 2. The OpenAPI/docusaurus integration we use does not generate documentation +// for paths whose tags are not listed in the root-level tags list. +// +// If none of the official tags seem appropriate for an endpoint, consider +// creating a new tag. +test('all tags are listed in the root "tags" list', async () => { + const { body: spec } = await app.request + .get('/docs/openapi.json') + .expect('Content-Type', /json/) + .expect(200); + + const rootLevelTagNames = new Set(spec.tags.map((tag) => tag.name)); + + // dictionary of all invalid tags found in the spec + let invalidTags = {}; + for (const [path, data] of Object.entries(spec.paths)) { + for (const [operation, opData] of Object.entries(data)) { + // ensure that the list of tags for every operation is a subset of + // the list of tags defined on the root level + + // check each tag for this operation + for (const tag of opData.tags) { + if (!rootLevelTagNames.has(tag)) { + // store other invalid tags that already exist on this + // operation + const preExistingTags = + (invalidTags[path] ?? {})[operation]?.invalidTags ?? []; + + // add information about the invalid tag to the invalid tags + // dict. + invalidTags = { + ...invalidTags, + [path]: { + ...invalidTags[path], + [operation]: { + operationId: opData.operationId, + invalidTags: [...preExistingTags, tag], + }, + }, + }; + } + } + } + } + + if (Object.keys(invalidTags).length) { + // create a human-readable list of invalid tags per operation + const msgs = Object.entries(invalidTags).flatMap(([path, data]) => + Object.entries(data).map( + ([operation, opData]) => + `${operation.toUpperCase()} ${path} (operation id: ${ + opData.operationId + }) has the following invalid tags: ${opData.invalidTags + .map((tag) => `"${tag}"`) + .join(', ')}`, + ), + ); + + // format message + const errorMessage = `The OpenAPI spec contains path-level tags that are not listed in the root-level tags object. The relevant paths, operation ids, and tags are as follows:\n\n${msgs.join( + '\n\n', + )}\n\nFor reference, the root-level tags are: ${spec.tags + .map((tag) => `"${tag.name}"`) + .join(', ')}`; + + console.error(errorMessage); + } + expect(invalidTags).toStrictEqual({}); +});