fix/enhance support for "additionalProperties" #86

Kosta-Github · 2019-06-26T12:23:06Z

about additionalProperties usage in swagger see for example this post

additionalProperties can be defined for any object type
they are adding unnamed and potentially typed properties to a type
the pitfall is that not defining an additionalProperties field on an object type needs to be treated as defining it as true (see URL given above)
this means that many object types will add a [key: string]: any; definition which is required according to the swagger spec
there is no dedicated dictionary object or class => dictionary has been removed

Note, that this is a breaking change, since the type template need to adjust to the newly introduced properties hasAdditionalProperties and additionalPropertiesType!

Kosta-Github · 2019-06-26T12:24:12Z

__tests__/__snapshots__/runner.js.snap

-    getInventory(parameters: {} & CommonRequestOptions): Promise < ResponseWithBody < 200, {
-        [key: string]: number
-    } >> {
+    getInventory(parameters: {} & CommonRequestOptions): Promise < ResponseWithBody < 200, object >> {


this will be fixed/enhanced in a separate PR dealing with pre-defined response definitions...

Kosta-Github · 2019-06-26T12:25:45Z

__tests__/__snapshots__/runner.js.snap

+export type test_add_props_12 = {
+
+    [key: string]: some_def;
+};


the types test_add_props_* have been validated according to the description about how to handle the different cases for additionalProperties ...

Kosta-Github · 2019-06-26T12:26:08Z

__tests__/fixtures/addProps/swagger.json

+        "test_add_props_11": { "type": "object", "additionalProperties": { "type": "object", "properties": { "nested_prop": { "type": "string" } } } },
+        "test_add_props_12": { "type": "object", "additionalProperties": { "$ref": "#/definitions/some_def" } }
+    }
+}


these are the new test cases

Kosta-Github · 2019-06-26T12:27:13Z

src/type-mappers/object.ts

+      minItems: 0,
+      title: "any",
+      properties: {}
+    });


I don't know about this one? is there a better way to specify the any type?

I think maybe makeAnyTypeSpec(swaggerType) would work?

Kosta-Github · 2019-06-26T12:29:16Z

templates/type.mustache

 %><%#isObject%>{
-<%#properties%>    '<%name%>'<%^isRequired%>?<%/isRequired%>: <%>type%>;<%/properties%>}<%/isObject%><%!
+<%#properties%>    '<%name%>'<%^isRequired%>?<%/isRequired%>: <%>type%>;<%/properties%><%#hasAdditionalProperties%>
+    [key: string]: <%#additionalPropertiesType%><%>type%><%/additionalPropertiesType%>;<%/hasAdditionalProperties%>}<%/isObject%><%!


use the newly introduced hasAdditionalProperties and additionalPropertiesType fields in the template

about `additionalProperties` usage in `swagger` see for example [this post](https://support.reprezen.com/support/solutions/articles/6000162892-support-for-additionalproperties-in-swagger-2-0-schemas) * `additionalProperties` can be defined for any `object` type * they are adding unnamed and potentially typed properties to a type * the pitfall is that not defining an `additionalProperties` field on an `object` type needs to be treated as defining it as `true` (see URL given above) * this means that many object types will add a `[key: string]: any;` definition which is required according to the swagger spec * there is no dedicated `dictionary` object or class => `dictionary` has been removed Note, that this is a breaking change, since the `type` template need to adjust to the newly introduced properties `hasAdditionalProperties` and `additionalPropertiesType`!

Markionium · 2019-07-01T17:08:33Z

src/type-mappers/object.ts

+      minItems: 0,
+      title: "any",
+      properties: {}
+    });


I think maybe makeAnyTypeSpec(swaggerType) would work?

Kosta-Github · 2019-07-17T10:10:12Z

any opinions on this PR?

Markionium · 2019-07-17T10:27:09Z

@Kosta-Github I didn't merge it yet since i was wondering if you would fix the following. If you don't have a lot of time to do this for now I'm ok to merge it :D

Kosta-Github · 2019-07-17T10:29:30Z

@Kosta-Github I didn't merge it yet since i was wondering if you would fix the following. If you don't have a lot of time to do this for now I'm ok to merge it :D

honestly, I will not have the time right now, I just came back from vacation and need to clean out several tasks accumulated so far...

Markionium · 2019-07-17T10:40:34Z

@Kosta-Github I didn't merge it yet since i was wondering if you would fix the following. If you don't have a lot of time to do this for now I'm ok to merge it :D

honestly, I will not have the time right now, I just came back from vacation and need to clean out several tasks accumulated so far...

Thats fine, in that case i'll merge it. Thanks a bunch for your contributions!

Markionium · 2019-07-24T10:42:06Z

Published as 1.11.0

andy-viv · 2019-08-02T23:28:30Z

