Avoid having TypeConverter defined as beans

[dstepanov]

No description provided.

[dstepanov]

Strange, what is defining it to be initiated at the runtime:

 Error: Classes that should be initialized at run time got initialized during image building:
 io.micronaut.http.server.cors.CorsOriginConverter was unintentionally initialized at build time. To see why io.micronaut.http.server.cors.CorsOriginConverter got initialized use --trace-class-initialization=io.micronaut.http.server.cors.CorsOriginConverter

[sdelamo]

What is the motivation for this change?

But more importantly, we need to update the documentation. We currently say:

You can register additional converters for types not supported by Micronaut by defining beans that implement the TypeConverter interface

However, this PR indicates the recommended approach is to:
Create a class, not a bean, implementing TypeConveterRegistrar.
Register the converter in that class.
List the TypeConverterRegistrar implementation in src/main/resources/META-INF/services/io.micronaut.core.convert.TypeConverterRegistrar so that it is loaded later via Java SPI.

Moreover, removing the beans SameSiteConverter and CorsOriginConveter and making them @Internal could be considered a breaking change.

[dstepanov]

Registering the converter using the SPI versus finding beans should be faster for the startup; having fewer beans is also a plus.

The only reason to have converters as beans is if you need to inject something external. I'm not sure what the point of having those public.

Now I tried to disable CorsOriginConverter, and it looks like it's not needed anyway and probably added in v1 because the configuration beans were not flexible enough.

[sdelamo]

Now I tried to disable CorsOriginConverter, and it looks like it's not needed anyway and probably added in v1 because the configuration beans were not flexible enough.

Or maybe we lack test coverage.

[sdelamo]

The only reason to have converters as beans is if you need to inject something external

What do you mean by something external?

[dstepanov]

The only reason to have converters as beans is if you need to inject something external

What do you mean by something external?

Like the bean context or something from it.

[dstepanov]

Now I tried to disable CorsOriginConverter, and it looks like it's not needed anyway and probably added in v1 because the configuration beans were not flexible enough.

Or maybe we lack test coverage.

I think it's used in the multiple CORS configuration and we should have tests for it.

[sdelamo]

Now I tried to disable CorsOriginConverter, and it looks like it's not needed anyway and probably added in v1 because the configuration beans were not flexible enough.

Or maybe we lack test coverage.

I think it's used in the multiple CORS configuration and we should have tests for it.

A TCK test fails:

JUnit Platform Suite:HTTP Server TCK for Netty:JUnit Jupiter:CorsSimpleRequestTest:corsSimpleRequestForLocalhostCanBeAllowedViaRegexConfiguration()

[sdelamo]

@dstepanov can you please update the documentation

we need to update the documentation. We currently say:

You can register additional converters for types not supported by Micronaut by defining beans that implement the TypeConverter interface

However, this PR indicates the recommended approach is to: Create a class, not a bean, implementing TypeConveterRegistrar. Register the converter in that class. List the TypeConverterRegistrar implementation in src/main/resources/META-INF/services/io.micronaut.core.convert.TypeConverterRegistrar so that it is loaded later via Java SPI.

[dstepanov]

It's strange that it would fail only in native

[dstepanov]

@sdelamo I have corrected docs

[sonarcloud]

Quality Gate failed

Failed conditions

68.8% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud