Introduce Converter in junit-platform-commons

[scordio]

Overview

• Resolves Introduce generic ConversionService SPI #853
• Relates to Implicit conversion from integral numbers to byte array #2702

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[scordio]

There is plenty of work to do 🙃

The current highlights:

• New ConversionService interface, with ClassLoader param instead of ConversionContext (I will come back to it)
• New TypedConversionService base class
• The implementation of the existing ConversionSupport.convert(String, Class, ClassLoader) is now part of the new DefaultConversionService
• A test setup to demonstrate how to solve Locale argument conversion not setting language and country properly #3141 (I'll shape it better as an integration test)

Any feedback would be highly appreciated!

[marcphilipp]

Thanks for the draft! 👍

The tests are failing due to:

org.junit.platform.commons.support.conversion.ConversionService: module org.junit.platform.commons does not declare uses

That's because junit-platform-commons/src/module/org.junit.platform.commons/module-info.java is missing

uses org.junit.platform.commons.support.conversion.ConversionService;

[marcphilipp]

Looks very promising! 👍

[marcphilipp]

This would make a good test case, though. We have existing tests that register services for tests using an extra class loader:

junit5/platform-tests/src/test/java/org/junit/platform/launcher/core/LauncherFactoryTests.java

Lines 353 to 366 in 16c6f72

	private static void withTestServices(Runnable runnable) {
	var current = Thread.currentThread().getContextClassLoader();
	var url = LauncherFactoryTests.class.getClassLoader().getResource("testservices/");
	try (var classLoader = new URLClassLoader(new URL[] { url }, current)) {
	Thread.currentThread().setContextClassLoader(classLoader);
	runnable.run();
	}
	catch (IOException e) {
	throw new UncheckedIOException(e);
	}
	finally {
	Thread.currentThread().setContextClassLoader(current);
	}
	}

We could generalize and move that method to a test utility class (e.g. in junit-jupiter-api/src/testFixtures) so it can be reused here.

[marcphilipp]

That's because junit-platform-commons/src/module/org.junit.platform.commons/module-info.java is missing

uses org.junit.platform.commons.support.conversion.ConversionService;

When you add that, you'll also have to add it to platform-tooling-support-tests/projects/jar-describe-module/junit-platform-commons.expected.txt to adjust the integration test.

[scordio]

I've been lagging behind with this one but I should be able to spend time on it in the upcoming weekend.

[scordio]

Would you like to include this in 5.13 too? I should have enough time in the upcoming days to finalize it.

[marcphilipp]

@scordio Sorry for the late reply! I was out for a few days. If you still have time to finish it in the next two weeks, we could still include it in 5.13.0-RC1. WDYT?

[scordio]

Sounds good!

[sbrannen]

Sorry, for chiming in so late in the game, but...

Instead of ConversionService, I would prefer that we call this new public SPI something more like Converter.

And I'm also wondering if it should be parameterized.

FYI: I'm taking inspiration from the conversion APIs in the Spring Framework, which have been evolved over the years.

For example, I see our Converter as basically a combination of the Converter and ConditionalConverter APIs from Spring.

Thoughts?

[marcphilipp]

I'd be okay with Converter instead of ConversionService as well.

I'm not sure about parameterizing it. Where we use it (for example, to dynamically resolve method parameters), we won't be able to benefit from the type arguments. It might be a bit nicer for ConversionService/Converter authors to implement but we could also provide a convenience base class similar to TypedArgumentConverter.

