papsign / Ktor-OpenAPI-Generator

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

Enum parser is okay with wrong values #111

Closed Szer closed 2 years ago

Szer commented 2 years ago

I'm using enum query params to limit set of incoming values. Current behaviour allows invalid (outside of enum) values to go through https://github.com/papsign/Ktor-OpenAPI-Generator/blob/f7e7048c945c65526a38efe655fa60e88d0fc19a/src/main/kotlin/com/papsign/ktor/openapigen/parameters/parsers/converters/primitive/EnumConverter.kt#L13

Repro

enum class RequestType{
    VALUE,
    ANOTHER,
}

@Path("/")
data class RequestParams(
    @QueryParam("") val type: RequestType? = null
)

Expected

I expect following queries to be valid

/ -> type=null
/?type=VALUE -> type=RequestType.VALUE
/?type=ANOTHER -> type=RequestType.ANOTHER
/?type=value -> type=RequestType.VALUE
/?type=another -> type=RequestType.ANOTHER

I also expect following queries to be invalid and return validation error

/?type=WRONG_VALUE -> ERROR

Actual

everything is valid, no validation whatsoever Also some (questionably) valid values are giving confusing result /?type=another leads to type=null which could cause bugs in software

/ -> type=null
/?type=VALUE -> type=RequestType.VALUE
/?type=ANOTHER -> type=RequestType.ANOTHER
/?type=value ->  type=null
/?type=another ->  type=null
/?type=WRONG_VALUE ->  type=null
Wicpar commented 2 years ago

Enums are case sensitive and wrong values return null if there is an error, if the field is nullable. If you want an error you must make the field not nullable. If you want case insensitivity i believe you can override ValueOf, but i'm not certain, it's been yers since i implemented it.

Wicpar commented 2 years ago

valueof won't do it... here is the implementation: https://github.com/papsign/Ktor-OpenAPI-Generator/blob/19e2b92c8f33fda348b45f01a7e9f5e367b09f12/src/main/kotlin/com/papsign/ktor/openapigen/parameters/parsers/converters/primitive/EnumConverter.kt#L8 if you want to change it to use valueOf i will accept the PR

Szer commented 2 years ago

Case sensitivity is not that important

If I passed something which is not null to a query parameter limited to a set of known fields it shouldn't be resolved to null I think. And probably should be validated, cause in open-api-spec it is validated

So in my opinion better version would be

/ -> null
/?type=VALUE -> RequestType.VALUE
/?type=ANOTHER-> RequestType.ANOTHER
/?type=anythingElse -> error

.valueOf should be enough, it will enforce correct values. I'll work on PR then :)