Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

Szer
Copy link
Contributor

@Szer Szer commented Oct 19, 2021

It fixes #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:

  • Add new field to ParameterModel. That also would require changes to QueryParam, HeaderParam, PathParam
    Pros: These annotations already exist and they could be placed freely on types you don't own
    Cons: A lot of changes required in builders.

  • Add new annotation which will tell that this enum needs strict parsing
    Pros: Really easy to implement
    Cons: You have to own the type to annotate with new annotation. Can't put it on 3rd party enums

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
Copy link
Collaborator

Wicpar commented Oct 19, 2021

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
Copy link
Contributor Author

Szer commented Oct 19, 2021

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

@Szer
Copy link
Contributor Author

Szer commented Oct 20, 2021

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

  • Add new field to ParameterModel. That also would require changes to QueryParam, HeaderParam, PathParam
    Pros: These annotations already exist and they could be placed freely on types you don't own
    Cons: A lot of changes required in builders.

  • Add new annotation which will tell that this enum needs strict parsing
    Pros: Really easy to implement
    Cons: You have to own the type to annotate with new annotation. Can't put it on 3rd party enums

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
Copy link
Contributor Author

Szer commented Oct 20, 2021

@Wicpar added opt-in behaviour

@Szer
Copy link
Contributor Author

Szer commented Oct 26, 2021

@Wicpar gently reminding about this PR :)

@Wicpar Wicpar merged commit 3b3429f into papsign:master Nov 2, 2021
@Szer Szer deleted the enum-validation branch November 2, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enum parser is okay with wrong values
2 participants