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

0.4.8 transport.perform_request with curb body is no longer parsed JSON #36

Closed
mikebaldry opened this issue Feb 10, 2014 · 9 comments
Closed

Comments

@mikebaldry
Copy link
Contributor

Since upgrading to 0.4.8, requests the go via the curb transport are returning a string body, instead of a hash body as in 0.4.7.

I'm seeing client.indices.get_aliases returning a string, instead of a hash for example.

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

Seems to be specific to the Curb based transport... Will look into it.

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

The culprit is obviously 80c48dd

@mikebaldry
Copy link
Contributor Author

That would be a good guess... curl response from the call to GET /_aliases does return as application/json so should be ok.

should the code be looking for Content-Type, not content-type in the headers hash?

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

should the code be looking for Content-Type, not content-type in the headers hash?

Good catch! Faraday "normalizes" the header like this, but obviously the Curb transport doesn't go through that...

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

This seems to be the problem:

client = ::Curl::Easy.new
=> #<Curl::Easy>

client.url = 'http://localhost:9200'
=> "http://localhost:9200"

client.http(:get)
=> true

client.header_str
=> "HTTP/1.1 200 OK\r\nContent-Type: application/json; charset=UTF-8\r\nContent-Length: 311\r\n\r\n"

client.headers
=> {}

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

Related: taf2/curb#184

@mikebaldry
Copy link
Contributor Author

it looks like #headers is for setting headers on the request, not getting them from the response

@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

Yes, but in any case seems like there's no version of headers parsed as a Hash in Curb yet.

Let's add the check for client.header_str =~ /\/json/ in https://github.com/elasticsearch/elasticsearch-ruby/blob/master/elasticsearch-transport/lib/elasticsearch/transport/transport/http/curb.rb#L31.

karmi added a commit that referenced this issue Feb 10, 2014
…transport

Because of the change introduced in elasticsearch/elasticsearch-ruby#80c48dd,
responses from the Curb transport were not correctly deserialized into Hash from JSON string.

That's because we actually haven't returned any response headers in the `Curb#perform_request` method.

Since Curb doesn't return any response headers as a Hash (see related), we simply check for:

    header_str =~ /\/json/

in `Curb#perform_request`.

This seems to have noticeable impact on the raw performance, adding ~ 10ms to _mean_ times
when running the client benchmark test. On the other hand, previously we did check body
whether it "looks like a JSON", which definitely has been influencing the end times,
so hopefully it balances out.

Once Curb adds response headers as a Hash, the code should change appropriately.

Closes: #36
Related: taf2/curb#184
karmi added a commit that referenced this issue Feb 10, 2014
…transport

Because of the change introduced in elasticsearch/elasticsearch-ruby#80c48dd,
responses from the Curb transport were not correctly deserialized into Hash from JSON string.

That's because we actually haven't returned any response headers in the `Curb#perform_request` method.

Since Curb doesn't return any response headers as a Hash (see related), we simply check for:

    header_str =~ /\/json/

in `Curb#perform_request`.

This seems to have noticeable impact on the raw performance, adding ~ 10ms to _mean_ times
when running the client benchmark test. On the other hand, previously we did check body
whether it "looks like a JSON", which definitely has been influencing the end times,
so hopefully it balances out.

Once Curb adds response headers as a Hash, the code should change appropriately.

Closes: #36
Related: taf2/curb#184
@karmi
Copy link
Contributor

karmi commented Feb 10, 2014

Closed in ecfe780

@karmi karmi closed this as completed Feb 10, 2014
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

2 participants