HTTParty.post is unusable about httparty HOT 12 OPEN

I'm still waiting for any reproduction example. So, I could debug and fix it

@TheSmartnik We're also getting this error after updating to 0.21.0.

In our case, when we issue a request that responds with 422 Unprocessable Entity, the response raw_body is an empty list []. When that body gets passed to encode_text and then as text to the TextEncoder initializer, the @text = +text operation on lib/httparty/text_encoder.rb:8 results in the error that is reported in the opening post.

Hope that helps.

from httparty.

Nope, been purely on specs for me. I am trying that fix too. I am personally of the opinion it would make sense to change it to be non breaking, to make peoples lives' easier, but as it is outside of the proper usage I would also consider this issue resolved if that fix works for everyone + the maintainers decide they do not wish to change it

from httparty.

Could you please provide a reproduction example

from httparty.

@john-h-k Were you able to fix this issue ? I am also getting this on HTTParty.get while passing query params.

from httparty.

➕ 1️⃣ on the breaking change. We started getting the same error after updating it to 0.21.0

from httparty.

I'm still waiting for any reproduction example. So, I could debug and fix it

from httparty.

We would like to upgrade to 0.21 per the security advisory, but are concerned about this issue, despite being unable to reproduce it using the following script. @mikevoets, could you please use this script as a starting point to give @TheSmartnik something they can work with?

# frozen_string_literal: true

require "bundler/inline"
gemfile(true) do
  ruby "3.1.2"
  source "https://rubygems.org"
  gem "httparty", "0.21.0"
  gem "minitest", "5.17.0"
  gem "webmock", "3.18.1"
end

require 'webmock/minitest'
require 'minitest/autorun'

class BugTest < Minitest::Test
  MOCK_URL = 'http://www.example.com/api'

  def test_1
    stub_request(:post, MOCK_URL).to_return(
      body: "[]",
      status: 422,
      headers: { 'Content-Type' => 'application/json' }
    )
    HTTParty.post(MOCK_URL)
  end
end

from httparty.

@jaredbeck @TheSmartnik Sure thing!

Here's the repro script. The only notable difference here is that the response is an array literal [] instead of a stringified array:

# frozen_string_literal: true

require "bundler/inline"
gemfile(true) do
  ruby "2.7.6"
  source "https://rubygems.org"
  gem "httparty", "0.21.0"
  gem "minitest", "5.17.0"
  gem "webmock", "3.18.1"
end

require 'webmock/minitest'
require 'minitest/autorun'

class BugTest < Minitest::Test
  MOCK_URL = 'http://www.example.com/api'

  def test_1
    stub_request(:post, MOCK_URL).to_return(
      body: [],
      status: 422,
      headers: { 'Content-Type' => 'application/json' }
    )
    HTTParty.post(MOCK_URL)
  end
end

from httparty.

@mikevoets Thanks for the reproduction 🙇

I've stumbled upon the same error when trying to upgrade httparty at GitLab 😓

I've fixed it by using a String for body: in specs instead of an Array 💪

I am wondering when HTTPReponse#body could NOT be a string 🤷 especially when used when used with read_body { ... } According the Ruby docs it should return a string 🤷

After reading the implementation and tests of Net::HTTP I see no evidence that body can be something else but a String.

WDYT?

@mikevoets @john-h-k Do you have a reproduction which happened in a real-world (non-test/spec) scenario? 🤔

from httparty.

Nope, been purely on specs for me. I am trying that fix too. I am personally of the opinion it would make sense to change it to be non breaking, to make peoples lives' easier, but as it is outside of the proper usage I would also consider this issue resolved if that fix works for everyone + the maintainers decide they do not wish to change it

Same for me - it only fails on tests.

from httparty.

This patch would fix it. I tested with the benchmarking tool @TheSmartnik linked and it doesn't seem to reintroduce the memory leak:

diff --git a/lib/httparty/text_encoder.rb b/lib/httparty/text_encoder.rb
index 006893e..a8302d1 100644
--- a/lib/httparty/text_encoder.rb
+++ b/lib/httparty/text_encoder.rb
@@ -5,7 +5,7 @@ module HTTParty
     attr_reader :text, :content_type, :assume_utf16_is_big_endian
 
     def initialize(text, assume_utf16_is_big_endian: true, content_type: nil)
-      @text = +text
+      @text = +String(text)
       @content_type = content_type
       @assume_utf16_is_big_endian = assume_utf16_is_big_endian
     end

I think @splattael (:wave:) is right though. The specs/fixtures should be changed to use Strings instead of Arrays.

from httparty.

The fault, dear Brutus, is not in our gems, but in our specs ..

sounds like we can close this one :)

from httparty.