Fetch error class using faultcode instead of body text matching

[lulalala]

Description

This is GitLab's attempt to fix #79: some custom fields can contain error codes. They are mistakenly treated as actual error.

Some error response hash now contains "faultcode", for example the body could be:

{
  "detail" => {
    "MalformedQueryFault" => {
      "FaultMessage" => "invalid field for query: Account.foo",
      "FaultCode" => "INVALID_FIELD" }
  },
  "faultcode" => "fns:INVALID_FIELD",
  "faultstring" => "invalid field for query: Account.foo"
}

So when response is a hash, we try to find faultcode to lookup
error class instead of body text matching.

Body text matching is bad because it can cause false positives,
when data happens to contain error code in the custom fields.

Notes

Bulk operation (which returns a hash) is the exception.
We assume it does not contain custom fields info based on experience.

Tasks

• TODO item before it can be reviewed and/or merged
• Another TODO item

Risks

We don't know originally when we implemented from_body, how does the typical response body looks like (the spec does not provide any believable example). We also suspect that it might have changed since (and possibly making it obsolete).

If we have more concrete examples, we should be more confident on whether this breaks/fixes things.

Hi @rringler would you still have the example response / use case from the past?

[lulalala]

Hi zendesk/billing could you review please? As this has been sitting for an year now.

[rringler]

Please be kind; it's been a good while since I've coded Ruby, but perhaps this could be simplified?

    def self.from_body(response)
      return unless CODE_MATCHER.match(response.body.to_s)
      body = response.body

      if !body.is_a?(Hash) || body.key?('results')
        return from_text(body.to_s)
      elsif faultcode = body['faultcode'].to_s.sub(/\Afns:/, '')
        return CODE_CLASSES[faultcode]
      end
    end

Existing tests pass (though I was only able to install 2.7 on my M2 mac), and I think it needs some commenting, but the logic seems a bit simpler?

[lulalala]

@rringler
Thanks! I think there are less lines of code, but less readable. I want to at least have the comments to explain my rationale.

I also prefer to not worry about what CODE_CLASSES[""] would return. WDYT?

[jurisgalang]

@lulalala apologies for a very long turn around time on your PR. but i'll work on getting this merged this month.

i like @rringler's code -- it's concise and it does not sacrifice readability (imo) with a very minor change (comments inserted):

def self.from_body(response)
  return unless CODE_MATCHER.match(response.body.to_s)
  body = response.body

  # Bulk operation response, assume it doesn't contain custom 
  # fields.
  if !body.is_a?(Hash) || body.key?('results')
    from_text(body.to_s)

  # NOTE: Some observed faultcodes are now in 400 responses,
  # and therefore won't reach here, but we assume some other 
  # faultcodes are in 200 responses and can still reach here.  
  elsif faultcode = body['faultcode'].to_s.sub(/\Afns:/, '')
    CODE_CLASSES[faultcode]
  end
end

wdyt?

[lulalala]

@jurisgalang no worries. I also forgot to reply 😛

Again I dislike passing "" into CODE_CLASSES hash, but it's not a blocker.

Your version's comment would suggest that !body.is_a?(Hash) means it is a bulk operation. I don't have an example to know whether that's true or not. If you can confirm for sure then I'll update my code.

I am against using regular expression to do an early return. It seems more expensive than the simple hash access and equality operations.