One thing we should consider is using a parameter object (like Spring's TypeDescriptor) instead of the raw Class. This could, for example, be used to later add support for nested classes with runtime enclosing types. Thoughts?

[sbrannen]

I'd be okay with Converter instead of ConversionService as well.

Then let's change it to Converter. 👍

I'm not sure about parameterizing it. Where we use it (for example, to dynamically resolve method parameters), we won't be able to benefit from the type arguments. It might be a bit nicer for ConversionService/Converter authors to implement but we could also provide a convenience base class similar to TypedArgumentConverter.

Although we (the framework itself) might not benefit from generics at the moment, I would rather introduce them now than later discover that we actually need them ourselves to support additional (more dynamic) use cases.

In other words, what downside would there be to supporting generics from the start?

One thing we should consider is using a parameter object (like Spring's TypeDescriptor) instead of the raw Class. This could, for example, be used to later add support for nested classes with runtime enclosing types. Thoughts?

Yes, as you know... I am a big fan of "parameter context objects" in APIs that need to stand the test of time.

As I mentioned before, the conversion subsystem in Spring Framework has evolved organically over time, and most modern implementations actually implement for GenericConverter and ConditionalConverter, which both deal with TypeDescriptor instead of Class (except for the direct Class usage in ConvertiblePair).

In fact, there's even a dedicated ConditionalGenericConverter interface which combines those two, which I implemented in the latest converter that I implemented: OptionalToObjectConverter.

I am of course not suggesting that we introduce mechanisms as complex as those in Spring; however, I do hope that we can design in the API in such a manner that we don't prevent ourselves from being able to evolve the APIs and feature set over time as necessary.

And... supporting generics and something like a TypeDescriptor parameter object would be a good step in that direction.

[scordio]

I've hidden my last comment as it's mostly outdated after @sbrannen's one, and

supporting generics and something like a TypeDescriptor parameter object would be a good step in that direction.

If you'd like to go this way, I'm happy to adjust my work in this direction!

[marcphilipp]

I agree with @sbrannen. Let's go with the generic version and the parameter object.

[scordio]

I think TypeDescriptor shouldn't be specific to the conversion feature, so I plan to define it in org.junit.platform.commons.

Any better ideas?

EDIT: or maybe it is? 🤔 looking at how the Spring Framework does it.

[sbrannen]

I think TypeDescriptor shouldn't be specific to the conversion feature, so I plan to define it in org.junit.platform.commons.

I think it's OK for TypeDescriptor to reside in org.junit.platform.commons.support.conversion.

[scordio]

@sbrannen I'm still trying to get a proper background before jumping on the actual changes, so here's a dumb question 🙂

Specifically about parameterized types, is the ResolvableType abstraction closer to what we would need, from your perspective? Or maybe you imagine that together with TypeDescriptor?

[marcphilipp]

To start with, I had thought a relatively simple interface should suffice for TypeDescriptor.

interface TypeDescriptor {
	Class<?> getType();
}

@sbrannen WDYT?

[scordio]

My suggestion is not to close the door for Type getType() so maybe Class<?> getRawClass() or similar?

Mostly to be ready for cases where TypeDescriptor might wrap a ParameterizedType.

[marcphilipp]

We could do what Java did (e.g. for Method.getReturnType()) and have Class<?> getType() now and potentially Type getGenericType() later.

[scordio]

To start with, I had thought a relatively simple interface should suffice for TypeDescriptor.

interface TypeDescriptor {
	Class<?> getType();
}

So far, I understood TypeDescriptor should replace the Class parameter in ConversionSupport.convert (and, consequently, in the Converter methods), so:

public static <T> T convert(Object source, Class<T> targetType, ClassLoader classLoader)

should become:

public static <T> T convert(Object source, TypeDescriptor targetType, ClassLoader classLoader)

Therefore, I planned to define TypeDescriptor as a public class, with a private constructor and factory methods like forField(Field) and forParameter(Parameter) that ConversionSupport users could rely on (including DefaultArgumentConverter).

@marcphilipp did you have something else in mind?

[scordio]

After renaming to Converter, I realized StringToObjectConverter is pretty much a similar abstraction.

I propose to refactor the StringToObjectConverter hierarchy and rewrite all classes as Converter implementations (still with package-private visibility and used by DefaultConverter only).

WDYT?

[sbrannen]

I propose to refactor the StringToObjectConverter hierarchy and rewrite all classes as Converter implementations (still with package-private visibility and used by DefaultConverter only).

WDYT?

That's pretty much the vision I had all along: to have single abstraction/API.

So, yes, that sounds good to me. 👍

Though... the devil is in details. 😉

[scordio]

For now, most of the code relies on getType(), but I plan to check if dedicated APIs could better encapsulate some use cases (similar to getWrapperType or isPrimitive).

[scordio]

I wonder if this should delegate to the other convert method or should invoke DefaultConverter directly, skipping the new service loader logic. For now, I went with the former.

[scordio]

Initially, this was:

Suggested change

	boolean canConvert(Object source, TypeDescriptor targetType);
	boolean canConvert(Object source, TypeDescriptor targetType, ClassLoader classLoader);

However, I couldn't see the benefit of the ClassLoader parameter here, so I deleted it. Let me know if you see some use cases that could benefit from it.

Plus, I'm not entirely sure if this should rather be:

Suggested change

	boolean canConvert(Object source, TypeDescriptor targetType);
	boolean canConvert(TypeDescriptor sourceType, TypeDescriptor targetType);

The way Converter is currently integrated into ConversionSupport makes it easy to provide the source instance, but I wonder if this will be flexible enough with a bit of foresight.

[scordio]

Should this carry the target TypeDescriptor too?

Suggested change

	protected abstract T convert(S source) throws ConversionException;
	protected abstract T convert(S source, TypeDescriptor targetType) throws ConversionException;

[scordio]

Being an internal API, I deleted this convert(String, Class<?>) variant to reduce complexity. The downside is that the ClassLoader parameter is mostly unused in the subclasses.