spring-projects/spring-boot

ElasticsearchDataAutoConfiguration: the auto configuration is applied even when the classpath does not have the required classes

reta opened this issue · 10 comments

reta commented

Problem

This is a joint discovery with @sothawo (see please [1] for the context).

Together we have built the Spring Data integration for OpenSearch [2] which is based on the upcoming Spring Data Elasticsearch 5.0.0 (in RC2 now). The OpenSearch integration relies on the client abstraction provided by Spring Data Elasticsearch (since OpenSearch and Elasticseach at least at this moment have very similar REST based clients) but excludes every single Elasticsearch dependency. It works fine in most cases but when used with Spring Boot 3.0.0 (in RC1 now), there is an exception at startup:

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'org.springframework.boot.autoconfigure.data.elasticsearch.ElasticsearchDataConfiguration$ReactiveRestClientConfiguration': Lookup method resolution failed
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.checkLookupMethods(AutowiredAnnotationBeanPostProcessor.java:472)
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.determineCandidateConstructors(AutowiredAnnotationBeanPostProcessor.java:342)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.determineConstructorsFromBeanPostProcessors(AbstractAutowireCapableBeanFactory.java:1283)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1185)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:561)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:521)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:326)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:324)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:931)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:916)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:584)
	at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:146)
	at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:730)
	at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:432)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:308)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1302)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1291)
	at io.pratik.elasticsearch.productsearchapp.ProductsearchappApplication.main(ProductsearchappApplication.java:36)
Caused by: java.lang.IllegalStateException: Failed to introspect Class [org.springframework.boot.autoconfigure.data.elasticsearch.ElasticsearchDataConfiguration$ReactiveRestClientConfiguration] from ClassLoader [jdk.internal.loader.ClassLoaders$AppClassLoader@5cb0d902]
	at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:483)
	at org.springframework.util.ReflectionUtils.doWithLocalMethods(ReflectionUtils.java:320)
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.checkLookupMethods(AutowiredAnnotationBeanPostProcessor.java:450)
	... 19 common frames omitted
Caused by: java.lang.NoClassDefFoundError: co/elastic/clients/ApiClient
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1012)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
	at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
	at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3402)
	at java.base/java.lang.Class.getDeclaredMethods(Class.java:2504)
	at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:465)
	... 21 common frames omitted
Caused by: java.lang.ClassNotFoundException: co.elastic.clients.ApiClient
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
	... 34 common frames omitted

The reason for it is that org.springframework.boot.autoconfigure.data.elasticsearch.ElasticsearchDataConfiguration$ReactiveRestClientConfiguration is being instantiated and inspected even when the classpath does not have the required classes (in this case ReactiveElasticsearchClient needs ElasticsearchTransport). The fix seems to be simple and low risk: @ConditionalOnClass(ElasticsearchTransport.class) (technically, we could use ApiClient but they both come in the same bundle).

Hope it makes sense.
Thank you.

[1] opensearch-project/spring-data-opensearch#1 (comment)
[2] https://github.com/opensearch-project/spring-data-opensearch

bclozel commented

Hello @reta, thanks for reaching out.

If I understand correctly, this requires an application to depend on "spring-data-elasticsearch" but explicitly exclude a mandatory compile dependency listed in its POM (here the elasticsearch client). I'm not sure we can consider that a valid use case, otherwise the "spring-data-elasticsearch" project would have made it optional in the first place.

I'm marking this for team-discussion so this is reviewed by the team. Doing so would have very broad implications as we would need to mark as conditional many auto-configurations for similar use cases and we would need to have integration tests for those.

reta commented

Helllo @bclozel, thank you for reply

That is correct, we have discussed that specific implementation with the Spring / Spring Data Elasticsearch team (@mp911de and @sothawo). As you rightly mention, it may not be a valid case for Spring Data Elasticsearch but for integrations built on top of it (and to reassure, none of the Spring Data Elasticsearch integrations are going to be affected). Thank you.

sothawo commented

This autoconfiguration is applied when spring-boot-starter-data-elasticsearch is used I assume. Wouldn't it then be the proper way to not use it but to provide an autoconfiguration and a starter for the Spring Data Opensearch integration?

reta commented

This autoconfiguration is applied when spring-boot-starter-data-elasticsearch is used I assume. Wouldn't it then be the proper way to not use it but to provide an autoconfiguration and a starter for the Spring Data Opensearch integration?

Absolutely, and we will, but the issue is that ElasticsearchClientAutoConfiguration kicks in when Spring Data Elasticsearch is present, so we would like to fix that first (the auto configuration for OpenSearch won't be contributed to Spring Boot unless there would be a desire to include it)

wilkinsona commented

Absolutely, and we will, but the issue is that ElasticsearchClientAutoConfiguration kicks in when Spring Data Elasticsearch is present.

As it should because the client is a mandatory dependency of Spring Data Elasticsearch. If Spring Data Elasticsearch is intended to be used without the Elastichsearch client, it should become an optional dependency. Until that time, for the reasons that Brian has explained above, we may not want to change Spring Boot.

reta commented

To add a bit more details may be, the non-reactive (JavaClientConfiguration) has the ConditionalOnClass annotation to deal with the fact that the dependency may not be there.

	@Configuration(proxyBeanMethods = false)
	@ConditionalOnClass(ElasticsearchClient.class)
	static class JavaClientConfiguration {
              ...
        }

I could also think of a valid Spring Data Elasticsearch scenario when the user wants to use RestHighLevelClient, in this case the Elastichsearch Java client could be excluded, only elasticsearch-rest-high-level-client is required.

sothawo commented

The RestHighLevelClient has been deprecated by Elasticsearch since last December, and all the code in Spring Data Elasticsearch using it will be deprecated from 5.0.0 on (the release coming in two weeks). The dependency on that old client is optional, and it will be then removed from Spring Data Elasticsearch in 5.1.

Spring Data Elasticsearch 4.4 had the RestHighLevelClient as default and the ElasticsearchClient as optional, version 5.0.0 switches that. Btw the RestHighLevelClient is only available for Elasticsearch 7.17, the current version of Elasticsearch is 8.5.0.

reta commented

For 5.0.0, both clients are supported (even deprecated one before it gets removed in 5.1), we should not require both clients on the classpath if the user took a risk to use RestHighLevelClient, not ElasticsearchClient (and as far as I can tell, Spring Data Elasticsearch 5.0.0 fully supports that).

sothawo commented

The user can still use the old RestHighLevelClient by adding this as an explicit dependency. But the dependency on the actual client library is there.
I have basically two reasons to have kept the old code in the next version:

  1. I don't remove code without having it deprecated before
  2. The new Elasticsearch client had some serious errors until recently; as I did not know if these would be fixed in time, I did not want to remove the old code yet.

Making the new client optional in Spring Data Elasticsearch and relying on the user to include the dependency in the right version would be an accident waiting to happen. Elasticsearch often enough has API changes even in patch level updates.

reta commented

@sothawo sure, I agree with what you said and the path forward makes sense to me. There is no even a suggestion to change the dependencies in any form, even more - no changes at all for Spring Data Elasticsearch. The only compelling case I am trying to make (beside "selfish" goal to better support OpenSearch) - I thought it makes sense to provide the way for user to flip the dependencies at own will and Spring Boot should not complain (it totally makes sense to me).

@bclozel @wilkinsona thank you for you time, closing the issue since there is no interest in the addressing such option.