Validate user info block configuration

[thestrabusiness]

🧰 What's being changed?

This commit adds some validation around user info block required by the
middleware.

One check runs at application boot and ensures that the block is not
nil.

The other checks occur at run time and ensure that if the block is not
configured properly, the application will not crash and log an error
instead. The request to the Readme API will be aborted and the response
will be passed on through the rest of the middleware stack.

🧪 Testing

The new checks are covered by the automated test suite.

Without a block configured:

$LOAD_PATH << File.expand_path("../../metrics-sdks/packages/ruby/lib", __FILE__)
require "readme/metrics"

class ApiApp
  def call(env)
    [200, {"Content-Length" => "2"}, ["Ok"]]
  end
end

map "/api" do
  options = {api_key: "API_KEY"}
  use Readme::Metrics, options
  run ApiApp.new
end

/Users/anthony/Projects/metrics-sdks/packages/ruby/lib/readme/metrics.rb:18:in `initialize': Missing block argument. You must provide a block when configuring the (Readme::Errors::ConfigurationError)
middleware. The block must return a hash containing user info:
  use Readme::Metrics, options do |env|
    { id: "unique_id", label: "Your user label", email: "Your user email" }
  end

With an improperly configured block (missing label value)

$LOAD_PATH << File.expand_path("../../metrics-sdks/packages/ruby/lib", __FILE__)
require "readme/metrics"

class ApiApp
  def call(env)
    [200, {"Content-Length" => "2"}, ["Ok"]]
  end
end

map "/api" do
  options = {api_key: "api_key"}
  use Readme::Metrics, options do |env|
    {id: "anthonym", email: "[email protected]"}
  end
  run ApiApp.new
end

Running a request produces the following log and the request to the client returns a 200:

[JoelQ]

Can you boot an incorrectly configured application and show a screenshot of the expected errors in the "Testing" section of the PR description?

[JoelQ]

Nice use of the squiggly heredoc!

[JoelQ]

Thoughts on capitalizing the first letter of these errors? Or are they being inserted into an existing sentence?

[erunion]

Thoughts on wrapping the options in backticks? Like:

`reject_params` option must be an array of strings

[thestrabusiness]

Thanks for the suggestions. I made a couple of changes.

[JoelQ]

Thoughts on making this a class method Readme::Errors.bad_block_message rather than a callable constant? I think that might be a bit more idiomatic.

[JoelQ]

I'm not sure we need a begin/end block here if we're manually raising? This feels like using exceptions for control-flow. Thoughts on expressing this as a conditional instead?

user_info = @get_user_info.call(env)

if user_info_invalid?(user_info)
  puts Errors::BadBlockMessage.call(user_info)
else
  payload = Payload.new(har, user_info, development: @development)
  @@request_queue.push(payload.to_json)
end

[status, headers, body]

I could see extracting the conditional into a private method so call doesn't get too cluttered

[thestrabusiness]

Part of what I wanted to do here was catch the situation where the block itself raises an exception so the application can continue running. Does that make sense to do?

[JoelQ]

I see what you're saying. I kind of feels like feature creep though? We have a separate card for not crashing the client request if the middleware has an issue. I think we'll want a single rescue around the whole process.

If the block raises, then we can't send a valid payload to Readme so I don't think there is much value to adding a local rescue just for the block.

[thestrabusiness]

Great that makes sense. I'll update this to not use rescue block.

[JoelQ]

👍 for making these constants!