Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_version_type ignored in native bulk #229

Open
royaldark opened this issue Jul 15, 2016 · 2 comments
Open

_version_type ignored in native bulk #229

royaldark opened this issue Jul 15, 2016 · 2 comments

Comments

@royaldark
Copy link
Contributor

#227 added support for _version and _version_type in bulk requests. This works great over REST, but the version type is ignored in native.

This happens because common.bulk/index-operation uses keys of the form _version_type, but native.conversion/->action-requests wants the key version-type (which, if passed in, would be stripped by common.bulk).

(.versionType
  (first
    (clojurewerkz.elastisch.native.conversion/->action-requests
      [(clojurewerkz.elastisch.native.bulk/index-operation
         {:_index "an_index", :_type "a_type", :_id "123456", :_version 5, :_version_type "external"})])))
=> #<INTERNAL>

Not sure the best solution - maybe ->action-requests needs to be more permissive and allow the key version_type as well as version-type?

@jqmtor
Copy link
Contributor

jqmtor commented Sep 24, 2016

Is it possible that this function should be slightly different?

It looks like it should ensure the compatibility of the keys that are passed to ->action-requests (see here), but it is only removing the leading underscore. So the :_version_type key is transformed to :version_type instead of :version-type. I tried looking for more insight in the commit messages but it didn't help.

I think it is also fair to argue that clojurewerkz.elastisch.common.bulk should not take care of these keys, since they are different in each client.

@michaelklishin what do you think? I am open to submit a PR with the required changes.

@michaelklishin
Copy link
Member

@quimrstorres if you have a test case that demonstrates the issue to go with it, feel free to do it. Af first glance your findings seem reasonable. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants