-
Notifications
You must be signed in to change notification settings - Fork 804
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
Support Elasticsearch 8 #1664
Support Elasticsearch 8 #1664
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,10 @@ class Connections: | |
singleton in this module. | ||
""" | ||
|
||
def __init__(self): | ||
def __init__(self, *, elasticsearch_class=Elasticsearch): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change allows |
||
self._kwargs = {} | ||
self._conns = {} | ||
self.elasticsearch_class = elasticsearch_class | ||
|
||
def configure(self, **kwargs): | ||
""" | ||
|
@@ -80,7 +81,7 @@ def create_connection(self, alias="default", **kwargs): | |
it under given alias. | ||
""" | ||
kwargs.setdefault("serializer", serializer) | ||
conn = self._conns[alias] = Elasticsearch(**kwargs) | ||
conn = self._conns[alias] = self.elasticsearch_class(**kwargs) | ||
return conn | ||
|
||
def get_connection(self, alias="default"): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
import collections.abc | ||
import copy | ||
|
||
from elasticsearch.exceptions import TransportError | ||
from elasticsearch.exceptions import ApiError | ||
from elasticsearch.helpers import scan | ||
|
||
from .aggs import A, AggBase | ||
|
@@ -693,7 +693,8 @@ def count(self): | |
|
||
d = self.to_dict(count=True) | ||
# TODO: failed shards detection | ||
return es.count(index=self._index, body=d, **self._params)["count"] | ||
resp = es.count(index=self._index, query=d.get("query", None), **self._params) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not keep |
||
return resp["count"] | ||
|
||
def execute(self, ignore_cache=False): | ||
""" | ||
|
@@ -707,7 +708,8 @@ def execute(self, ignore_cache=False): | |
es = get_connection(self._using) | ||
|
||
self._response = self._response_class( | ||
self, es.search(index=self._index, body=self.to_dict(), **self._params) | ||
self, | ||
es.search(index=self._index, body=self.to_dict(), **self._params).body, | ||
) | ||
return self._response | ||
|
||
|
@@ -799,7 +801,7 @@ def execute(self, ignore_cache=False, raise_on_error=True): | |
for s, r in zip(self._searches, responses["responses"]): | ||
if r.get("error", False): | ||
if raise_on_error: | ||
raise TransportError("N/A", r["error"]["type"], r["error"]) | ||
raise ApiError("N/A", meta=responses.meta, body=r) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. elasticsearch-py 8.x has two types of errors now, |
||
r = None | ||
else: | ||
r = Response(s, r) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
from unittest import SkipTest, TestCase | ||
from unittest.mock import Mock | ||
|
||
from elastic_transport import ObjectApiResponse | ||
from elasticsearch import Elasticsearch | ||
from elasticsearch.exceptions import ConnectionError | ||
from elasticsearch.helpers import bulk | ||
|
@@ -47,7 +48,7 @@ | |
|
||
def get_test_client(wait=True, **kwargs): | ||
# construct kwargs from the environment | ||
kw = {"timeout": 30} | ||
kw = {"request_timeout": 30} | ||
|
||
if "PYTHON_CONNECTION_CLASS" in os.environ: | ||
from elasticsearch import connection | ||
|
@@ -131,8 +132,9 @@ def es_version(client): | |
@fixture | ||
def write_client(client): | ||
yield client | ||
client.indices.delete(index="test-*", ignore=404) | ||
client.indices.delete_template(name="test-template", ignore=404) | ||
for index_name in client.indices.get(index="test-*", expand_wildcards="all"): | ||
client.indices.delete(index=index_name) | ||
Comment on lines
+135
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deleting indices with a wildcard require a cluster setting in Elasticsearch 8. I had to specify |
||
client.options(ignore_status=404).indices.delete_template(name="test-template") | ||
|
||
|
||
@fixture | ||
|
@@ -160,55 +162,58 @@ def data_client(client): | |
|
||
@fixture | ||
def dummy_response(): | ||
return { | ||
"_shards": {"failed": 0, "successful": 10, "total": 10}, | ||
"hits": { | ||
"hits": [ | ||
{ | ||
"_index": "test-index", | ||
"_type": "company", | ||
"_id": "elasticsearch", | ||
"_score": 12.0, | ||
"_source": {"city": "Amsterdam", "name": "Elasticsearch"}, | ||
}, | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "42", | ||
"_score": 11.123, | ||
"_routing": "elasticsearch", | ||
"_source": { | ||
"name": {"first": "Shay", "last": "Bannon"}, | ||
"lang": "java", | ||
"twitter": "kimchy", | ||
return ObjectApiResponse( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In elasticsearch-py 8.x, the response pretends it is a dictionary to be backwards-compatible, but it does not support The alternative is to add |
||
meta=None, | ||
body={ | ||
"_shards": {"failed": 0, "successful": 10, "total": 10}, | ||
"hits": { | ||
"hits": [ | ||
{ | ||
"_index": "test-index", | ||
"_type": "company", | ||
"_id": "elasticsearch", | ||
"_score": 12.0, | ||
"_source": {"city": "Amsterdam", "name": "Elasticsearch"}, | ||
}, | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "42", | ||
"_score": 11.123, | ||
"_routing": "elasticsearch", | ||
"_source": { | ||
"name": {"first": "Shay", "last": "Bannon"}, | ||
"lang": "java", | ||
"twitter": "kimchy", | ||
}, | ||
}, | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "47", | ||
"_score": 1, | ||
"_routing": "elasticsearch", | ||
"_source": { | ||
"name": {"first": "Honza", "last": "Král"}, | ||
"lang": "python", | ||
"twitter": "honzakral", | ||
}, | ||
}, | ||
}, | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "47", | ||
"_score": 1, | ||
"_routing": "elasticsearch", | ||
"_source": { | ||
"name": {"first": "Honza", "last": "Král"}, | ||
"lang": "python", | ||
"twitter": "honzakral", | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "53", | ||
"_score": 16.0, | ||
"_routing": "elasticsearch", | ||
}, | ||
}, | ||
{ | ||
"_index": "test-index", | ||
"_type": "employee", | ||
"_id": "53", | ||
"_score": 16.0, | ||
"_routing": "elasticsearch", | ||
}, | ||
], | ||
"max_score": 12.0, | ||
"total": 123, | ||
], | ||
"max_score": 12.0, | ||
"total": 123, | ||
}, | ||
"timed_out": False, | ||
"took": 123, | ||
}, | ||
"timed_out": False, | ||
"took": 123, | ||
} | ||
) | ||
|
||
|
||
@fixture | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,11 @@ | |
from elasticsearch_dsl import connections, serializer | ||
|
||
|
||
class DummyElasticsearch: | ||
def __init__(self, *args, hosts, **kwargs): | ||
self.hosts = hosts | ||
|
||
|
||
def test_default_connection_is_returned_by_default(): | ||
c = connections.Connections() | ||
|
||
|
@@ -33,27 +38,36 @@ def test_default_connection_is_returned_by_default(): | |
|
||
|
||
def test_get_connection_created_connection_if_needed(): | ||
c = connections.Connections() | ||
c.configure(default={"hosts": ["es.com"]}, local={"hosts": ["localhost"]}) | ||
c = connections.Connections(elasticsearch_class=DummyElasticsearch) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those tests are about |
||
c.configure( | ||
default={"hosts": ["https://es.com:9200"]}, | ||
local={"hosts": ["https://localhost:9200"]}, | ||
) | ||
|
||
default = c.get_connection() | ||
local = c.get_connection("local") | ||
|
||
assert isinstance(default, Elasticsearch) | ||
assert isinstance(local, Elasticsearch) | ||
assert isinstance(default, DummyElasticsearch) | ||
assert isinstance(local, DummyElasticsearch) | ||
|
||
assert [{"host": "es.com"}] == default.transport.hosts | ||
assert [{"host": "localhost"}] == local.transport.hosts | ||
assert default.hosts == ["https://es.com:9200"] | ||
assert local.hosts == ["https://localhost:9200"] | ||
|
||
|
||
def test_configure_preserves_unchanged_connections(): | ||
c = connections.Connections() | ||
c = connections.Connections(elasticsearch_class=DummyElasticsearch) | ||
|
||
c.configure(default={"hosts": ["es.com"]}, local={"hosts": ["localhost"]}) | ||
c.configure( | ||
default={"hosts": ["https://es.com:9200"]}, | ||
local={"hosts": ["https://localhost:9200"]}, | ||
) | ||
default = c.get_connection() | ||
local = c.get_connection("local") | ||
|
||
c.configure(default={"hosts": ["not-es.com"]}, local={"hosts": ["localhost"]}) | ||
c.configure( | ||
default={"hosts": ["https://not-es.com:9200"]}, | ||
local={"hosts": ["https://localhost:9200"]}, | ||
) | ||
new_default = c.get_connection() | ||
new_local = c.get_connection("local") | ||
|
||
|
@@ -62,9 +76,12 @@ def test_configure_preserves_unchanged_connections(): | |
|
||
|
||
def test_remove_connection_removes_both_conn_and_conf(): | ||
c = connections.Connections() | ||
c = connections.Connections(elasticsearch_class=DummyElasticsearch) | ||
|
||
c.configure(default={"hosts": ["es.com"]}, local={"hosts": ["localhost"]}) | ||
c.configure( | ||
default={"hosts": ["https://es.com:9200"]}, | ||
local={"hosts": ["https://localhost:9200"]}, | ||
) | ||
c.add_connection("local2", object()) | ||
|
||
c.remove_connection("default") | ||
|
@@ -77,15 +94,16 @@ def test_remove_connection_removes_both_conn_and_conf(): | |
|
||
|
||
def test_create_connection_constructs_client(): | ||
c = connections.Connections() | ||
c.create_connection("testing", hosts=["es.com"]) | ||
c = connections.Connections(elasticsearch_class=DummyElasticsearch) | ||
c.create_connection("testing", hosts=["https://es.com:9200"]) | ||
|
||
con = c.get_connection("testing") | ||
assert [{"host": "es.com"}] == con.transport.hosts | ||
assert con.hosts == ["https://es.com:9200"] | ||
|
||
|
||
def test_create_connection_adds_our_serializer(): | ||
c = connections.Connections() | ||
c.create_connection("testing", hosts=["es.com"]) | ||
c = connections.Connections(elasticsearch_class=Elasticsearch) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test however uses the real client, for what it's worth. |
||
c.create_connection("testing", hosts=["https://es.com:9200"]) | ||
|
||
assert c.get_connection("testing").transport.serializer is serializer.serializer | ||
c_serializers = c.get_connection("testing").transport.serializers | ||
assert c_serializers.serializers["application/json"] is serializer.serializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think disabling security is OK here as HTTPS is not a concern of this library: this is handled at the elasticsearch-py/elastic-transport-python level.