opensearch-project / opensearch-ruby

Ruby Client for OpenSearch
Apache License 2.0
93 stars 47 forks source link

Don't modify the response if the body is frozen #212

Closed Earlopain closed 10 months ago

Earlopain commented 10 months ago

Fixes an incompatibility with webmock since 3.19, where it started to freeze the response body for performance reasons

Description

https://github.com/bblimke/webmock/pull/1033 https://github.com/bblimke/webmock/blob/v3.19.0/CHANGELOG.md#3190

When using code like the following, the response object tries to modify a frozen string:

def test_cluster_health_called
  stub_request(:get, "http://opensearch/_cluster/health")

  method_that_calls_out_to_cluster_health
  # => FrozenError: can't modify frozen String: ""
end

This started happening since webmock version 3.19 because they applied the frozen string literal comment to their codebase. When not specifying a response body with to_return(body: something) the string will come from webmock itself and be frozen.

This changes the response to not modify the body when the string is frozen, avoiding this error.

On a side-node, this also fixes webmock when the test files itself are using the frozen string literal comment, like so:

# frozen_string_literal: true

def test_cluster_health_called
  stub_request(:get, "http://opensearch/_cluster/health").to_return(body: "{}")

  method_that_calls_out_to_cluster_health
  # => FrozenError: can't modify frozen String: "{}"
end

Additional context: https://github.com/elastic/elastic-transport-ruby/issues/63 https://github.com/elastic/elastic-transport-ruby/pull/64


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Earlopain commented 10 months ago

I am not sure this is right. It's fine for webmock, but If I create a frozen ISO_8859_1 body I will now not see a problem with encoding, sending bad data to OpenSearch. I think we need something like a check of whether the encoding is UTF-8 and force_encoding only then.

That's a fair point, I've changed it to validate that the encoding is correct and not do anything in that case like you suggested. It still works as expected with this change.

Earlopain commented 10 months ago

What's the release cadence? It hasn't been long since the this PR, I know, but activity in the repository seems to have somewhat stalled with the last main commit being almost 3 months back.

dblock commented 10 months ago

@Earlopain open an issue asking for a release? @nhtruong let's make one?

nhtruong commented 10 months ago

After this is merged, I'll cut a new tag and release it ASAP.

Earlopain commented 10 months ago

Sounds good, thanks for sharing your plans (:

nhtruong commented 9 months ago

@Earlopain It's been deployed with 3.1.0 :)

Earlopain commented 9 months ago

Cool, thank you!