Feature/#22 open api 3 spec #63

maxiejbe · 2020-08-06T00:50:15Z

Adding swagger2openapi package
Generating OpenApiV3 spec (Serving in api-docs/v3 and api-spec/v3). Preserving default routes with v2.
Note that if you change default to specV3 we need to adjust all the tests.
Creating _v3.json with OpenApiV3 spec
Writing files sync (to discuss)

coveralls · 2020-08-06T00:51:37Z

Coverage increased (+0.4%) to 93.103% when pulling 5387df5 on maxiejbe:feature/#22-open-api-3-spec into 1b8755b on mpashkovskiy:master.

maxiejbe · 2020-08-06T00:59:19Z

@mpashkovskiy Security check is failing 😕. All tests passing on my local. Would you double check?

mpashkovskiy · 2020-08-06T08:40:41Z

I think execution of npm audit will report a vulnurability in swagger2openapi. I think they have to upgrade ajv dependency to 6.x version (it is a major version upgrade). Let's wait a little.

mpashkovskiy · 2020-08-06T08:48:14Z

index.js

@@ -141,14 +145,19 @@ function serveApiDocs() {
  spec.definitions = mongooseModelsSpecs || {};
  updateSpecFromPackage();
  spec = patchSpec(predefinedSpec);
-  app.use(packageInfo.baseUrlPath + '/api-spec', (req, res, next) => {


Is it possible to have next line like specV3 = convertOpenApiVersionToV3(spec); and then serve it separately as described in the comment in the issue?

maxiejbe · 2020-08-06T15:10:21Z

I think execution of npm audit will report a vulnurability in swagger2openapi. I think they have to upgrade ajv dependency to 6.x version (it is a major version upgrade). Let's wait a little.

Yes, in the meanwhile I've installed the ajv 5x peer dependency.

maxiejbe · 2020-08-08T12:16:55Z

I think execution of npm audit will report a vulnurability in swagger2openapi. I think they have to upgrade ajv dependency to 6.x version (it is a major version upgrade). Let's wait a little.

Made a PR yesterday, it was dupe:
Mermade/oas-kit#261
Mermade/oas-kit#262
They are planning to ship a major version removing ajv completely. Should we wait for that?

mpashkovskiy · 2020-08-10T12:21:03Z

If there is no rush I would wait for oas-kit to fix the issue.

mpashkovskiy · 2020-08-10T12:35:27Z

Shouldn't we add some extra assertions in existing tests fo make sure that v3 spec is generated along with v2?
Overall the PR looks good.

kiprasmel · 2020-08-10T15:50:13Z

If there is no rush I would wait for oas-kit to fix the issue.

fyi, I've been using yarn (v1) over npm for over a year now for a reason. Apart from being overall better, it can solve this issue with module resolutions.

matveypashkovskiy · 2020-08-11T14:01:14Z

If there is no rush I would wait for oas-kit to fix the issue.

fyi, I've been using yarn (v1) over npm for over a year now for a reason. Apart from being overall better, it can solve this issue with module resolutions.

Frankly speaking I doubt that yarn can solve major version dependency upgrade or an actual removal of a dependency as it requires source code changes. Otherwise I heard positive feedback about yarn.

maxiejbe · 2020-08-12T13:58:24Z

Shouldn't we add some extra assertions in existing tests fo make sure that v3 spec is generated along with v2?
Overall the PR looks good.

While adding the test to check that v3 was generated I noted a really nice bug 😅
We were converting once specv2 to specv3 and then serving the same specv3 every time.
So the conversion logic now is now inside swaggerServeMiddleware and applySpecMiddlewares.
But as those middlewares are also used by both v2 and v3 and we have the async callback for v3, I tried to solve both together in a generic elegant way 😄

maxiejbe · 2020-08-12T14:58:32Z

If there is no rush I would wait for oas-kit to fix the issue.

fyi, I've been using yarn (v1) over npm for over a year now for a reason. Apart from being overall better, it can solve this issue with module resolutions.

Yeah, in my experience yarn has plenty of issues with Ubuntu. Now on macOS I can give it another chance!

kiprasmel · 2020-08-12T18:52:42Z

@mpashkovskiy

Frankly speaking I doubt that yarn can solve major version dependency upgrade or an actual removal of a dependency as it requires source code changes. Otherwise I heard positive feedback about yarn.

true - it's not magic afterall:D

@maxiejbe

Yeah, in my experience yarn has plenty of issues with Ubuntu. Now on macOS I can give it another chance!

Strange - I've been using windows, linux (arch linux) for over 2 years and now macos for 1 year, and never had any issues with yarn, it's been great:D

kiprasmel · 2020-08-12T18:54:31Z

.gitignore

 api-spec.json
+api-spec_v3.json


I'd just add test_spec*.json once instead. Same with api-spec*.json.

kiprasmel · 2020-08-13T21:39:43Z

index.js

+
+  const apiSpecBasePath = packageInfo.baseUrlPath.concat('/api-spec');
+  const baseSwaggerServePath = packageInfo.baseUrlPath.concat('/' + swaggerUiServePath);
+


Not sure if the packageInfo will always be defined - above [1] it's being checked before getting accessed - make sure everything's fine:D

[1] https://github.com/mpashkovskiy/express-oas-generator/pull/63/files#diff-168726dbe96b3ce427e7fedce31bb0bcR70-R99

Yep, it is! Before initializing the middlewares prepareSpec is called, which is using updateSpecFromPackage.

kiprasmel · 2020-08-13T21:43:24Z

index.js

+/**
+ * @description Applies spec middlewares
+ */
+function applySpecMiddlewares(version = '') {


I'd document the type of the version variable perhaps

Done on the method signature 👍

kiprasmel · 2020-08-14T18:32:25Z

test/index_tests.js

+          expect(JSON.parse(specV3res.body)).toEqual(specV3);
+          done();
+        });
+      });
    });
  });


I'd consider using beforeEach(async (done) => { to avoid unrelated nesting / promise hell (esp. if we ever wanted to do anything further:D)

I'd consider using beforeEach(async (done) => { to avoid unrelated nesting / promise hell (esp. if we ever wanted to do anything further:D)

Yeah, for now request is using callbacks everywhere. I'll take some time to promisify and use async/await everywhere (maybe when rewriting the tests by deprecating init).

mpashkovskiy · 2020-08-17T06:41:58Z

Just to clarify: accordingly to a comment in oas-kit we are waiting for schemesafe v1.0.0. Right?

maxiejbe · 2020-08-17T11:51:40Z

Just to clarify: accordingly to a comment in oas-kit we are waiting for schemesafe v1.0.0. Right?

Yeah, correct. I'll upgrade swagger2openapi as soon as it ships.

maxiejbe · 2020-09-03T18:56:59Z

@mpashkovskiy Just bumped swagger2openapi to 7.0.0 but there's still a security/snyk test failing. Would you please review? Thanks!

maxiejbe · 2020-09-07T18:14:34Z

@mpashkovskiy @kiprasmel Sorry, there was an unnecessary "ajv": "^5.0.0" dependency. This is ready to be shipped!

mpashkovskiy · 2020-09-08T13:40:47Z

Excellent job, @maxiejbe and @kiprasmel let's test it locally and if no problems found I will publih new version to npm.

mpashkovskiy · 2020-09-08T13:56:34Z

minor bug:

run node server_advanced.js
go to http://localhost:8080/api-docs/v3/
click link Specification JSON

Expected: json will be opened
Observer: empty page with URL http://localhost:8080/api-docs/v3/undefined/api-spec opened

mpashkovskiy · 2020-09-08T13:58:52Z

same behavior for all pages:

maxiejbe · 2020-09-08T14:02:10Z

same behavior for all pages:

http://localhost:8080/api-docs/

http://localhost:8080/api-docs/v2/

http://localhost:8080/api-docs/v3/

Good catch. Was it working previous to this PR?

maxiejbe · 2020-09-08T15:25:36Z

@mpashkovskiy Yep, the Specification json link joins baseUrlPath (from package.json) + /api-spec.

Current issue
When baseUrlPath is undefined
Result: current-path + undefined/api-spec

Possible fix
When baseUrlPath is undefined:
We can use to /api-spec (No base url, adding trailing slash).
The issue with this fix is that it would always go to the v2 (default) version.

Another possibility
What if we also declare the route /api-docs/_version_/api-spec? In that case we could only link to api-spec (without the slash).
It would work with both versions.

What do you think?

mpashkovskiy · 2020-09-08T16:44:34Z

@maxiejbe I created a PR #68, but I completely forgot that we serve now two versions, let me fix it.
I like idea of /api-docs/_version_/api-spec let's see

mpashkovskiy · 2020-09-08T16:56:44Z

done, please review #68

mpashkovskiy · 2020-09-08T16:57:39Z

and I found that there is already /api-spec/v2 and /api-spec/v3 paths so no need to introduce new /api-docs/_version_/api-spec

maxiejbe · 2020-09-08T17:11:15Z

done, please review #68

Done! Just a few details. Haven't tried it locally but looking good.

and I found that there is already /api-spec/v2 and /api-spec/v3 paths so no need to introduce new /api-docs/_version_/api-spec

Yeah, that was only if we needed to include only the corresponding api-spec version. I like the approach of having both v2 and v3 links.

maxiejbe added 5 commits August 5, 2020 21:44

Including swagger2openapi package

9b0192b

predefinedSpec with OpenAPIV3 interface

0c0db5d

Ignoring test open api v3 spec files

346430d

Creating openapi util

5b66d12

Generating open api v3 spec file + converting pre-serve (not enabled)

4e02436

maxiejbe marked this pull request as draft August 6, 2020 00:50

Adding back eslint and typescript local validations

60dcd64

maxiejbe mentioned this pull request Aug 6, 2020

Open Api 3 spec? #22

Closed

maxiejbe marked this pull request as ready for review August 6, 2020 00:58

maxiejbe marked this pull request as draft August 6, 2020 00:58

mpashkovskiy reviewed Aug 6, 2020

View reviewed changes

maxiejbe added 2 commits August 6, 2020 12:06

Installing missing ajv peer dependency

15e26b8

Splitting api-docs and api-spec routes into v2, v3 and default

243ef97

maxiejbe added 4 commits August 6, 2020 12:17

Testing with updated ajv unmet peer dependency

d27748e

Back to previous ajv peer dependency version

45fbfd0

Reverting conditions for better coverage

5f356d8

Removing old comment about basic server

9f2f8a7

Fixing same spec v3 generated every time

78826ce

maxiejbe marked this pull request as ready for review August 12, 2020 16:05

kiprasmel reviewed Aug 12, 2020

View reviewed changes

maxiejbe added 2 commits August 12, 2020 16:09

.gitignore wildcard for test_spec*.json and api-spec*.json

866341e

Using top spec declaration instead of argument for middlewares

cd24492

kiprasmel reviewed Aug 14, 2020

View reviewed changes

Documenting version params for middlewares

4c53dc4

kiprasmel approved these changes Aug 15, 2020

View reviewed changes

maxiejbe mentioned this pull request Aug 15, 2020

Roadmap 1.X + 2.X #66

Open

10 tasks

Checking res.headersSent in basic and advanced examples

a2aa633

maxiejbe added 2 commits September 3, 2020 15:41

Bumps swagger2openapi to 6.2.3

5daa90b

Bumps swagger2openapi to 7.0.0

762b693

Removing unnecessary ajv dependency

5387df5

mpashkovskiy merged commit 742c8d6 into mpashkovskiy:master Sep 8, 2020

maxiejbe deleted the feature/#22-open-api-3-spec branch September 9, 2020 14:14


		const apiSpecBasePath = packageInfo.baseUrlPath.concat('/api-spec');
		const baseSwaggerServePath = packageInfo.baseUrlPath.concat('/' + swaggerUiServePath);

		api-spec.json
		api-spec_v3.json

Feature/#22 open api 3 spec #63

Feature/#22 open api 3 spec #63

Conversation

maxiejbe commented Aug 6, 2020 • edited Loading

coveralls commented Aug 6, 2020 • edited Loading

maxiejbe commented Aug 6, 2020 • edited Loading

mpashkovskiy commented Aug 6, 2020

mpashkovskiy Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

maxiejbe Aug 6, 2020

Choose a reason for hiding this comment

maxiejbe commented Aug 6, 2020

maxiejbe commented Aug 8, 2020 • edited Loading

mpashkovskiy commented Aug 10, 2020

mpashkovskiy commented Aug 10, 2020

kiprasmel commented Aug 10, 2020 • edited Loading

matveypashkovskiy commented Aug 11, 2020

maxiejbe commented Aug 12, 2020 • edited Loading

maxiejbe commented Aug 12, 2020 • edited Loading

kiprasmel commented Aug 12, 2020

kiprasmel Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

maxiejbe Aug 12, 2020

Choose a reason for hiding this comment

kiprasmel Aug 13, 2020

Choose a reason for hiding this comment

maxiejbe Aug 14, 2020

Choose a reason for hiding this comment

kiprasmel Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

maxiejbe Aug 14, 2020

Choose a reason for hiding this comment

kiprasmel Aug 14, 2020

Choose a reason for hiding this comment

maxiejbe Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

mpashkovskiy commented Aug 17, 2020 • edited Loading

maxiejbe commented Aug 17, 2020 • edited Loading

maxiejbe commented Sep 3, 2020 • edited Loading

maxiejbe commented Sep 7, 2020

mpashkovskiy commented Sep 8, 2020

mpashkovskiy commented Sep 8, 2020 • edited Loading

mpashkovskiy commented Sep 8, 2020

maxiejbe commented Sep 8, 2020

maxiejbe commented Sep 8, 2020 • edited Loading

mpashkovskiy commented Sep 8, 2020

mpashkovskiy commented Sep 8, 2020

mpashkovskiy commented Sep 8, 2020

maxiejbe commented Sep 8, 2020 • edited Loading

maxiejbe commented Aug 6, 2020 •

edited

Loading

coveralls commented Aug 6, 2020 •

edited

Loading

maxiejbe commented Aug 6, 2020 •

edited

Loading

mpashkovskiy Aug 6, 2020 •

edited

Loading

maxiejbe commented Aug 8, 2020 •

edited

Loading

kiprasmel commented Aug 10, 2020 •

edited

Loading

maxiejbe commented Aug 12, 2020 •

edited

Loading

maxiejbe commented Aug 12, 2020 •

edited

Loading

kiprasmel Aug 12, 2020 •

edited

Loading

kiprasmel Aug 13, 2020 •

edited

Loading

maxiejbe Aug 14, 2020 •

edited

Loading

mpashkovskiy commented Aug 17, 2020 •

edited

Loading

maxiejbe commented Aug 17, 2020 •

edited

Loading

maxiejbe commented Sep 3, 2020 •

edited

Loading

mpashkovskiy commented Sep 8, 2020 •

edited

Loading

maxiejbe commented Sep 8, 2020 •

edited

Loading

maxiejbe commented Sep 8, 2020 •

edited

Loading