jillesvangurp / kt-search

Multi platform kotlin client for Elasticsearch & Opensearch with easily extendable Kotlin DSLs for queries, mappings, bulk, and more.
MIT License
107 stars 23 forks source link

[FEAT] Ability to get errors #87

Closed charlie-harvey closed 11 months ago

charlie-harvey commented 11 months ago

Describe the enhancement

I would like the Status of the request easily available. If it is already easily available, I would like to see it in the docs. I would also like to see tests that fail intentionally and check to see if it fails.

Why is this needed?

In the previous version of the library the results returned were the objects from the ElasticSearch lib (IndexResponse etc.). These objects have the public RestStatus status() function. This allows me to check if my request was successful.

I have been staring at it for a while and I think DocumentIndexResponse.result might be the place to check. Or maybe it is in DocumentIndexResponse.shards.successful? Or DocumentIndexResponse.shards.failed? Why are these Int fields?

if (response.status().equals(RestStatus.CREATED) || response.status().equals(RestStatus.OK)) {
    return Ok(uuid)
}

How do you think it should be done?

I thought maybe a try/catch, but everything is wrapped in Result types, which is great! But then the Result type is unwound and put into the DocumentIndexResponse so I can't tell if it was successful or not.

Hopefully its just some documentation. Or maybe I'll figure this out by myself with what is available. Thanks.

jillesvangurp commented 11 months ago

The old client is no longer supported because the underlying java client that came with the Elastic Java Rest High Level client has been deprecated.

The new client provides it's own Rest client abstraction which uses the RestResponse sealed class to represent the different responses that can come back with different http status codes.

Most of the client functions either return the expected 2xx style parsed response or throw an exception. So there is no need to check the response for a status code because you either get the response object or an exception is thrown. In some cases where e.g. a 404 is expected for non existing documents, the response is nullable.

In case of an exception, you can get the wrapped response (which is now exposed as a val) or the status code to check either the type or the integer rest status code.

But thanks for reporting this. I actually updated the documentation a bit and added a section on error handling. Please have a look at this: https://jillesvangurp.github.io/kt-search/manual/ClientConfiguration.html.

charlie-harvey commented 11 months ago

I am a kind of person who really needs to see examples. This is what I had:

        val uuid = UUID.randomUUID()
        val indexResponse = searchClient.indexDocument(
            target = "my-index",
            document = """{"some":"stuff"}""",
            id = uuid.toString()
        )
        if (indexResponse.shards.successful == 1) {
            return Ok(uuid)
        }
        return Err(indexResponse)

But with a try/catch, would I be doing something like this instead?

        val uuid = UUID.randomUUID()
        try {
            val indexResponse = searchClient.indexDocument(
                target = "my-index",
                document = """{"some":"stuff"}""",
                id = uuid.toString()
            )
            return Ok(uuid)
        } catch (e: Exception) {
            return Err(e)
        }

Thanks again.

jillesvangurp commented 11 months ago

You can catch a RestException instead of an Exception. But something like that yes. I would do this centrally and not all over the place.

Mostly what goes wrong with this type of logic falls into two categories: