Development: Add weaviate client configuration
#12028
Conversation
- Add WeaviateConfigurationProperties for configuration management - Add WeaviateClientConfiguration for client bean creation - Add WeaviateService for basic operations - Add Weaviate Java client v6 dependency to build.gradle - Configure Weaviate settings in application.yml (disabled by default) - Enable Weaviate in application-dev.yml for development - Add configuration properties test - Support both secure and non-secure connections - Use gRPC port for better performance
github-actions
bot
added
tests
server
|
@florian-glombik Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
…e-configuration # Conflicts: # src/main/java/de/tum/cit/aet/artemis/core/config/ConfigurationValidator.java # src/main/resources/META-INF/spring.factories
|
@florian-glombik Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
|
@florian-glombik Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
|
@florian-glombik Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
florian-glombik
changed the title
Development: Add weaviate setting to configurationDevelopment: Add weaviate client on startup when configured
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@florian-glombik Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
florian-glombik
changed the title
Development: Add weaviate client on startup when weaviate is configuredDevelopment: Add weaviate client configuration
|
@florian-glombik Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
MarcosOlivaKaczmarek
left a comment
MarcosOlivaKaczmarek
left a comment
There was a problem hiding this comment.
Code looks good overall, just some questions and an optional readability improvement
src/main/java/de/tum/cit/aet/artemis/core/config/ConfigurationValidator.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/ConfigurationValidator.java
Outdated
Show resolved
Hide resolved
...tum/cit/aet/artemis/core/exception/failureAnalyzer/WeaviateConfigurationFailureAnalyzer.java
Outdated
Show resolved
Hide resolved
…d-weaviate-configuration' into feature/general/global-search/add-weaviate-configuration
|
@florian-glombik Test coverage has been automatically updated in the PR description. |
| */ | ||
| @Lazy | ||
| @Configuration | ||
| @ConditionalOnProperty(prefix = "artemis.weaviate", name = "enabled", havingValue = "true") |
There was a problem hiding this comment.
Why use this over the common pattern:
@Conditional(WeaviateEnabled.class)
You'd need to add the WeaviateEnabled class, check other, similar @Conditional usages to see how it's usually implemented
| * | ||
| * @param enabled whether Weaviate integration is enabled | ||
| * @param host the Weaviate server host | ||
| * @param port the Weaviate HTTP port |
There was a problem hiding this comment.
I would call this httpPort to make the distinction to the grpcPort more immediately obvious.
| private static final String DEFAULT_HOST = "localhost"; | ||
|
|
||
| private static final int DEFAULT_HTTP_PORT = 8001; | ||
|
|
||
| private static final int DEFAULT_GRPC_PORT = 50051; |
There was a problem hiding this comment.
I'd much prefer if these values were loaded from the respective config file, rather than being hardcoded. Imagine we want to change this in the future, adapt it in the config file, but forget to change it here... this will lead to confusion, pretty sure
| private static final int MIN_PORT = 1; | ||
|
|
||
| private static final int MAX_PORT = 65535; | ||
|
|
||
| public static final String HTTP_SCHEME = "http"; | ||
|
|
||
| public static final String HTTPS_SCHEME = "https"; |
There was a problem hiding this comment.
Again I'd much prefer if these were loaded from some config file. Sure, the HTTP[S]_SCHEME may not be required here, but the min and max ports probably should be
There was a problem hiding this comment.
We are loading the values from the config file - these are just static constants for comparison and validation reasons
Max and Min port are statically defined values https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers
public ConfigurationValidator(Environment environment,
@Value("${" + Constants.PASSKEY_REQUIRE_FOR_ADMINISTRATOR_FEATURES_PROPERTY_NAME + ":false}") boolean isPasskeyRequiredForAdministratorFeatures,
@Value("${artemis.user-management.internal-admin.username:#{null}}") String internalAdminUsername,
@Value("${artemis.user-management.internal-admin.password:#{null}}") String internalAdminPassword, @Value("${artemis.weaviate.enabled:false}") boolean weaviateEnabled,
@Value("${artemis.weaviate.host:#{null}}") String weaviateHost, @Value("${artemis.weaviate.port:8080}") int weaviatePort,
@Value("${artemis.weaviate.grpc-port:50051}") int weaviateGrpcPort, @Value("${artemis.weaviate.scheme:#{null}}") String weaviateScheme) {
this.environment = environment;
this.artemisConfigHelper = new ArtemisConfigHelper();
this.isPasskeyRequiredForAdministratorFeatures = isPasskeyRequiredForAdministratorFeatures;
this.internalAdminUsername = internalAdminUsername;
this.internalAdminPassword = internalAdminPassword;
this.weaviateEnabled = weaviateEnabled;
this.weaviateHost = weaviateHost;
this.weaviatePort = weaviatePort;
this.weaviateGrpcPort = weaviateGrpcPort;
this.weaviateScheme = weaviateScheme;
}
| /** | ||
| * FailureAnalyzer that provides helpful error messages when the Weaviate configuration is invalid. | ||
| * This analyzer catches {@link WeaviateConfigurationException} and formats it into a user-friendly message | ||
| * with both a description of the problem and recommended actions to fix it. | ||
| */ | ||
| public class WeaviateConfigurationFailureAnalyzer extends AbstractFailureAnalyzer<WeaviateConfigurationException> { |
There was a problem hiding this comment.
Should probably add a conditional so this is only enabled when Weaviate is also enabled
| */ | ||
| class WeaviateConfigurationFailureAnalyzerTest { | ||
|
|
||
| private final WeaviateConfigurationFailureAnalyzer analyzer = new WeaviateConfigurationFailureAnalyzer(); |
There was a problem hiding this comment.
Shouldn't this be done via @Autowired?
| assertThat(analysis.getAction()).contains("enabled: true"); | ||
| assertThat(analysis.getAction()).contains("enabled: false"); |
There was a problem hiding this comment.
This is a very confusing test. Why does it require the action to contain both enabled: true and enabled: false? You should probably simplify this test, or comment what you are testing here exactly
| */ | ||
| class WeaviateConnectionFailureAnalyzerTest { | ||
|
|
||
| private final WeaviateConnectionFailureAnalyzer analyzer = new WeaviateConnectionFailureAnalyzer(); |
There was a problem hiding this comment.
Why not with @Autowired?
There was a problem hiding this comment.
I'm a bit sceptical about these tests, because they do really just "ensure" that you can't majorly edit the descriptive texts
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
Checklist
General
Server
Motivation and Context
@Nayer-kotry and @florian-glombik are going to develop a global keyword and semantic search within Artemis in the upcoming months.
After adding a general Setup for Weaviate in the Artemis environment in #11962, we do now integrate it into Artemis and configure a client that can communicate with Artemis.
This currently is a development only setup, not production ready.
Description
gsonversion, as required for the Weaviate client https://mvnrepository.com/artifact/com.google.code.gson/gsonapplication-dev.ymlDeferredEagerBeanInitializer.java, that allows to still use the FailureAnalyzer even, if theDeferredEagerBeanInitializer.javacatches an exceptionWeaviateClientConfiguration.java(other changes are just just required to make it configurable right away)Steps for Testing
artemis.weaviate.enabledtotruein yourapplication-local.ymlConnected to Weaviate atin the first 5-15 seconds after the URl was printedReview Progress
Code Review
Manual Tests
Test Coverage
Server
Last updated: 2026-01-27 12:45:37 UTC
Screenshots
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.