Fix delete requests crashing before they reach the API

[eastagiletracker]

This PR proposes a fix for webhooks.delete, custom_fields.delete and broadcasts.delete, which raise NoMethodError inside Faraday before the request ever reaches the API. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/493. You can sign in with your GitHub ID to claim ownership of the project.

The defect

Connection#process_request returns the params object unchanged when it is empty. The Client and Connection wrapper methods default params to {}, so for any non-GET verb that empty Hash is handed to Faraday as the request body, and Net::HTTP calls bytesize on it while framing the request — NoMethodError: undefined method 'bytesize' for {}:Hash, raised before a single byte is written to the socket. webhooks.delete(id), custom_fields.delete(id) and broadcasts.delete(id) send no payload at all, so they raise on every call; tags.update(id) and subscribers.update(id) do the same whenever options.slice(...) comes back empty. tags.remove_from_subscriber escapes it only because it passes '' as an explicit params argument rather than relying on the default.

custom_fields.delete and broadcasts.delete have a second problem on the same line: they call @client.delete(path) without asking for the raw response, so Client#handle_response returns response.body and response.success? is then sent to a Hash. Their specs stub the client double as though it returned the response object, which is why the suite stayed green. Both now request the raw response the way tags.remove_from_subscriber already does.

Reproducing it on main

The failure needs a listening socket, because it happens while the body is framed, after the connection is opened. Against a local stub that reports the request line it received:

require 'socket'
require 'convertkit'

server = TCPServer.new('127.0.0.1', 0)
seen = []
Thread.new do
  loop do
    sock = server.accept
    seen << sock.gets.to_s.strip
    while (line = sock.gets) && line.strip != ''; end
    body = '{"success":true}'
    sock.print("HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: #{body.bytesize}\r\nConnection: close\r\n\r\n#{body}")
    sock.close
  end
end

class LocalClient < ConvertKit::Client
  def initialize(url)
    @connection = ConvertKit::Connection.new(url)
  end
end
client = LocalClient.new("http://127.0.0.1:#{server.addr[1]}/")

%w[webhooks custom_fields broadcasts].each do |resource|
  result = begin
    client.public_send(resource).delete(1).inspect
  rescue => e
    "#{e.class}: #{e.message}"
  end
  sleep 0.1
  request_line = seen.shift.to_s
  request_line = 'nothing' if request_line.empty?
  puts "client.#{resource}.delete(1) => #{result} | server received: #{request_line}"
end

On main at a7c7d66:

$ bundle exec ruby -Ilib repro.rb
client.webhooks.delete(1) => NoMethodError: undefined method `bytesize' for {}:Hash | server received: nothing
client.custom_fields.delete(1) => NoMethodError: undefined method `bytesize' for {}:Hash | server received: nothing
client.broadcasts.delete(1) => NoMethodError: undefined method `bytesize' for {}:Hash | server received: nothing

On this branch:

$ bundle exec ruby -Ilib repro.rb
client.webhooks.delete(1) => true | server received: DELETE /automations/hooks/1 HTTP/1.1
client.custom_fields.delete(1) => true | server received: DELETE /custom_fields/1 HTTP/1.1
client.broadcasts.delete(1) => true | server received: DELETE /broadcasts/1 HTTP/1.1

The change

• lib/convertkit/connection.rb — an empty payload becomes '', which is exactly the body tags.add_to_subscriber, tags.remove_from_subscriber and subscribers.unsubscribe already send today, so nothing that currently works changes on the wire. A non-empty payload is still JSON.generated, and GET params still pass through untouched.
• lib/convertkit/resources/custom_fields.rb and lib/convertkit/resources/broadcasts.rb — pass '', true so success? is read off the Faraday response.

Verification

• Baseline on main: bundle exec rspec gives 141 examples, 0 failures. On this branch: 144 examples, 0 failures — no test that passed before fails now.
• Three new examples in spec/lib/convertkit/connection_spec.rb pin the empty-payload request body for delete, post and put, and the #delete examples in custom_fields_spec and broadcasts_spec now assert the raw-response contract. Reverting only lib/ while keeping the specs turns all five red (expected: "" got: {}, and received :delete with unexpected arguments), so they fail without the change and pass with it.
• The script above is the end-to-end check: it shows the three calls returning true and the DELETE actually arriving at the server, which the spec suite alone cannot show because it stubs the transport.

How this was managed

This work was planned and tracked on a board imported from this repository's own pull requests and labels — 29 stories in all. The story behind this PR is Empty request payloads crash before the request is sent, and the board is at https://eastagiletracker.com/projects/493.

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.

---

Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com