refactor (poc): elasticsearchsystem does not depend on co.elastic client anymore

[osoykan]

No description provided.

[Copilot]

Pull request overview

This PR refactors the Elasticsearch testing system to remove the dependency on the official Elasticsearch Java client (co.elastic.clients) and replaces it with Ktor HTTP client for direct REST API calls. This change simplifies the architecture by eliminating a heavyweight client dependency in favor of lightweight HTTP calls.

Key Changes

• Replaced Elasticsearch official client with Ktor HTTP client for all Elasticsearch operations
• Changed migration interface from accepting ElasticsearchClient to ElasticsearchSystem
• Removed certificate-based security in favor of basic auth with isSecure flag

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
build.gradle.kts	Removed elastic client dependency, added Ktor HTTP client dependencies
ElasticsearchSystem.kt	Complete rewrite to use Ktor HTTP client with direct REST API calls, added helper methods for index operations
Options.kt	Removed ElasticClientConfigurer, replaced certificate with isSecure flag, changed cleanup/migration signatures
ElasticsearchTestSystemTests.kt	Updated migrations and tests to use new ElasticsearchSystem API with JSON string queries
Extensions.kt	Updated documentation examples to reflect new API
util.kt	Deleted file (no longer needed without elastic client)
ElasticsearchExposedCertificateTest.kt	Deleted test file (certificate handling removed)
stove-testing-e2e-elasticsearch.api	Public API changes reflecting the refactoring

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The error checking logic is incorrect. When creating an index that already exists, Elasticsearch returns a 400 Bad Request, not a success status. The current check response.status.isSuccess() || response.status.value == HttpStatusCode.BadRequest.value will pass for ANY 400 error, not just "index already exists".

This should either:

1. Parse the error response to check if it's specifically an "index already exists" error
2. Only allow success status codes and let other errors fail clearly
3. Document that this method silently ignores all 400 errors

