HTTParty.post is unusable

[john-h-k]

#754 was a breaking change for us and ever since we have gotten undefined method +@' for ["Not found"]:Array` errors continuously whenever trying to use HTTParty

[TheSmartnik]

Could you please provide a reproduction example

[hs-ssamdani]

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

[hs-istahelin]

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

[TheSmartnik]

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

[mikevoets]

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.

[jaredbeck]

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

[mikevoets]

@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

[splattael]

@mikevoets Thanks for the reproduction :bow:

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

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

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

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? :thinking:

[john-h-k]

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

[mikevoets]

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.

[JonMidhir]

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.

[jaredbeck]

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

sounds like we can close this one :)