papsign / Ktor-OpenAPI-Generator

Ktor OpenAPI/Swagger 3 Generator
Apache License 2.0
241 stars 42 forks source link

Added validation error on parsing enum values outside of valid enum values #113

Closed Szer closed 2 years ago

Szer commented 2 years ago

It fixes https://github.com/papsign/Ktor-OpenAPI-Generator/issues/111 with throwing OpenAPIBadContentException on parsing wrong value.

This validation won't add anything to RouteSelectors, so you have to manually catch error via StatusPage ktor feature to show desired StatusCode and message to the user. Which I think totally fine because current code throws OpenAPIRequiredFieldException on omitting non-nullable query parameter, so this is the same behavior.

Behavior change

Assuming we have enum parameter with name type and valid values: [VALID]

Nullable

BEFORE

| Route        | StatusCode | Parsed Enum Value     |
|--------------|------------|-----------------------|
| /            | 200        | null                  |
| /?type=VALID | 200        | VALID                 |
| /?type=else  | 200        | null                  |

AFTER

| Route        | StatusCode | Parsed Enum Value     |
|--------------|------------|-----------------------|
| /            | 200        | null                  |
| /?type=VALID | 200        | VALID                 |
| /?type=else  | ERROR      |                       | // CHANGED from 200 to ERROR

NON nullable

BEFORE

| Route        | StatusCode | Parsed Enum Value     |
|--------------|------------|-----------------------|
| /            | ERROR      |                       |
| /?type=VALID | 200        | VALID                 |
| /?type=else  | ERROR      |                       |

AFTER

| Route        | StatusCode | Parsed Enum Value     |
|--------------|------------|-----------------------|
| /            | ERROR      |                       |
| /?type=VALID | 200        | VALID                 |
| /?type=else  | ERROR      |                       | // CHANGED type of ERROR

ADDED 20 Oct

Added opt-in behaviour to this feature. Alternative implementations were:

I've decided to go with the second approach just because I was overwhelmed by amount of changes needed for the first approach.

So in this PR I've added new annotation StrictEnumParsing which presence will change behaviour of EnumConverter Because it is new annotation, no behaviour change for existing code.

Wicpar commented 2 years ago

looks good, my only worry would be with API that relied on bad input being null instead of an error, it would be good to have a way to confiure that per type with an annotation. If the ktype retains the annotations properly it would be good to add an annotation, like @IgnoreInvalid MyEnum that parses null in the case of bad values. If it requires an overhaul we'll leave it at that.

Szer commented 2 years ago

Make sense for that behaviour to be opt-in, I look into it.

Szer commented 2 years ago

So I've added opt-in behaviour to this feature. Alternative implementations were

I've decided to go with the second approach just because I was overwhelmed by amount of changes needed for the first approach.

So in this PR I've added new annotation StrictEnumParsing which presence will change behaviour of EnumConverter Because it is new annotation, no behaviour change for existing code.

Szer commented 2 years ago

@Wicpar added opt-in behaviour

Szer commented 2 years ago

@Wicpar gently reminding about this PR :)