@Kosta-Github specifically said this was a breaking change. This should have been published as 2.0.0.

andy-viv · 2019-08-02T23:45:45Z

@Kosta-Github can you provide a migration guide? Or updates to the README?

andy-viv · 2019-08-03T00:27:09Z

src/type-mappers/object.ts

+  }
+  if (
+    swaggerType.additionalProperties === undefined ||
+    swaggerType.additionalProperties === true


If additionalProperties is undefined, wouldn't it make more sense to default to false?

Kosta-Github · 2019-08-03T18:00:12Z

@andy-viv

note that I had mentioned it within the PR description:

Note, that this is a breaking change, since the type template need to adjust to the newly introduced properties hasAdditionalProperties and additionalPropertiesType!
about additionalProperties === undefined: it was a deliberate decision from the swagger team (or JSON schema team?) to treat it like additionalProperties === true; see also: https://support.reprezen.com/support/solutions/articles/6000162892-support-for-additionalproperties-in-swagger-2-0-schemas (which is also contained as a link within the code changes as comment)

Kosta-Github · 2019-08-03T18:12:17Z

@andy-viv re-reading the comments in the link above, I might come to the conclusion that for the swagger only support case, only the additionalProperties: { type: ... } case should be handled, ignoring all other cases possible with JSON schema, just like additionalProperties === undefined | true | false ...

andy-viv · 2019-08-05T18:42:19Z

note that I had mentioned it within the PR description:

@Kosta-Github yes, that criticism was actually directed toward @Markionium.

Also, I figured out why this broke for my project... We had custom templates for type.mustache which i needed to updated.

andy-viv · 2019-08-05T18:54:03Z

@Kosta-Github I still think we should just treat additionalProperties === undefined the same way as additionalProperties === false... I do think supporting additionalProperties === true provides some value in terms of usage in typescript types.

Barring that, I would be okay with only supporting the case where additionalProperties is an object as you said.

Markionium · 2019-08-08T10:22:56Z

Hey @andy-viv, @Kosta-Github ,

I sincerely apologise. I definitely messed this one up, sorry for the inconvenience. I read it was a breaking change when merging the PR. But somehow forgot about it when releasing it a couple days later.

I think it might be worth investing some time in a better process for this.

Just to be clear from our side on what to do. You mitigated the problem? @mtennoe went ahead and published it as 2.0.0, so you can consume that.

We considered un-publishing the version that should have been published as 2.0.0. But that would also break others that took a direct dependency on that version.

Again sorry for the inconvenience,

Mark

andy-viv · 2019-08-22T22:53:33Z

Hey Mark,

Sorry for the late response. No problem. For now I have had to pin to 1.10.4 because we will need to update our custom templates to match the new requirements.

I plan on opening a PR to do what I said in this comment:

we should just treat additionalProperties === undefined the same way as additionalProperties === false

Until then, we're going to leave it pinned to 1.10.4.

Thank you @mtennoe and @Markionium for the work on this project!

as mentioned mtennoe#86 (comment) treat additionalProperties === undefined the same way as additionalProperties === false

Kosta-Github commented Jun 26, 2019

View reviewed changes

Markionium approved these changes Jul 1, 2019

View reviewed changes

Markionium merged commit c92098e into mtennoe:master Jul 17, 2019

andy-viv reviewed Aug 3, 2019

View reviewed changes

mtennoe mentioned this pull request Aug 15, 2019

Types with additionalProperties are not generated correctly #91

Closed

mohofo7 added a commit to mohofo7/swagger-typescript-codegen that referenced this pull request Oct 30, 2021

fix: treat undefined additionalProperties same false

df58a5a

as mentioned mtennoe#86 (comment) treat additionalProperties === undefined the same way as additionalProperties === false

mohofo7 mentioned this pull request Oct 30, 2021

fix: treat undefined additionalProperties same false #131

Open

fix/enhance support for "additionalProperties" #86

fix/enhance support for "additionalProperties" #86

Uh oh!

Conversation

Kosta-Github commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Kosta-Github commented Jul 17, 2019

Uh oh!

Markionium commented Jul 17, 2019

Uh oh!

Kosta-Github commented Jul 17, 2019

Uh oh!

Markionium commented Jul 17, 2019

Uh oh!

Markionium commented Jul 24, 2019

Uh oh!

andy-viv commented Aug 2, 2019

Uh oh!

andy-viv commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kosta-Github commented Aug 3, 2019

Uh oh!

Kosta-Github commented Aug 3, 2019

Uh oh!

andy-viv commented Aug 5, 2019

Uh oh!

andy-viv commented Aug 5, 2019

Uh oh!

Markionium commented Aug 8, 2019

Uh oh!

andy-viv commented Aug 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kosta-Github commented Jun 26, 2019 •

edited

Loading

andy-viv commented Aug 2, 2019 •

edited

Loading