c.e.r.response/ok? erroneously reports false when updating a document

[Josh-Tilles]

Elastisch’s Getting Started and Indexing pages make it sound like clojurewerkz.elastisch.rest.response/ok? should always be used to determine if an Elasticsearch command succeeded, but it can give false negatives. Specifically, as the following REPL interactions demonstrate, it won't recognize a successful document update. I believe this is because c.e.r.response/ok? merely delegates to c.e.r.response/created? but successful updates return "created": false.

(require '(clojurewerkz.elastisch.rest [document :as es.document]
                                       [response :as es.response]))
;= nil

(def es-conn (clojurewerkz.elastisch.rest/connect))
;= #'user/es-conn

(es.document/put es-conn "example-index" "example-mapping" "example-id"
                 {"example_attribute" "initial value"})
;= {:_index "example-index", :_type "example-mapping", :_id "example-id", :_version 1, :created true}

(es.response/ok? *1)
;= true

(es.document/put es-conn "example-index" "example-mapping" "example-id"
                 {"example_attribute" "*updated* value"})
;= {:_index "example-index", :_type "example-mapping", :_id "example-id", :_version 2, :created false}

(es.response/ok? *1)
;= false

(es.document/get es-conn "example-index" "example-mapping" "example-id")
;= {:_index "example-index", :_type "example-mapping", :_id "example-id", :_version 2, :found true, :_source {:example_attribute "*updated* value"}

Note that the returned document contains the updated value, indicating that the PUT did actually succeed.

[michaelklishin]

@MerelyAPseudonym please submit a PR? Here's the docs repo.

[Josh-Tilles]

@michaelklishin Sure! I was already looking into how to resolve the issue, although I'm not confident that it's a documentation problem per se—even if the user were warned about the limitation of ok?, she/he would still need a way to verify that a PUT command worked. One solution might be "make sure that the response body doesn't contain a top-level error attribute", but why not teach ok? that trick?

If you do feel like a provisional update to the docs is still valuable, let me know and I'd be happy to make the changes and submit a PR.

[Josh-Tilles]

Hm. From commits a76e759 and d568425, it looks like ok? persists in the codebase for legacy reasons. Could/should it be considered deprecated?

[michaelklishin]

@MerelyAPseudonym your understanding is correct.

[Josh-Tilles]

Also, full disclosure: my hunch is that this issue is actually because the clojurewerkz.elastisch.rest.{,*.}clj APIs have a couple of design flaws:

1. it's impossible to override Elastisch's defaults for certain clj-http request options (in particular, :throw-exceptions)
2. Elastisch discards the HTTP response data, preventing the user from using anything not in the :body (in particular, the HTTP status code)

[Josh-Tilles]

@michaelklishin Oh, thanks for the confirmation! (I hadn't refreshed the page before writing my previous comment)

[michaelklishin]

Those are not design flaws but choices made by HTTP service clients. If you want to use an HTTP client directly, you don't need Elastisch.

[Josh-Tilles]

I disagree, but I'm not confident we're on the same page. I'd guess that a PR would be the most effective illustration of what I was talking about, so… consider the ball in my court?

[michaelklishin]

One simple question I'd like someone who claims abstracting HTTP API is a flaw to answer: how would you support an API that is nearly identical to the native client? If this is a bug then all abstractions are and everybody should use HTTP directly. Except that from my experience, nobody wants to, and this is why Elastisch is possibly the most widely adopted ClojureWerkz project.

Anyhow, if you want to submit a PR for the docs, please do. I'm locking this issue.

[michaelklishin]

Part of the problem here is that ES has a pretty unfortunate way of indicating success/failure scenarios. I will try to cover more of them but it really should have been reflected in the HTTP response status first and foremost. Maybe it's something that's better in 5.0, in prior versions it's certainly a mess :(

[michaelklishin]

I switched to detecting the lack of failure when we don't have known indicators of success. Thank you, @MerelyAPseudonym, for providing a test case.