Fix V2 parameter types #489

yamacent · 2021-02-26T12:28:58Z

close #470

drwpow · 2021-02-27T14:55:56Z

Thanks so much! I’ll review it soon

yamacent · 2021-02-27T17:40:12Z

@drwpow Thanks for you reply! I'll modify tests as well if the direction of this PR is okay.

drwpow · 2021-03-02T16:08:27Z

src/transform/type.ts

+}
+
+function transformItemsObj(obj: ItemsObject): string {
+  switch (obj.type) {


This code actually already exists in utils.ts#L29. So by re-adding different logic, we could introduce bugs.

drwpow · 2021-03-02T16:16:04Z

Thanks so much for your PR! I think the fix is much easier than expected. We’re using the same code for OpenAPI 3.0 and Swagger 2.0, and the unknown is happening because it’s trying to parse everything as 3.0 (it’s looking for [parameter].schema.type, when it should be looking for just [parameter].type). All we need to change is one line on parameters.ts#L43.

- paramObj.schema ? transformSchemaObj(paramObj.schema) : "unknown"
+ if (version === 2) {
+  paramObj.type ? transformSchemaObj(paramObj) : "unknown"
+ } else if (version === 3) {
+   paramObj.schema ? transformSchemaObj(paramObj.schema) : "unknown"
+ }

So that’s step 1. Step 2 would be to pass that version from transform/index.ts into transform/paths.ts and finally into transform/parameters.ts. That should fix it!

Also, I find it best to start by actually making a new test in parameters.test.ts first, and then working until it passes.

yamacent · 2021-03-03T14:29:08Z

@drwpow
Thank you for your review and kind explanation!
I added tests to parameters.test.ts and fixed the code to pass the test as you say.
Please review again.
Also, could you please tell me how to fix tests in tests/v2 directory?

drwpow · 2021-03-04T04:05:03Z

Also, could you please tell me how to fix tests in tests/v2 directory?

Ah so you know what? This problem is a bit trickier than expected. I seemed to miss this critical part of the Open API 2.0 specs that states that we only use the top-level object if in is NOT body. Otherwise, we do use schema if in is body.

Will comment a fix on your PR and I think we can wrap this up!

drwpow · 2021-03-04T04:08:12Z

src/transform/parameters.ts

        mappedParams[reference.in][reference.name || paramName] = {
          ...reference,
-          schema: { $ref: paramObj.$ref },
+          ...(version === 2 ? schema : version === 3 ? { schema } : {}),


So because this is a bit difficult to follow, let’s take up a little more space:

if (version === 2) { mappedParams[reference.in][reference.name || paramName] = { ..reference, $ref: paramObj.$ref, } as any; } else if (version === 3) { mappedParams[reference.in][reference.name || paramName] = { ..reference, schema: { $ref: paramObj.$ref }, } as any; }

Your version takes up fewer lines, and is good in that regard. But you have a nested ternary + a spread operator, and it’s very easy for someone to make a mistake (if not you, the next person to touch this).

This version is more verbose, but it’s a bit clearer.

I agree with you. Fixed in 8c02bd7

Perhaps we need to consider the case where in is body here as well?

drwpow · 2021-03-04T04:12:36Z

src/transform/parameters.ts

-      };\n`;
+      let type = ``;
+      if (version === 2) {
+        type = paramObj.type ? transformSchemaObj(paramObj) : "unknown";


So according to the OpenAPI 2.0 spec, this will cause a regression, which is why the V2 tests are failing (to your credit, I think the specs are very confusing here, and I suspect it’s why they changed it for 3.0 🙂 ).

Instead, this needs to be:

Suggested change

type = paramObj.type ? transformSchemaObj(paramObj) : "unknown";

if (paramObj.in === 'body' && paramObj.schema) {

type = transformSchemaObj(paramObj.schema);

} else if (paramObj.type) {

type = transformSchemaObj(paramObj);

} else {

type = 'unknown';

}

Thank you! I missed the spec.. fixed in fe3b69b

drwpow · 2021-03-04T04:13:31Z

src/transform/parameters.ts

-      output += `    "${paramName}"${required}: ${
-        paramObj.schema ? transformSchemaObj(paramObj.schema) : "unknown"
-      };\n`;
+      let type = ``;


nit: can we use another name, other than type? Only because type is a special word in TypeScript (it doesn’t seem to be causing any problems here, but just for clarity)

I see! I renamed it to paramType. bb668ed

drwpow · 2021-03-04T04:13:46Z

src/transform/parameters.ts

+      if (version === 2) {
+        type = paramObj.type ? transformSchemaObj(paramObj) : "unknown";
+      } else if (version === 3) {
+        type = paramObj.schema ? transformSchemaObj(paramObj.schema) : "unknown";


👍🏻 I believe this is still correct.

drwpow · 2021-03-04T04:14:05Z

tests/parameters.test.ts

-            items: {
-              type: "string",
+describe.only("transformParametersArray()", () => {
+  describe("v2", () => {


Nice tests! 💯

drwpow · 2021-03-04T04:15:08Z

tests/parameters.test.ts

-      ]).trim()
-    ).toBe(
-      `query: {
+            { in: "path", name: "three_d_secure", required: true, type: "string" },


Suggestion: with our new knowledge, let’s try adding a body param here, like: { in: "body", schema: { type: "string" } }. Then we can test if it generated correctly.

fix openapi-ts#489 (comment)

yamacent · 2021-03-04T07:30:04Z

@drwpow By the way, do we need to keep the changes I did in src/types/index.ts?

drwpow · 2021-03-07T04:36:42Z

src/types/index.ts

 export interface ParameterObject {
  name?: string; // required
  in?: "query" | "header" | "path" | /* V3 */ "cookie" | /* V2 */ "formData" | /* V2 */ "body"; // required
  description?: string;
  required?: boolean;
  deprecated?: boolean;
  schema?: ReferenceObject | SchemaObject; // required
+  type?: "string" | "number" | "integer" | "boolean" | "array" | "file"; // V2 ONLY
+  items?: ItemsObject; // V2 ONLY


This should be ReferenceObject | SchemaObject I believe.

drwpow · 2021-03-07T04:37:45Z

src/types/index.ts

@@ -60,13 +60,22 @@ export interface OperationObject {
  responses?: Record<string, ReferenceObject | ResponseObject>; // required
 }

+export interface ItemsObject {


I believe we can remove this; this seems to be a duplication of SchemaObject but with fewer fields (see SchemaObject["items"] as a reference

drwpow · 2021-03-07T04:43:00Z

This is looking great! It looks like the only thing keeping the tests from passing are to take tests/v2/generated/*.ts and copy them over to tests/v2/expected/*.ts. Basically these are “snapshot” tests that test very, very large schemas.

These snapshot tests are bad at testing individual features because there are so many lines. But they are good at making sure that complete schemas are generating correctly. I believe all the changes I see point to your PR working as intended; it looks like we are now at an improvement with no other regressions!

I’ll go ahead and approve this, and as soon as you copy the files to tests/v2/expected/*.ts the tests should be working. Then we can merge & release this!

codecov · 2021-03-08T05:08:42Z

Codecov Report

Merging #489 (ce97089) into main (3a20869) will increase coverage by 0.21%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
+ Coverage   84.61%   84.82%   +0.21%     
==========================================
  Files           9        9              
  Lines         312      323      +11     
  Branches       98      105       +7     
==========================================
+ Hits          264      274      +10     
- Misses         45       46       +1     
  Partials        3        3

Impacted Files	Coverage Δ
src/transform/index.ts	`56.60% <50.00%> (ø)`
src/transform/parameters.ts	`97.43% <92.85%> (-2.57%)`	⬇️
src/transform/operation.ts	`96.42% <100.00%> (ø)`
src/transform/paths.ts	`91.66% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a20869...ce97089. Read the comment docs.

yamacent · 2021-03-08T05:12:38Z

@drwpow
I fixed types where you pointed out ( a5b9c79 ) and updated tests/v2/expected/*.ts as you said ( 2e9f004 ).
Please confirm.

drwpow · 2021-03-08T06:06:42Z

This looks great, thank you!

drwpow · 2021-03-08T06:07:15Z

@all-contributors please add @yamacent for code, bug, test

allcontributors · 2021-03-08T06:07:23Z

@drwpow

I've put up a pull request to add @yamacent! 🎉

yamacent · 2021-03-08T06:15:52Z

@drwpow Once again, thank you for your kind review!

drwpow · 2021-03-09T19:22:46Z

This was just released in 3.1.0! 🚀 Thank you again for your contribution.

Fix V2 parameter types

2598f10

yamacent force-pushed the fix-v2-parameter-types branch from ea0a6f4 to 2598f10 Compare February 26, 2021 12:36

drwpow reviewed Mar 2, 2021

View reviewed changes

Fix V2 parameter types

424f91f

drwpow reviewed Mar 4, 2021

View reviewed changes

drwpow mentioned this pull request Mar 4, 2021

openapi 2: parameters with known type parsed to unknown #470

Closed

yamacent added 4 commits March 4, 2021 15:04

Make it readable

8c02bd7

fix openapi-ts#489 (comment)

Use schema object when in is body

fe3b69b

fix openapi-ts#489 (comment)

Rename variable

bb668ed

fix openapi-ts#489 (comment)

Fix bug

fa5fcfa

drwpow reviewed Mar 7, 2021

View reviewed changes

yamacent added 3 commits March 8, 2021 13:31

Fix types

a5b9c79

Update v2 snapshot tests

2e9f004

Merge remote-tracking branch 'upstream/main' into fix-v2-parameter-types

ce97089

drwpow merged commit 927e526 into openapi-ts:main Mar 8, 2021

allcontributors bot mentioned this pull request Mar 8, 2021

docs: add yamacent as a contributor #511

Merged

yamacent deleted the fix-v2-parameter-types branch March 8, 2021 06:10

-        type = paramObj.type ? transformSchemaObj(paramObj) : "unknown";
+  if (paramObj.in === 'body' && paramObj.schema) {
+    type = transformSchemaObj(paramObj.schema);
+  } else if (paramObj.type) {
+    type = transformSchemaObj(paramObj);
+  } else {
+    type = 'unknown';
+  }

Uh oh!

Fix V2 parameter types #489

Fix V2 parameter types #489

Uh oh!

Conversation

yamacent commented Feb 26, 2021

Uh oh!

drwpow commented Feb 27, 2021

Uh oh!

yamacent commented Feb 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drwpow commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamacent commented Mar 3, 2021

Uh oh!

drwpow commented Mar 4, 2021

Uh oh!

drwpow Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamacent Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamacent commented Mar 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drwpow commented Mar 7, 2021

Uh oh!

codecov bot commented Mar 8, 2021

Codecov Report

Uh oh!

yamacent commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drwpow commented Mar 8, 2021

Uh oh!

drwpow commented Mar 8, 2021

Uh oh!

allcontributors bot commented Mar 8, 2021

Uh oh!

yamacent commented Mar 8, 2021

Uh oh!

drwpow commented Mar 9, 2021

Uh oh!

Uh oh!

drwpow commented Mar 2, 2021 •

edited

Loading

drwpow Mar 4, 2021 •

edited

Loading

yamacent Mar 4, 2021 •

edited

Loading

yamacent commented Mar 8, 2021 •

edited

Loading