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

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

Merged
merged 1 commit into from
Dec 4, 2023
Merged

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

merged 1 commit into from
Dec 4, 2023

Conversation

Earlopain
Copy link
Collaborator

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

Description

bblimke/webmock#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:
elastic/elastic-transport-ruby#63
elastic/elastic-transport-ruby#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.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CHANGELOG.md Outdated Show resolved Hide resolved
@Earlopain
Copy link
Collaborator Author

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.

CHANGELOG.md Outdated Show resolved Hide resolved
Fixes an incompatibility with webmock since 3.19, where it started to
freeze the response body for performance reasons

Signed-off-by: Earlopain <14981592+Earlopain@users.noreply.github.com>
@nhtruong nhtruong merged commit bde7e80 into opensearch-project:main Dec 4, 2023
55 checks passed
@Earlopain Earlopain deleted the frozen-body branch December 4, 2023 20:20
@Earlopain
Copy link
Collaborator Author

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
Copy link
Member

dblock commented Dec 8, 2023

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

@nhtruong
Copy link
Collaborator

nhtruong commented Dec 10, 2023

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

@Earlopain
Copy link
Collaborator Author

Sounds good, thanks for sharing your plans (:

@nhtruong
Copy link
Collaborator

@Earlopain It's been deployed with 3.1.0 :)

@Earlopain
Copy link
Collaborator Author

Cool, thank you!

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

Successfully merging this pull request may close these issues.

3 participants