fix/enhance support for "additionalProperties"

Kosta-Github commented 5 years ago

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 commented 5 years ago

any opinions on this PR?

Markionium commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

Published as 1.11.0

andy-viv commented 4 years ago

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

andy-viv commented 4 years ago

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

Kosta-Github commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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!

mtennoe / swagger-typescript-codegen

fix/enhance support for "additionalProperties" #86