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

Fix native :fields behavior mismatch with rest #193

Merged
merged 1 commit into from Jan 11, 2016

Conversation

Projects
None yet
3 participants
@dspiteself
Contributor

dspiteself commented Jan 11, 2016

When processing results from a search query 'convert-fields-result' called getValue instead of getValues. This caused the first of the values to be returned. This behavior is inconsistent with the behavior of the rest api which always returns a vector.

Fix native :fields behavior mismatch with rest
When processing results from a search query 'convert-fields-result' called getValue instead of getValues. This caused the first of the values to be returned. This behavior is inconsistent with the behavior of the rest api which always returns a vector.
@dspiteself

This comment has been minimized.

Show comment
Hide comment
@dspiteself

dspiteself Jan 11, 2016

Contributor

Even though tests pass. It is extremely likely this breaks code in the wild.

Contributor

dspiteself commented Jan 11, 2016

Even though tests pass. It is extremely likely this breaks code in the wild.

@favila

This comment has been minimized.

Show comment
Hide comment
@favila

favila Jan 11, 2016

What follows is a higher-level restatement of the problem this patch addresses.

Currently, if you query elasticsearch specifying "fields", e.g.

{"fields": ["fieldA", "fieldB"],
 "query": ...}

With the elastich (and elasticsearch) REST client the result for fields will always be arrays:

// "fields" result from REST
{"hits":
  {"hits": [
    {"fields": {
      "fieldA":["valueA1", "valueA2"],
      "fieldB":["valueB1"]
    }}
  ]}
}

If you issue the same query using the native client, the result for fields will always be scalar and only supply the first value:

// fields result from native client (WRONG)
{"hits":
  {"hits": [
    {"fields": {
      "fieldA":"valueA1", // "valueA2" missing!
      "fieldB":"valueB1"
    }}
  ]}
}

The attached patch makes the second (wrong, native client) result look like the first (correct, REST) result.

favila commented Jan 11, 2016

What follows is a higher-level restatement of the problem this patch addresses.

Currently, if you query elasticsearch specifying "fields", e.g.

{"fields": ["fieldA", "fieldB"],
 "query": ...}

With the elastich (and elasticsearch) REST client the result for fields will always be arrays:

// "fields" result from REST
{"hits":
  {"hits": [
    {"fields": {
      "fieldA":["valueA1", "valueA2"],
      "fieldB":["valueB1"]
    }}
  ]}
}

If you issue the same query using the native client, the result for fields will always be scalar and only supply the first value:

// fields result from native client (WRONG)
{"hits":
  {"hits": [
    {"fields": {
      "fieldA":"valueA1", // "valueA2" missing!
      "fieldB":"valueB1"
    }}
  ]}
}

The attached patch makes the second (wrong, native client) result look like the first (correct, REST) result.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Jan 11, 2016

Member

I understand and the right thing to do here is to fix things. Would you mind submitting the same change against 2.1.x-stable? I can cut a point release shortly after. Thank you.

Member

michaelklishin commented Jan 11, 2016

I understand and the right thing to do here is to fix things. Would you mind submitting the same change against 2.1.x-stable? I can cut a point release shortly after. Thank you.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Jan 11, 2016

Member

Actually, don't worry, the change is a single commit so I can trivially cherry pick it :)

Member

michaelklishin commented Jan 11, 2016

Actually, don't worry, the change is a single commit so I can trivially cherry pick it :)

michaelklishin added a commit that referenced this pull request Jan 11, 2016

Merge pull request #193 from dspiteself/native-fields-getvalue-bug
Fix native :fields behavior mismatch with rest

@michaelklishin michaelklishin merged commit 4126a3e into clojurewerkz:master Jan 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Jan 11, 2016

Member

2.2.1 is up on Clojars.

Member

michaelklishin commented Jan 11, 2016

2.2.1 is up on Clojars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment