elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
695 stars 70 forks source link

Add bytes method to Response struct #164

Closed laurocaetano closed 3 years ago

laurocaetano commented 3 years ago

This patch has a simple change to wrap the reqwest::Response::bytes() into the client Response struct. Additionally I've taken the liberty to do some cosmetic changes in the overall struct - I hope that's fine too.

Thanks @dodomorandi for opening up the issue.

Closes https://github.com/elastic/elasticsearch-rs/issues/160

elasticmachine commented 3 years ago

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cla-checker-service[bot] commented 3 years ago

💚 CLA has been signed

russcam commented 3 years ago

Hi @laurocaetano, please can you sign the CLA so that we can pull this in. Also, would you mind adding an integration test to https://github.com/elastic/elasticsearch-rs/blob/master/elasticsearch/tests/client.rs for a bytes response? The tests can be run with

cargo make  test-elasticsearch

which will spin up an elasticsearch docker instance to run against.

laurocaetano commented 3 years ago

Thanks so much @russcam! I'm going to work on that tomorrow morning :)

It feels good to do open-source again :heart:

laurocaetano commented 3 years ago

I've updated the PR adding the integration tests, thanks for the review!


A couple of notes on the integration tests:

The task test-elasticsearch is private, so I only ran the tests with cargo make test.

The integration tests only run when I set TEST_SUITE=platinum: This is due to this condition. I am not sure if that is intended though. The following change would behave as I would expect:

-condition = { env_set = [ "ELASTICSEARCH_URL" ], env = { "TEST_SUITE" = "platinum" } }
+condition = { env_set = [ "ELASTICSEARCH_URL", "TEST_SUITE" ] }

I also get failures when running the cert tests:

    certificate_certificate_validation
    default_certificate_validation
    fail_certificate_certificate_validation
    full_certificate_validation

But I did not have enough time to debug those failures as well. It is probably due to some lack of configuration when running the tests on my machine.

Please let me know what you think about these issues. Probably I am missing something on my configs.

laurocaetano commented 3 years ago

Do you see this when running with TEST_SUITE platinum? I suspect the issue is likely the assertions in the tests; the assertions try to accommodate differences across Windows, macOS and Linux. They pass for me locally on Windows and Ubuntu 18.04 through WSL2. I think these can be addressed separately to this PR, if you'd like to open a separate issue with details about your setup?

I tried running locally (Ubuntu 20.04) and the cert tests fail. Some of them fail with:

---- full_certificate_ca_chain_validation stdout ----
Error: Error { kind: Http(reqwest::Error { kind: Request, url: Url { scheme: "https", host: Some(Domain("localhost")), port: Some(9200), path: "/", query: None, fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 111, kind: ConnectionRefused, message: "Connection refused" })) }) }

I've created the issue: https://github.com/elastic/elasticsearch-rs/issues/167. I will invest a bit more time into it the next days, and understand the real issue in these failures on my machine.

russcam commented 3 years ago

Thanks @laurocaetano.

I re-ran the tests now on Ubuntu 18.04 with WSL 2, and am seeing test failures. The TLS related dependencies of reqwest were updated between 0.10 and 0.11, so I suspect the tests need some adjustment, which we can follow up separately on #167.

russcam commented 3 years ago

Thank you for your contribution @laurocaetano! 👍

laurocaetano commented 3 years ago

Thanks for your support @russcam :+1: