ClassNotFoundException with quarkus.hibernate-search.elasticsearch.analysis.configurer

[lgarin]

Describe the bug
Starting with Quarkus 0.28, the Quarkus augmentation phase fails with a ClassNotFoundException as soon as a value is configured using the property quarkus.hibernate-search.elasticsearch.analysis.configurer.

Expected behavior
With Quarkus version 0.27, the same class can be loaded without issue.

Actual behavior
Quarkus statup fails with the following stack trace:

20:28:50,919 INFO  [io.qua.dep.QuarkusAugmentor] Beginning quarkus augmentation
20:28:51,073 ERROR [io.qua.dev.DevModeMain] Failed to start Quarkus: java.lang.IllegalArgumentException: java.lang.ClassNotFoundException: org.acme.AnalysisConfigurer 
	at io.smallrye.config.Converters.lambda$static$7c03a47f$1(Converters.java:90)
	at io.smallrye.config.Converters$BuiltInConverter.convert(Converters.java:477)
	at io.smallrye.config.SmallRyeConfig.getOptionalValue(SmallRyeConfig.java:139)
	at io.smallrye.config.SmallRyeConfig.getOptionalValue(SmallRyeConfig.java:131)
	at io.quarkus.runtime.configuration.ConfigUtils.getOptionalValue(ConfigUtils.java:92)
	at io.quarkus.deployment.configuration.OptionalObjectConfigType.acceptConfigurationValueIntoGroup(OptionalObjectConfigType.java:81)
	at io.quarkus.deployment.configuration.GroupConfigType.acceptConfigurationValueIntoLeaf(GroupConfigType.java:238)
	at io.quarkus.deployment.configuration.OptionalObjectConfigType.acceptConfigurationValue(OptionalObjectConfigType.java:36)
	at io.quarkus.deployment.configuration.ConfigDefinition.loadConfiguration(ConfigDefinition.java:501)
	at io.quarkus.deployment.ExtensionLoader.loadStepsFrom(ExtensionLoader.java:229)
	at io.quarkus.deployment.ExtensionLoader.loadStepsFrom(ExtensionLoader.java:144)
	at io.quarkus.deployment.QuarkusAugmentor.run(QuarkusAugmentor.java:88)
	at io.quarkus.runner.RuntimeRunner.run(RuntimeRunner.java:111)
	at io.quarkus.dev.DevModeMain.doStart(DevModeMain.java:177)
	at io.quarkus.dev.DevModeMain.start(DevModeMain.java:95)
	at io.quarkus.dev.DevModeMain.main(DevModeMain.java:66)
Caused by: java.lang.ClassNotFoundException: org.acme.AnalysisConfigurer 
	at io.quarkus.runner.RuntimeClassLoader.findClass(RuntimeClassLoader.java:229)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
	at io.quarkus.runner.RuntimeClassLoader.loadClass(RuntimeClassLoader.java:172)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:415)
	at io.smallrye.config.Converters.lambda$static$7c03a47f$1(Converters.java:88)
	... 15 more

To Reproduce
Steps to reproduce the behavior:

1. Create a new project with the Hibernate Search + Elasticsearch extension.
2. Create a new class (org.acme.AnalysisConfigurer) which implements ElasticsearchAnalysisConfigurer.
3. Configure the application according to the next section.

Configuration

quarkus.hibernate-search.elasticsearch.version=7.4
quarkus.hibernate-search.elasticsearch.analysis.configurer=org.acme.AnalysisConfigurer 

Environment (please complete the following information):

• Output of uname -a or ver: Microsoft Windows [Version 10.0.18362.418]
• Output of java -version: Java(TM) SE Runtime Environment (build 1.8.0_144-b01)
• Quarkus version or git rev: 1.0.0.CR1

[Sanne]

@yrodiere you think you could have a look?

[yrodiere]

This looks a lot like org.acme.AnalysisConfigurer just doesn't exist in your project.

I tried to create a reproducer, and it works as expected: https://github.com/yrodiere/quarkus-i5305

Tested on Fedora and Windows.

Please check your setup or provide a reproducer.

[jaikiran]

Hello @lgarin, @yrodiere,

Caused by: java.lang.ClassNotFoundException: org.acme.AnalysisConfigurer

Notice that the ClassNotFoundException message has a trailing whitespace after the class name, so that's a sign that the property value of quarkus.hibernate-search.elasticsearch.analysis.configurer is having a trailing whitespace.

