Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OAS3.1 for joining (fixes #1021) #1191

Merged
merged 8 commits into from
Aug 8, 2023
Merged

Conversation

bcoughlan
Copy link
Contributor

@bcoughlan bcoughlan commented Jul 26, 2023

What/Why/How?

Currently, all features except "join" support OAS3.1.

Reference

Closes #1021

Testing

Added a unit test.

Screenshots (optional)

Check yourself

  • Code is linted
  • Tested with redoc/reference-docs/workflows - not sure what this means. I have ran it with my own 3.1 specs.
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines - not sure.

@bcoughlan bcoughlan requested a review from a team as a code owner July 26, 2023 08:07
@RomanHotsiy RomanHotsiy requested a review from tatomyr July 26, 2023 08:27
@tatomyr
Copy link
Contributor

tatomyr commented Jul 26, 2023

Hi @bcoughlan,

Thanks for your contribution!
Could you ensure that we're joining only definitions with the same version? At least, I see no point in joining OAS 3.0 with 3.1.
Also, there's a difference between 3.0 & 3.1 in terms of joining - whilst in 3.0 we use x-webhooks, in 3.1 it should be just webhooks (see the docs). It would be great if you could handle this difference.

Cheers!

@bcoughlan
Copy link
Contributor Author

bcoughlan commented Jul 26, 2023

@tatomyr I've added the check to ensure all definitions are the same version.

I'll investigate the x-webhooks change also and get back to you. I'm tempted to just const openapiXWebhooks = openapi['x-webhooks'] ?? openapi['webhooks'] but I imagine it's more involved than that.

@bcoughlan
Copy link
Contributor Author

bcoughlan commented Jul 26, 2023

diff --git a/packages/cli/src/commands/join.ts b/packages/cli/src/commands/join.ts
index 8ceaed16..9af85b7d 100644
--- a/packages/cli/src/commands/join.ts
+++ b/packages/cli/src/commands/join.ts
@@ -29,7 +29,7 @@ import {
   sortTopLevelKeysForOas,
 } from '../utils';
 import { isObject, isString, keysOf } from '../js-utils';
-import { Oas3Parameter, Oas3PathItem, Oas3Server } from '@redocly/openapi-core/lib/typings/openapi';
+import { Oas3Parameter, Oas3PathItem, Oas3Server, Oas3_1Definition } from '@redocly/openapi-core/lib/typings/openapi';
 import { OPENAPI3_METHOD } from './split/types';
 import { BundleResult } from '@redocly/openapi-core/lib/bundle';
 
@@ -141,7 +141,7 @@ export async function handleJoin(argv: JoinOptions, config: Config, packageVersi
     }
   }
 
-  const seenVersions = new Set();
+  const seenVersions : Set<OasVersion> = new Set();
   for (const document of documents) {
     try {
       const version = detectOpenAPI(document.parsed);
@@ -165,6 +165,8 @@ export async function handleJoin(argv: JoinOptions, config: Config, packageVersi
     }
   }
 
+  const outputVersion = Array.from(seenVersions)[0];
+
   if (argv.lint) {
     for (const document of documents) {
       await validateApi(document, config.styleguide, externalRefResolver, packageVersion);
@@ -211,7 +213,7 @@ export async function handleJoin(argv: JoinOptions, config: Config, packageVersi
     collectExternalDocs(openapi, context);
     collectPaths(openapi, context);
     collectComponents(openapi, context);
-    collectXWebhooks(openapi, context);
+    collectXWebhooks(outputVersion, openapi, context);
     if (componentsPrefix) {
       replace$Refs(openapi, componentsPrefix);
     }
@@ -585,10 +587,11 @@ export async function handleJoin(argv: JoinOptions, config: Config, packageVersi
   }
 
   function collectXWebhooks(
-    openapi: Oas3Definition,
+    outputVersion: OasVersion,
+    openapi: Oas3_1Definition,
     { apiFilename, api, potentialConflicts, tagsPrefix, componentsPrefix }: JoinDocumentContext
   ) {
-    const xWebhooks = 'x-webhooks';
+    const xWebhooks = outputVersion == OasVersion.Version3_1 ? 'webhooks' : 'x-webhooks';
     const openapiXWebhooks = openapi[xWebhooks];
     if (openapiXWebhooks) {
       if (!joinedDef.hasOwnProperty(xWebhooks)) {

Let me know if I'm gone off the tracks here. If it's ok I can tidy it up and add tests.

@tatomyr
Copy link
Contributor

tatomyr commented Jul 27, 2023

@bcoughlan I believe this it the way (at least for most definitions). If you want, you may double check the test set I've added to the original issue: #1021 (comment) (there are definitions with webhooks and with x-webhooks).
The only thing, I'd suggest renaming the collectXWebhooks function to collectWebhooks (and also removing the x prefix in other variables).

@bcoughlan
Copy link
Contributor Author

@tatomyr I've updated the branch with the webhooks changes, and I've tested it manually also with some sample specs.

Copy link
Contributor

@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small suggestion. Overall it looks good!

packages/cli/src/commands/join.ts Outdated Show resolved Hide resolved
@tatomyr
Copy link
Contributor

tatomyr commented Jul 31, 2023

Thank you again @bcoughlan!
I have to finish one more chore before we can merge this. I'll get back to you soon.

@tatomyr
Copy link
Contributor

tatomyr commented Aug 7, 2023

Hi @bcoughlan,

We've just updated our publishing process (as well as our contributing guide). Could you also add a changeset to your PR?

Here's the instruction:

Update your branch with the latest main
Run npm i && npx changeset
Choose to include the @redocly/cli package with a minor bump
Provide a short description, e.g. 'Added join support for OAS 3.1 definitions.'
Added join support for OAS 3.1 definitions.
Commit the created file.

Thanks!

@changeset-bot
Copy link

changeset-bot bot commented Aug 7, 2023

🦋 Changeset detected

Latest commit: 1726222

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/cli Minor
@redocly/openapi-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bcoughlan
Copy link
Contributor Author

Done. The changeset file looks ok, although the comment from the bot has an extra package.

@RomanHotsiy
Copy link
Member

@bcoughlan it's expected. Thanks 🙏

@tatomyr tatomyr merged commit 2d7b87c into Redocly:main Aug 8, 2023
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OAS3.1 for joining
3 participants