[PROPOSAL] x-extensible-enum generate real extensible type definition

[BurnedMarshal]

Description

Custom extension x-extensible-enum generate a type definition as enum and NonEmptyString composition.

export enum EnumTypeEnum {
...
}

export type EnumType = t.TypeOf<typeof EnumType>;
export const EnumType = t.union(
  [
    enumType<EnumTypeEnum>(EnumTypeEnum, "EnumType"),
    NonEmptyString
  ],
  "ExtensibleEnumType"
);

Motivation and Context

When is required extends an API interface adding new value to an enum, the type decoder generated from the previous specification will fail the decode operation. To avoid this behaviour the new decoder will support the enum defined values and any other NonEmptyString.

How Has This Been Tested?

Unit tests and e2e tests

Screenshots (if appropriate):

Types of changes

• Chore (improvement with no change in the behaviour)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[pagopa-github-bot]

	Warnings
⚠️	Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

• single story: [#123456] my PR title
• multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS

[fabriziopapi]

I do not understand the PRO with this union type: a union between a enum and a string, is a string. When using this type I'll always have to manage both the type, becouse the variable domain is not finite.

[BurnedMarshal]

I do not understand the PRO with this union type: a union between a enum and a string, is a string. When using this type I'll always have to manage both the type, becouse the variable domain is not finite.

The scope of the type PreferredLanguageEnum | (string & INonEmptyStringTag) is the same of (string & INonEmptyStringTag) but it allow to discriminate on Typescript code execution path for the single PreferredLanguageEnum. For example:

export enum EnumTypeEnum {
 "ONE" = "ONE",
 "TWO" = "TWO"
}

export type EnumType = t.TypeOf<typeof EnumType>;
export const EnumType = t.union(
  [
    enumType<EnumTypeEnum>(EnumTypeEnum, "EnumType"),
    NonEmptyString
  ],
  "ExtensibleEnumType"
);

const a = EnumTypeEnum.ONE as EnumType;

switch (a) {
  case EnumTypeEnum.ONE:
    typeof a;  // EnumTypeEnum.ONE
    break;
  default:
    typeof a; // EnumTypeEnum.TWO | (string & NonEmptyString)
}

Business logic MUST take care of other possible values like Zalando guidelines indicate on point 102

[fabriziopapi]

The scope of the type PreferredLanguageEnum | (string & INonEmptyStringTag) is the same of (string & INonEmptyStringTag) but it allow to discriminate on Typescript code execution path for the single PreferredLanguageEnum

Ok, so could be useful to have a sort of "auto-complete" values instead of manage them as string constants.

[AleDore]

Just a general consideration about Zalando guidelines. Zalando sometimes use custom-extensions to manage specific behaviour while dealing with different kinds of edge case in API Design's process. Do we really want to adopt custom-extensions in our specs? I see couple of drawbacks with this choice, last but not least the fact that we are integrating different systems in a complex ecosystem, so we must agree that all systems involved in a particular integration MUST adopt the same guidelines and then provide support with these custom-extensions. It is a strong assumption IMHO

[BurnedMarshal]

Just a general consideration about Zalando guidelines. Zalando sometimes use custom-extensions to manage specific behaviour while dealing with different kinds of edge case in API Design's process. Do we really want to adopt custom-extensions in our specs? I see couple of drawbacks with this choice, last but not least the fact that we are integrating different systems in a complex ecosystem, so we must agree that all systems involved in a particular integration MUST adopt the same guidelines and then provide support with these custom-extensions. It is a strong assumption IMHO

I will do some consideration. Custom extensions are part of the OAS3 specification (ref) so they don't violate the standard. The introduction of an extension should derive from a specific need (before the adoption of OAS3 we use x-one-of to add the oneOf capability to our OAS2). Coming back to this specific case, x-extensible-enum has as mayor benefit avoid to create new API versions any time a new value will be added into the value set.
Nevertheless this PR make the x-extensible-enum working as expected on the generation of type definition from specifications, if we don't want to use extensible enum the enum type is always available and works as expected.

[michaeldisaro]

just a typo "enumm"

[BurnedMarshal]

is not a typo, enum is a reserved word 😅

[michaeldisaro]

Right! But imho that double "mm" is not very good loking. Anyway, it's just style.

[AleDore]

Just a general consideration about Zalando guidelines. Zalando sometimes use custom-extensions to manage specific behaviour while dealing with different kinds of edge case in API Design's process. Do we really want to adopt custom-extensions in our specs? I see couple of drawbacks with this choice, last but not least the fact that we are integrating different systems in a complex ecosystem, so we must agree that all systems involved in a particular integration MUST adopt the same guidelines and then provide support with these custom-extensions. It is a strong assumption IMHO

I will do some consideration. Custom extensions are part of the OAS3 specification (ref) so they don't violate the standard. The introduction of an extension should derive from a specific need (before the adoption of OAS3 we use x-one-of to add the oneOf capability to our OAS2). Coming back to this specific case, x-extensible-enum has as mayor benefit avoid to create new API versions any time a new value will be added into the value set. Nevertheless this PR make the x-extensible-enum working as expected on the generation of type definition from specifications, if we don't want to use extensible enum the enum type is always available and works as expected.

Yep, custom-extensions are allowed by OAS3 specifications :) .I'm bumping discussion about specs because other codegens are more strict on spec parsing so it can be difficult to add support with custom-extensions. BTW I'm wondering about the specs we expose outside our systems.