Perhaps this is relevant to the recent change done in trimming property values #5050?

[yrodiere]

@jaikiran Spot on, thanks for the heads up. Adding a trailing space does reproduce the issue.

So this is a problem with the configuration extension rather than Hibernate Search: class names are not trimmed properly before Quarkus attempts to convert them to a Class.

@kenfinnigan, @dmlloyd, I'm not familiar with anything else than the Hibernate Search extension. Should we fix this by adding some trimming specifically for the Class converter, or is there a more global fix we should apply? As @gsmet said in your original pull request, I expect this will affect many property types.

[kenfinnigan]

MP Config spec dictates that config values are not trimmed.

So either we break the spec, or find a way to warn users about extraneous spaces

[yrodiere]

@kenfinnigan Fair enough, but we are not talking about extraction of string values here, we are talking about extraction of Class values.

The problem is that I have a @ConfigItem of type Class in the Hibernate Search extension, and Quarkus doesn't trim the string before it tries to convert the property value to a Class.

Surely the spec doesn't mandate that we keep the trailing spaces and fail miserably when interpreting a property value as a Class?

I believe this has something to do with the built-in org.eclipse.microprofile.config.spi.Converter implementations?

[Sanne]

@kenfinnigan I don't think that's reasonable for classnames : the spec doesn't have our converters.

Let's agree that the spec should be applied on String types - for other things we should be more pragmatic.

[kenfinnigan]

But everything is a string type to start with

[Sanne]

sure, everything is a string to begin with. But the spec shouldn't say what we do within the implementation with those strings: it's not reasonable to not trim classnames.

So I'd say this has nothing to do with spec compliance.

[yrodiere]

I found this in io.smallrye.config.Converters:

    @SuppressWarnings("unchecked")
    static final Converter<Class<?>> CLASS_CONVERTER = BuiltInConverter.of(6, (Converter & Serializable) value -> {
        try {
            return value != null ? Class.forName(value, true, SecuritySupport.getContextClassLoader()) : null;
        } catch (ClassNotFoundException e) {
            throw new IllegalArgumentException(e);
        }
    });

What if we added a trim() in there? Would it break the spec?

[Sanne]

@yrodiere +1 to patch there.

[kenfinnigan]

There doesn't appear to be any TCK tests that verify good or bad, behavior based on a value having trailing spaces for any converter.

There's also #5116, so should all non String converters do a similar trim?

Thoughts @dmlloyd @jmesnil ?

[emmanuelbernard]

Whatever is taking the String and converting it to a class should do the trimming. That does seem like clean DevX improvement and I can’t see any drawback. I can’t imagine the TCK testing that a trailing space should fail a class load.

[emmanuelbernard]

Meaning even if we only do it on known converters like integers and classes etc. That’s already progress.

[dmlloyd]

I believe that Class is a spec-covered converter.

That said - for specific converters, I think we're nearly always "TCK-safe" if we accept values that are rejected by specification. The risk is that the spec is improved in such a way that it begins to accept values which were previously rejected and the result of conversion of such values differs from what our improvements resulted in. This is always something to keep in mind as we're always trying to improve the specification but don't always exactly get our way!

That said - for class names and numerical values, it seems like an easy improvement to trim leading and trailing spaces... though at a very slight risk for classes (did you know that class names can contain spaces?). I don't know what kind of psychopath would generate a class with a space at the beginning or end of its name, but it's not disallowed!

I think that trimming whitespace for enumerated values is reasonable as well.

A question though: should we post a warning in cases like this, when we disregard trimmed whitespace?

[yrodiere]

I'm working on a pull request. Let me know if I have to add a warning.

[dmlloyd]

Please hold off on any config-related changes for the next couple of days. I'm rebasing a way-too-large config fix branch and it's already very painful...

[yrodiere]

@dmlloyd Sorry I saw your message too late, I was already almost done. Feel free to ignore my PR until you are done: I will rebase mine when yours is merged.
PR: smallrye/smallrye-config#181

[yrodiere]

The problem is solved in smallrye-config 1.4.0, but Quarkus is still using smallrye-config 1.3.9 at the moment. Upgrade PR: #5387

[rsvoboda]

smallrye-config 1.5.1 is used now, can be this issue closed ?
https://github.com/quarkusio/quarkus/blob/master/bom/runtime/pom.xml#L34

[gsmet]

@rsvoboda good catch