feat: add Elasticsearch provider and update database registry with tests#25
Conversation
WalkthroughA new ElasticsearchDatabaseProvider class is introduced that implements the DatabaseProvider interface for Elasticsearch containers. The provider includes configuration options for security, clustering, JVM settings, and port mappings, plus Docker argument construction and connection validation. The provider is registered in the database registry and corresponding test coverage is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The new Elasticsearch provider contains substantial logic across form definitions, Docker argument construction, validation, and connection handling. The registry and test updates are straightforward additions that follow existing patterns. Primary review focus should be on the provider implementation's correctness, form field definitions, environment variable construction, and validation logic. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/test/unit/database-registry.test.ts (1)
122-131: Optional: table‑driven test to reduce repetition.Consider parametrizing provider ID/name/port assertions to avoid duplicated boilerplate as the list grows.
src/features/databases/providers/elasticsearch.provider.tsx (2)
74-84: Default version to a widely available tag.Using this.versions[0] currently pins to a specific 9.x which may not exist in the registry, breaking pulls. Prefer 'latest' by default.
Apply this diff:
- defaultValue: this.versions[0], + defaultValue: 'latest',
208-262: Minor: type safety for config and sensitive env handling.Consider a typed ElasticsearchConfig instead of any, and ensure env logging (if any) redacts ELASTIC_PASSWORD.
I can draft an ElasticsearchConfig interface and update usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/databases/providers/elasticsearch.provider.tsx(1 hunks)src/features/databases/registry/database-registry.ts(2 hunks)src/test/unit/database-registry.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/test/unit/database-registry.test.ts (1)
src/features/databases/registry/database-registry.ts (1)
databaseRegistry(71-71)
src/features/databases/providers/elasticsearch.provider.tsx (4)
src/features/databases/registry/database-provider.interface.ts (2)
DatabaseProvider(14-73)FieldsOptions(6-8)src/features/databases/types/form.types.ts (1)
FieldGroup(37-41)src/features/databases/types/docker.types.ts (2)
DockerRunArgs(16-22)ValidationResult(43-46)src/shared/types/container.ts (1)
Container(14-29)
src/features/databases/registry/database-registry.ts (1)
src/features/databases/providers/elasticsearch.provider.tsx (1)
ElasticsearchDatabaseProvider(14-296)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (4)
src/test/unit/database-registry.test.ts (2)
109-115: Elasticsearch registry assertions look good.Solid presence/name/port checks. No issues.
116-120: Count assertion is future‑proof.Using >= 7 is appropriate as providers grow.
src/features/databases/registry/database-registry.ts (1)
1-1: Provider wired correctly into the registry.Import and registration look correct. No regressions observed.
Also applies to: 67-68
src/features/databases/providers/elasticsearch.provider.tsx (1)
26-42: No changes needed — 9.x Docker tags are available.Official Elasticsearch 9.x images (e.g., 9.1.5 and 9.0.8) are available on docker.elastic.co as of October 2025, confirming the listed versions will pull successfully.
| getBasicFields({ isEditMode = false }: FieldsOptions): FormField[] { | ||
| return [ |
There was a problem hiding this comment.
Fix interface mismatch and undefined‑arg crash in getBasicFields.
DatabaseProvider.getBasicFields accepts an optional options arg. Your implementation requires it, which is not type‑compatible and will throw if called with undefined.
Apply this diff:
- getBasicFields({ isEditMode = false }: FieldsOptions): FormField[] {
+ getBasicFields(options: FieldsOptions = {}): FormField[] {
+ const { isEditMode = false } = options;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getBasicFields({ isEditMode = false }: FieldsOptions): FormField[] { | |
| return [ | |
| getBasicFields(options: FieldsOptions = {}): FormField[] { | |
| const { isEditMode = false } = options; | |
| return [ |
🤖 Prompt for AI Agents
In src/features/databases/providers/elasticsearch.provider.tsx around lines
45-46, the method currently destructures a required options arg which breaks the
DatabaseProvider.getBasicFields signature (options should be optional) and will
throw when called with undefined; change the function signature to accept an
optional parameter with a default (e.g., FieldsOptions = {}) so the
destructuring has a safe default and matches the interface, and ensure the
return type remains FormField[].
| label: 'Performance', | ||
| description: 'Configure performance settings', | ||
| fields: [ | ||
| { | ||
| name: 'elasticsearchSettings.bootstrapMemoryLock', | ||
| label: 'Bootstrap Memory Lock', | ||
| type: 'checkbox', | ||
| defaultValue: true, | ||
| helpText: | ||
| 'Lock memory on startup to prevent swapping (recommended for performance)', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Bootstrap memory lock default can cause startup failures.
Setting bootstrap.memory_lock=true without '--ulimit memlock=-1:-1' often fails. Either add ulimits/sysctls support to DockerRunArgs and set them here, or default this to false.
Minimal safe change:
- defaultValue: true,
+ defaultValue: false,
helpText:
'Lock memory on startup to prevent swapping (recommended for performance)',If you’d like, I can propose a follow‑up that extends DockerRunArgs with ulimits/sysctls and updates the run logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label: 'Performance', | |
| description: 'Configure performance settings', | |
| fields: [ | |
| { | |
| name: 'elasticsearchSettings.bootstrapMemoryLock', | |
| label: 'Bootstrap Memory Lock', | |
| type: 'checkbox', | |
| defaultValue: true, | |
| helpText: | |
| 'Lock memory on startup to prevent swapping (recommended for performance)', | |
| }, | |
| ], | |
| label: 'Performance', | |
| description: 'Configure performance settings', | |
| fields: [ | |
| { | |
| name: 'elasticsearchSettings.bootstrapMemoryLock', | |
| label: 'Bootstrap Memory Lock', | |
| type: 'checkbox', | |
| defaultValue: false, | |
| helpText: | |
| 'Lock memory on startup to prevent swapping (recommended for performance)', | |
| }, | |
| ], |
🤖 Prompt for AI Agents
In src/features/databases/providers/elasticsearch.provider.tsx around lines
177-188, the field defaultValue currently enables bootstrap.memory_lock which
can cause container startup failures without appropriate ulimits/sysctls; change
the defaultValue from true to false to avoid forcing memory locking by default,
and if you prefer to keep true document or implement support for passing
ulimits/sysctls through DockerRunArgs and the run logic so containers can be
started with '--ulimit memlock=-1:-1' (minimal safe change: set defaultValue to
false).
| // ==================== Utilities ==================== | ||
| getConnectionString(container: Container): string { | ||
| const username = 'elastic'; | ||
| // Default to https as security is enabled by default | ||
| const protocol = 'https'; | ||
| return `${protocol}://${username}:${container.password}@localhost:${container.port}`; | ||
| } |
There was a problem hiding this comment.
Connection string should respect enableAuth, username, and protocol.
Currently always https with basic auth. When security is disabled, ES serves http and no auth; current string will fail.
Apply this diff:
- getConnectionString(container: Container): string {
- const username = 'elastic';
- // Default to https as security is enabled by default
- const protocol = 'https';
- return `${protocol}://${username}:${container.password}@localhost:${container.port}`;
- }
+ getConnectionString(container: Container): string {
+ const securityEnabled = container.enableAuth !== false;
+ const protocol = securityEnabled ? 'https' : 'http';
+ const username = container.username ?? 'elastic';
+ const auth = securityEnabled
+ ? `${encodeURIComponent(username)}:${encodeURIComponent(container.password ?? '')}@`
+ : '';
+ return `${protocol}://${auth}localhost:${container.port}`;
+ }| validateConfig(config: any): ValidationResult { | ||
| const errors: string[] = []; | ||
|
|
||
| if (!config.password || config.password.length < 6) { | ||
| errors.push('Password must be at least 6 characters'); | ||
| } | ||
|
|
||
| if (!config.version) { | ||
| errors.push('Elasticsearch version is required'); | ||
| } | ||
|
|
||
| return { | ||
| valid: errors.length === 0, | ||
| errors, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate password only when security is enabled; add basic port check.
Prevents false negatives for unsecured dev clusters and catches invalid ports early.
Apply this diff:
validateConfig(config: any): ValidationResult {
const errors: string[] = [];
- if (!config.password || config.password.length < 6) {
- errors.push('Password must be at least 6 characters');
- }
+ const securityEnabled = config?.elasticsearchSettings?.securityEnabled !== false;
+ if (securityEnabled && (!config.password || config.password.length < 6)) {
+ errors.push('Password must be at least 6 characters');
+ }
+
+ if (typeof config.port !== 'number' || config.port < 1024 || config.port > 65535) {
+ errors.push('Port must be between 1024 and 65535');
+ }
if (!config.version) {
errors.push('Elasticsearch version is required');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validateConfig(config: any): ValidationResult { | |
| const errors: string[] = []; | |
| if (!config.password || config.password.length < 6) { | |
| errors.push('Password must be at least 6 characters'); | |
| } | |
| if (!config.version) { | |
| errors.push('Elasticsearch version is required'); | |
| } | |
| return { | |
| valid: errors.length === 0, | |
| errors, | |
| }; | |
| } | |
| validateConfig(config: any): ValidationResult { | |
| const errors: string[] = []; | |
| const securityEnabled = config?.elasticsearchSettings?.securityEnabled !== false; | |
| if (securityEnabled && (!config.password || config.password.length < 6)) { | |
| errors.push('Password must be at least 6 characters'); | |
| } | |
| if (typeof config.port !== 'number' || config.port < 1024 || config.port > 65535) { | |
| errors.push('Port must be between 1024 and 65535'); | |
| } | |
| if (!config.version) { | |
| errors.push('Elasticsearch version is required'); | |
| } | |
| return { | |
| valid: errors.length === 0, | |
| errors, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/features/databases/providers/elasticsearch.provider.tsx around lines 272
to 287, the validator currently enforces a password length unconditionally and
lacks port validation; change it to only validate password when security is
enabled (e.g., check config.security?.enabled or equivalent) and add a basic
port check that ensures config.port exists, is a number, and is within the valid
range (1–65535) so invalid ports are caught early; return the same
ValidationResult structure with errors pushed accordingly.
1 participant
Summary by CodeRabbit
Release Notes