Set application/x-ndjson content type on bulk requests. #618

jmcarp · 2017-07-13T02:52:29Z

Resolves #609

tylerjharden

This looks good to me. Nice work to stay compliant as ES changes.

honzakral

Thank you for the patch! It is very solid, left a few tiny issues and one big one - changes like this needs tests.

honzakral · 2017-07-13T19:13:25Z

elasticsearch/connection/http_urllib3.py

@@ -111,6 +111,8 @@ def perform_request(self, method, url, params=None, body=None, timeout=None, ign
            if not isinstance(method, str):
                method = method.encode('utf-8')

+            request_headers = dict(self.headers)
+            request_headers.update(headers or {})


It would be nice if this block only ran if headers: to avoid copying the self.headers dict every time when it is only needed for the bulk-like bodies

honzakral · 2017-07-13T19:16:53Z

elasticsearch/client/__init__.py

@@ -1164,7 +1164,8 @@ def bulk(self, body, index=None, doc_type=None, params=None):
        if body in SKIP_IN_PATH:
            raise ValueError("Empty value passed for a required argument 'body'.")
        return self.transport.perform_request('POST', _make_path(index,
-            doc_type, '_bulk'), params=params, body=self._bulk_body(body))
+            doc_type, '_bulk'), params=params, body=self._bulk_body(body),
+            headers={'Content-Type': 'application/x-ndjson'})


we use lowercase for all headers to avoid conflicts - see https://github.com/elastic/elasticsearch-py/blob/master/elasticsearch/connection/http_urllib3.py#L59-L62

A test is needed to make sure the headers are merged properly

There are other APIs using the _bulk_body like msearch and mpercolate

honzakral · 2017-07-13T19:21:57Z

elasticsearch/transport.py

@@ -269,6 +269,8 @@ def perform_request(self, method, url, params=None, body=None):

        :arg method: HTTP method to use
        :arg url: absolute url (without host) to target
+        :arg headers: dictionary of headers, will be handed over to the
+            underlying :class:`~elasticsearch.Connection` class for serialization


no serialization happen to the headers, we need to set expectations correctly

davidt99 · 2017-08-07T15:53:29Z

@jmcarp Any plan to fix the review comments? Most of the driver requests (if not all) doesn't state content type, resulting warning logs in elastic 5.5.
The header support you wrote will be allow adding content type to all the request, thus fixing the warning.

honzakral · 2017-08-07T15:57:46Z

@davidt99 All requests are anotated with content-type accepted by elasticsearch since d862c8a (released in 5.2). You can also set headers globally by specifying headers={'custom': 'header'} when instantiating Elasticsearch.

davidt99 · 2017-08-07T16:20:43Z

@honzakral Oh, I was using 5.1 and looked for similar issue. Thanks.

jmcarp · 2017-08-08T04:00:23Z

I cleaned this up based on your suggestions @honzakral; let me know if this needs more revisions. I'm not sure why the tests are failing, but it looks like they're failing in the same way as master.

honzakral

thank you for the updated patch, it looks really good. Only outstanding issues are more of a housekeeping issues where a PR should ideally only touch the code it needs to touch for easier merging and reviewing.

I am more than happy to make those changes when merging your PR, will do that in a few days. It is after all my pedantic standard, not any fault with the code :)

honzakral · 2017-08-15T14:54:03Z

elasticsearch/client/__init__.py

+            if param in SKIP_IN_PATH:
+                raise ValueError("Empty value passed for a required argument.")
+        return self.transport.perform_request('GET', _make_path(index,
+            doc_type, id, '_percolate', 'count'), params=params, body=body)


several APIs have been removed from master as they have been removed in elasticsearch 6.0. Either please remove them from the patch or base the patch on 5.x

honzakral · 2017-08-15T14:54:51Z

elasticsearch/client/__init__.py