[balanza]

I strongly disagree with both the need and the implementation.

Implementation
Given types X and Y where X is a specialization (or subtype) of Y, X ∪ Y is equal to Y. That said, an enum E union NonEmptyString will result in NonEmptyString, so what's the point? We can just ignore enum 🤷

In addition, the behavior should be a choice for the client, but it's being declared by the server (which exposes the API spec).

Need
We're saying we want to ignore a specific category of types from being checked. Passing "baz" to a field defined as "foo" | "bar" is no different than passing a string to a number field - it's a misalignment with the specification.

In my opinion, clients can choose either to perform validation on incoming data or not.

Is there a solution in the middle? What would it be? Clients would have to make informed decisions on the kind of misalignment received, to determine whether is acceptable or not. This can only happen with validation rules tailored to every specific case, hence I see no one-size-fits-all behavior.

Maybe it would make sense to generate two decoders- one with enums and one with simple strings - and let the client decide which to use. But it still would be an arbitrary choice as enum is no different type than number or array 🤷

[gunzip]

x-extensible-enum semantics states that the client validation is always optional and that the provided range is just a hint on possible values. imho this comes in handy in switch cases or wherever you need some IDE hints. server validation may be stricter of course. so

generate two decoders- one with enums and one with simple strings

that would be nice, but I won't "let the client choose" instead state (in the docs) that clients MUST use the less strict decoder and the server SHOULD/MUST use the stricter one (I see some parallelism with the exact interface case).

https://en.wikipedia.org/wiki/Robustness_principle

[BurnedMarshal]

Implementation Given types X and Y where X is a specialization (or subtype) of Y, X ∪ Y is equal to Y. That said, an enum E union NonEmptyString will result in NonEmptyString, so what's the point? We can just ignore enum 🤷

You are right, the enum is an helper for the client available directly into the generated code. Otherwise the client should manually copy all the possible declared values taken ad example from the description (ref) IMHO an error prone strategy (error into the API definition, error on the client implementation).

In addition, the behavior should be a choice for the client, but it's being declared by the server (which exposes the API spec).

The generated code is for the client. The client must handle the "other value" case and the generated type helps the developers to do that.

[gunzip]

The generated code is for the client

don't we use these types on the server part as well?

[michaeldisaro]

Following the whole discussion I think that's right desiring to have an extensible enum that works as extensible, but I agree with others saying that we should not "downgrading" it to a simple NonEmptyString. @balanza had a nice idea about 2 decoders. That could become handy.

[BurnedMarshal]

The generated code is for the client

don't we use these types on the server part as well?

Yes, sorry. Not always the encoder, but mostly the type.

[BurnedMarshal]

Following some previous comments, I've changed the generation when the strict option is provided. So x-extensible-enum generate a not extensible type and decoder for server side that will use the strict option.
cc @gunzip @balanza @michaeldisaro