Suggested change

	check(response.status.isSuccess() || response.status.value == HttpStatusCode.BadRequest.value) {
	val isAlreadyExistsError = response.status == HttpStatusCode.BadRequest &&
	response.bodyAsText().contains("\"type\":\"resource_already_exists_exception\"")
	check(response.status.isSuccess() || isAlreadyExistsError) {

[Copilot]

The extractSearchHits method accesses nested JSON fields without null checks. If the response structure is unexpected or if there are no hits, this could throw a NullPointerException.

Consider adding safe navigation:

internal fun <T : Any> JsonNode.extractSearchHits(clazz: Class<T>): List<T> =
  this.get(ResponseField.HITS)?.get(ResponseField.HITS)?.mapNotNull { hit -> 
    hit.get(ResponseField.SOURCE)?.let { objectMapper.treeToValue(it, clazz) } 
  } ?: emptyList()

Suggested change

	this[ResponseField.HITS][ResponseField.HITS]
	.mapNotNull { hit -> hit[ResponseField.SOURCE]?.let { objectMapper.treeToValue(it, clazz) } }
	this.get(ResponseField.HITS)?.get(ResponseField.HITS)?.mapNotNull { hit ->
	hit.get(ResponseField.SOURCE)?.let { objectMapper.treeToValue(it, clazz) }
	} ?: emptyList()

[Copilot]

Calling .config {} on an already-configured HttpClient creates a new client instance, which means the custom client provided via httpClientConfigurer will have its configuration replaced. This defeats the purpose of allowing custom client configuration.

If users provide a custom httpClientConfigurer, their base URL configuration will be overwritten. Either:

1. Don't apply defaultRequest to custom clients, or
2. Document that custom clients must set the base URL themselves, or
3. Pass the baseUrl to the configurer function so users can apply it in their custom client configuration

[Copilot]

The parameter order shouldGet(index: String, key: String, ...) is inconsistent with other methods like save(id: String, instance: T, index: String) and shouldDelete(key: String, index: String) where the key/id comes before the index.

For consistency across the API, consider using the same parameter order throughout. Looking at the usage in tests, there's already confusion with both shouldGet<ExampleInstance>(ANOTHER_INDEX, exampleInstance.id) and shouldGet<ExampleInstance>(key = exampleInstance.id, index = TEST_INDEX) being used.

[Copilot]

The configureTrustAllCertificates() function disables SSL certificate validation entirely, which poses a significant security risk. While this may be acceptable for testing environments, it should be clearly documented as a security risk and ideally made configurable.

Consider:

1. Adding a prominent warning in the documentation about this being for testing only
2. Making certificate validation configurable via ElasticsearchSystemOptions
3. Restricting this to only apply when explicitly enabled or in test environments

[Copilot]

The shouldQuery method now requires suspend but the query parameter type changed from Query (typed) to String (raw JSON). This is a breaking API change that:

1. Forces all callers to use raw JSON strings instead of type-safe query builders
2. Removes compile-time type safety for query construction
3. Makes it harder to catch query syntax errors at compile time

Consider providing both overloads:

• One accepting raw JSON strings for flexibility
• One accepting a type-safe query DSL or builder for better developer experience

[Copilot]

The authentication header is only set when cfg.isSecure is true, but authentication might be required even for non-HTTPS connections. Elasticsearch can be configured with authentication over HTTP.

The condition should check cfg.password.isNotBlank() without requiring cfg.isSecure, or make this more explicit:

if (cfg.password.isNotBlank()) {
  header(HttpHeaders.Authorization, cfg.basicAuthHeader())
}

Suggested change

	if (cfg.isSecure && cfg.password.isNotBlank()) {
	if (cfg.password.isNotBlank()) {

[Copilot]

String interpolation in the query JSON with "$desc" is vulnerable to JSON injection. If desc contains special characters like quotes, it could break the JSON structure or allow injection attacks.

Use proper JSON serialization instead:

val queryByDesc = objectMapper.writeValueAsString(
  mapOf(
    "query" to mapOf(
      "term" to mapOf(
        "description.keyword" to desc
      )
    )
  )
)

Suggested change

	val queryByDesc = """
	{
	"query": {
	"term": {
	"description.keyword": "$desc"
	}
	}
	}
	""".trimIndent()
	val objectMapper = com.fasterxml.jackson.module.kotlin.jacksonObjectMapper()
	val queryByDesc = objectMapper.writeValueAsString(
	mapOf(
	"query" to mapOf(
	"term" to mapOf(
	"description.keyword" to desc
	)
	)
	)
	)

[Copilot]

The documentation mentions isSecure parameter but doesn't explain when to use true vs false. It should clarify:

• When isSecure should be true (HTTPS connections, typically with certificates)
• When it should be false (HTTP connections, local testing)
• The security implications of each choice

Example:

* @param isSecure Whether to use HTTPS (default: false). Set to true when connecting to Elasticsearch over HTTPS with valid certificates. For local testing without TLS, use false.

Suggested change

	* @param isSecure Whether to use HTTPS (default: false for provided instances)
	* @param isSecure Whether to use HTTPS (default: false for provided instances). Set to true when connecting to Elasticsearch over HTTPS with valid certificates. For local testing or unsecured HTTP connections, use false. Note: Setting false means data is sent in plaintext and may be intercepted; setting true enables encrypted communication.

[Copilot]

The migration collection type parameter changed from ElasticsearchClient to ElasticsearchSystem. This is a breaking change that affects all existing migration implementations.

While this change makes sense (migrations shouldn't depend on the elastic client), existing users will need to:

1. Update all their DatabaseMigration<ElasticsearchClient> implementations to DatabaseMigration<ElasticsearchSystem>
2. Change their migration code from using ElasticsearchClient APIs to ElasticsearchSystem methods

Consider documenting this breaking change prominently in release notes or migration guide.

[codecov]

Codecov Report

❌ Patch coverage is 61.76471% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.93%. Comparing base (29f69ab) to head (0fff6b4).

Files with missing lines	Patch %	Lines
...e/testing/e2e/elasticsearch/ElasticsearchSystem.kt	59.66%	35 Missing and 13 partials ⚠️
...rendyol/stove/testing/e2e/elasticsearch/Options.kt	76.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #915      +/-   ##
==========================================
- Coverage   77.48%   76.93%   -0.55%     
==========================================
  Files         100       99       -1     
  Lines        3273     3295      +22     
  Branches      272      270       -2     
==========================================
- Hits         2536     2535       -1     
- Misses        588      608      +20     
- Partials      149      152       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.