Handle query params with non primitive type in Ktor controller

[nsprod]

This PR aims to fix #370

The fix only applies to optional parameter for now on. I would prefer to fix required query params in another PR if you don't mind.

cc @ulrikandersen

[ulrikandersen]

First of all; thanks for the contribution!

I am curios to know what's preventing you from also handling required parameters?
Handling non-required only feels a bit indeterministic from a user's perspective. I think a user would expect the same conversion to happen regardless of required/optional.

[ulrikandersen]

This check matches enums too, which are already handled by DefaultConversionService. Providing the ability to customise the encoding/decoding of enums would be nice, though it should probably be the topic of a separate PR.

[nsprod]

First of all; thanks for the contribution!

I am curios to know what's preventing you from also handling required parameters? Handling non-required only feels a bit indeterministic from a user's perspective. I think a user would expect the same conversion to happen regardless of required/optional.

Agreed, it is not optimal. I focused on optional parameters mainly because this is the use case I currently have at work and it would unblock my team. And I'm new to Kotlin / Ktor so I wanted to get a first feedback on the changes I have made. But I'm going to add support for required parameters 👍

[ulrikandersen]

Agreed, it is not optimal. I focused on optional parameters mainly because this is the use case I currently have at work and it would unblock my team. And I'm new to Kotlin / Ktor so I wanted to get a first feedback on the changes I have made. But I'm going to add support for required parameters 👍

The changes you made so far are good. That's also why I am encouraging you to handle the required ones too 🙂

For tests it is great to reuse the instantDateTime example, and in addition I think it would be useful with an example that demonstrates the different cases of (query) parameters that you are adding code for (string with format, required/optional). This example can later be extended to cover more cases. Makes sense?

For handling required fields perhaps you could add a getTypedOrFail that we can use instead of io.ktor.server.util.getOrFail.

[ulrikandersen]

I'm afraid the decision on when to use DataConversionPlugin is a be a bit more tricky than this. At first glance DefaultConversionService handles only primitive types, but the service hands off the decision further to platform specific implementations like this one for JVM where more types are handled by default (enums, BigDecimal, UUID, ...).

Perhaps we need to explicitly whitelist types that we wish to handle with DataConversionPlugin instead?

[ulrikandersen]

We could also introduce a manual flag to which enables the use of DataCoversionPlugin for any non primitive type. The user of Fabrikt would then manually have to provide converters for any enum, date times, etc. That approach is very deterministic and less magic and will work regardless of the platform (jvm, js, etc).

What would you, as a user, prefer @nsprod ?

[nsprod]

Nice catch!

Actually it might be even easier. I think we can forget about the DefaultConversionService and always call call.application.conversionService. As it seems the DataConversion class first tries to use a custom converter if any support a given type and if it doesn't it fallbacks on using DefaultConversionService. (source)

override fun fromValues(values: List<String>, type: TypeInfo): Any? {
        if (values.isEmpty()) {
            return null
        }
        val converter = converters[type.type] ?: DefaultConversionService
        return converter.fromValues(values, type)
    }

So I can remove requiresKtorDataConversionPlugin altogether and always use call.application.conversionService.
If you don't install the DataConversionplugin, Ktor DataConversion class will always fallback on using the DefaultConversionService.
If you install the DataConversionplugin, Ktor DataConversion class will either use your custom converter if it matches the type being converted or fallback on using the DefaultConversionService if the type doesn't match.

Does this sound correct to you?

[ulrikandersen]

Unfortunately call.application.conversionService seems to be an extension function defined in the DataConversion plugin (source) meaning that the generated code will not compile unless that dependency has been added to the project.

After having though more about this, I am learning towards your current implementation and relying on the DataConversion plugin for anything not primitive.

The JVM specific DefaultConversionService has support for a few additional types (BigDecimal, BigInteger and UUID), but the user can really easily provide conversion for those using the plugin. This approach also solves an issue I have observed with enums: the default conversion service compares on the enum name where we actually want it to compare on the the enum value. Example from your spec: ?order=ASC would work with the default converter, but ?order=asc (which is the casing used in the spec) would not.

[nsprod]

Unfortunately call.application.conversionService seems to be an extension function defined in the DataConversion plugin (source) meaning that the generated code will not compile unless that dependency has been added to the project.

Right, this is exactly why I introduced requiresKtorDataConversionPlugin. I was tired yesterday haha.

After having though more about this, I am learning towards your current implementation and relying on the DataConversion plugin for anything not primitive.

The JVM specific DefaultConversionService has support for a few additional types (BigDecimal, BigInteger and UUID), but the user can really easily provide conversion for those using the plugin. This approach also solves an issue I have observed with enums: the default conversion service compares on the enum name where we actually want it to compare on the the enum value. Example from your spec: ?order=ASC would work with the default converter, but ?order=asc (which is the casing used in the spec) would not.

Agreed, given the kind of types you usually deal with in query parameters, this should be pretty straightforward to provide a custom implementation if necessary.

I'm going to roll back my last commit.

PS: One other solution would be to rely on reflection to test if the DataConversion dependency is part of the project. I'm not a seasoned JVM developer, but I guess we don't want to go this way.

[ulrikandersen]

PS: One other solution would be to rely on reflection to test if the DataConversion dependency is part of the project. I'm not a seasoned JVM developer, but I guess we don't want to go this way.

Agreed. I would also avoid reflection for this.

[ulrikandersen]

You were quicker than me 🙈 See my comment above.

[ulrikandersen]

Nice work 💪

I think we should extend this to apply to path parameters too, but we can do that in a separate future PR.

[ulrikandersen]

Would we good to assert on the response message as well so we know that it is the correct reason for a 400.

You can use the StatusPages plugin:

install(StatusPages) {
    exception<BadRequestException> { call, cause ->
        call.respond(HttpStatusCode.BadRequest, cause.message ?: "Bad Request")
    }
}

[ulrikandersen]

Fixed in #376.

[nsprod]

Thanks for the fix and the review 🙏