@@ -1387,4 +1540,3 @@ def field_caps(self, index=None, body=None, params=None):
        """
        return self.transport.perform_request('GET', _make_path(index,
            '_field_caps'), params=params, body=body)
-


please configure your editor not to make changes like these. It makes the PR hard to review when they are unrelated stylistic changes in unrelated places.

honzakral · 2017-08-15T14:54:59Z

elasticsearch/connection/base.py

@@ -123,5 +123,3 @@ def _raise_error(self, status_code, raw_data):
            logger.warning('Undecodable raw error response from server: %s', err)

        raise HTTP_EXCEPTIONS.get(status_code, TransportError)(status_code, error_message, additional_info)
-
-


please configure your editor not to make changes like these. It makes the PR hard to review when they are unrelated stylistic changes in unrelated places.

honzakral · 2017-08-15T14:55:19Z

elasticsearch/transport.py

@@ -292,7 +294,7 @@ def perform_request(self, method, url, params=None, body=None):

        if body is not None:
            try:
-                body = body.encode('utf-8', 'surrogatepass')            
+                body = body.encode('utf-8', 'surrogatepass')


please configure your editor not to make changes like these. It makes the PR hard to review when they are unrelated stylistic changes in unrelated places.

honzakral · 2017-08-15T14:55:28Z

test_elasticsearch/test_server/test_helpers.py

@@ -16,7 +16,7 @@ def bulk(self, *args, **kwargs):
        self._called += 1
        if self._called in self._fail_at:
            raise self._fail_with
-        return  self.client.bulk(*args, **kwargs)
+        return self.client.bulk(*args, **kwargs)


please configure your editor not to make changes like these. It makes the PR hard to review when they are unrelated stylistic changes in unrelated places.

jmcarp · 2017-08-21T04:34:17Z

I rebased on master and dropped the deprecated methods, and reverted whitespace changes. This is ready for another look when you have time @honzakral.

fxdgear · 2017-10-04T19:53:30Z

This LGTM. Can you please rebase off master and push again? I had to change the version of ES that travis uses.

Resolves elastic#609

jmcarp · 2017-10-04T20:14:51Z

Done @fxdgear.

fxdgear · 2017-10-04T20:22:09Z

Great thanks @jmcarp

outside. However, `Urllib3HttpConnection` did not use these headers but only its default ones. With this commit, `Urllib3HttpConnection` will either use the default ones or merge the default headers with the provided ones if there are any. Relates elastic#618

outside. However, `Urllib3HttpConnection` did not use these headers but only its default ones. With this commit, `Urllib3HttpConnection` will either use the default ones or merge the default headers with the provided ones if there are any. Relates #618

Set application/x-ndjson content type on bulk requests.

outside. However, `Urllib3HttpConnection` did not use these headers but only its default ones. With this commit, `Urllib3HttpConnection` will either use the default ones or merge the default headers with the provided ones if there are any. Relates elastic#618

jmcarp force-pushed the issue-609-x-ndjson branch 2 times, most recently from 38d30c0 to 7ee9be3 Compare July 13, 2017 03:41

tylerjharden approved these changes Jul 13, 2017

View reviewed changes

honzakral suggested changes Jul 13, 2017

View reviewed changes

honzakral changed the base branch from master to 5.x August 15, 2017 14:52

honzakral changed the base branch from 5.x to master August 15, 2017 14:52

honzakral suggested changes Aug 15, 2017

View reviewed changes

jmcarp force-pushed the issue-609-x-ndjson branch from 1cf8fd8 to 2befb61 Compare August 21, 2017 04:24

jmcarp added 2 commits October 4, 2017 16:07

Set application/x-ndjson content type on bulk requests.

073db90

Resolves elastic#609

Clean up custom headers.

3045516

jmcarp force-pushed the issue-609-x-ndjson branch from 2befb61 to 3045516 Compare October 4, 2017 20:08

fxdgear merged commit a03504c into elastic:master Oct 4, 2017

danielmitterdorfer mentioned this pull request Dec 13, 2017

Use provided request headers in urllib3 #682

Closed

rciorba added a commit to rciorba/elasticsearch-py that referenced this pull request Mar 2, 2018

Merge pull request elastic#618 from jmcarp/issue-609-x-ndjson

b7b5bdd

Set application/x-ndjson content type on bulk requests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set application/x-ndjson content type on bulk requests. #618

Set application/x-ndjson content type on bulk requests. #618

jmcarp commented Jul 13, 2017

tylerjharden left a comment

honzakral left a comment

honzakral Jul 13, 2017

honzakral Jul 13, 2017

honzakral Jul 13, 2017

honzakral Jul 13, 2017

davidt99 commented Aug 7, 2017

honzakral commented Aug 7, 2017

davidt99 commented Aug 7, 2017

jmcarp commented Aug 8, 2017

honzakral left a comment •

edited

Loading

honzakral Aug 15, 2017

honzakral Aug 15, 2017

honzakral Aug 15, 2017

honzakral Aug 15, 2017

honzakral Aug 15, 2017

jmcarp commented Aug 21, 2017

fxdgear commented Oct 4, 2017

jmcarp commented Oct 4, 2017

fxdgear commented Oct 4, 2017

		@@ -123,5 +123,3 @@ def _raise_error(self, status_code, raw_data):
		logger.warning('Undecodable raw error response from server: %s', err)

		raise HTTP_EXCEPTIONS.get(status_code, TransportError)(status_code, error_message, additional_info)

Set application/x-ndjson content type on bulk requests. #618

Set application/x-ndjson content type on bulk requests. #618

Conversation

jmcarp commented Jul 13, 2017

tylerjharden left a comment

Choose a reason for hiding this comment

honzakral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidt99 commented Aug 7, 2017

honzakral commented Aug 7, 2017

davidt99 commented Aug 7, 2017

jmcarp commented Aug 8, 2017

honzakral left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarp commented Aug 21, 2017

fxdgear commented Oct 4, 2017

jmcarp commented Oct 4, 2017

fxdgear commented Oct 4, 2017

honzakral left a comment •

edited

Loading