equinix-labs / metal-go

[Deprecated] Golang client for Equinix Metal
https://deploy.equinix.com/labs/equinix-sdk-go/
MIT License
3 stars 2 forks source link

Improve error messages #168

Closed displague closed 12 months ago

displague commented 1 year ago

Today the error messages emitted only report the HTTP Status message.

422 Unrecognized Entities https://github.com/equinix-labs/metal-go/blob/main/metal/v1/api_projects.go#L175 https://github.com/equinix-labs/metal-go/blob/b1e03ba5195bd47eed1dd3db3ce38d2b494b349f/templates/client.mustache#L736-L754 https://www.rfc-editor.org/rfc/rfc7807#section-3

In packngo the possible error bodies were converted into the error message returned, along with the HTTP status message.

422 Unrecognized Entities: foo is not valid with bar, baz is required https://github.com/packethost/packngo/blob/master/packngo.go#L71-L81

metal-go (arguably all of the generated SDKs) should offer some examples and improved experience around accessing the reported error message(s).

displague commented 1 year ago

Metal Errors are returned as such:

HTTP 422 Unrecognized something
Content-type: application/json

{"errors":["the explanation"]}

and sometimes: {"error": "single error"}

In the OAS3 spec, we have the Error type defined: https://github.com/equinix-labs/metal-go/blob/b1e03ba5195bd47eed1dd3db3ce38d2b494b349f/spec/oas3.fetched/components/schemas/Error.yaml

This renders to model_error.go

That type does not implement Error() string though (which would make it a go error type). The generator has no idea how to parse the errors nor how that would be presented as a Go error.

If we created a separate errors.go file in v1/ (through either patching or additional templates or a cp)

// copyrights
package v1

func (o *Error) Error() string {
  errs := []string{}
  errs = append(errs, r.Errors...)
  if r.Error != "" {
      errs = append(errs, r.Error)
  }
  return strings.Join(errs, ", ")
}

Inside of client.mustache, we would modify the error handling of formatErrorMessage so that where it currently checks for Title and Details we would preset str to Error(). Something like:

if err, ok := v.(error); ok {
  str = err.Error()
}

(We could consider removing the RFC-7807 handling portion since that was generic and not implemented in this API)

ctreatma commented 1 year ago

I'm not sure we get much out of patching the Error() method into model_error.go, since we still have to wire it in somewhere else anyway and I don't think we'd expect users to be interacting directly with an error model rather than with the GenericOpenAPIError type that wraps it. IMO we could simply update formatErrorMessage to cover both of the code snippets from your suggestion, which is more straightforward to do with custom templates.

github-actions[bot] commented 12 months ago

This issue has been resolved in version 0.26.0 :tada: