Refactor ConversionService to remove mutable operations from ConversionService.SHARED

[dstepanov]

Fixes #7657

Hopefully replaced all the possible usages of ConversionService.SHARED.

Some use cases cannot work after this change:
The detached request body is being converted:

        when:
        def request = HttpRequest.POST('/', JsonObject.createObjectNode([:]))

        then:
        request.getBody(String).get() != '{}'

Not sure if it's something that is even make sense, the fix now it to use this:

        when:
        request.setConversionService(conversionService)

        then:
        request.getBody(String).get() == '{}'

[yawkat]

fyi this will conflict with #8100, I made a similar ConversionService change in DefaultHttpClient there

[graemerocher]

@dstepanov can you resolve the conflicts

[dstepanov]

I think we can remove concurrent maps in the default implementation. I have measured it can get us 50% more throughput. The idea would be only to allow adding a new converter via TypeConverterRegistrar.

[graemerocher]

Return MutableConversionService to make the API fluent

[dstepanov]

Do you want to have it generic like it was before?

[graemerocher]

Makes sense

[dstepanov]

I'm looking at it again, and I don't like to have it in this way. There are zero usages of chaining in the current codebase. The registration API defines this signature: void register(ConversionService<?> conversionService) which prevents to do the chaining. IMHO the generic parameter only makes the code ugly, and the chaining is not very important in this case. I would prefer not to have it. WDYT @micronaut-projects/core-developers ?

[graemerocher]

It is probably fine without it but the generic argument remains a potentially large source of breaking changes

[graemerocher]

I'm concerned about the removal of the generic type argument here as it likely to break a lot of end user code. Is this a necessary change? What is the justification?

[dstepanov]

It was only used for the mutable add methods, now those methods are gone so the generic parameter is unused.

[graemerocher]

It does mean though that any place a user injected ConversionService<?> will break which might be a significant breaking change. Thoughts @sdelamo ?

[dstepanov]

Users might need to modify their code anyway if relying on the shared conversion service.

[alvarosanchez]

Our docs use ConversionService<?> in examples. Still I never liked that generic type argument, so my +1 to remove it if it's not needed

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
60 Code Smells

77.1% Coverage
0.0% Duplication