Useful http Errors about go-client HOT 14 CLOSED

Created PR FusionAuth/fusionauth-client-builder#22 to return nil instead of io.EOFsince that isn't an exceptional error.

from go-client.

Fixed in the latest version. Thanks!

from go-client.

I have no issues in general with this PR.

However, it seems semi-normal to return a 401 without a response body. Is the main issue that in Go this is un-expected, or at least in context of its use in the Terraform provider?

FusionAuth will return an empty body with a 404 and a 401. A 404 may be an expected status code depending upon how and why you are making an API request.

What about something like :

err = fmt.Errorf("A non-successful status code was returned. Status: %d %s", resp.StatusCode, http.StatusText(resp.StatusCode))

from go-client.

The issue isn't a lack of a body, the issue is the error is getting replaced by the error coming from trying to read the empty body.

As for the error message, works for me.

from go-client.

Ok, thanks for the clarification.

So would this also solve the problem, buy only setting the err reference if not equal to io.EOF ?

if resp.StatusCode < 200 || resp.StatusCode > 299 {
  if rc.ErrorRef != nil {
    var tErr = json.NewDecoder(resp.Body).Decode(rc.ErrorRef)
    if tErr != io.EOF {
      err = tErr
    }
  }
}

If so, is there a pro or con to either approach?

I appreciate your input since I am not very Go smart.

from go-client.

Taking another thought at this, I wonder if this would be better:

if terr := json.NewDecoder(resp.Body).Decode(rc.ErrorRef); terr != nil {
	err = fmt.Errorf("A non-successful status code was returned. Status: %d %s", resp.StatusCode, http.StatusText(resp.StatusCode))
}

Since terr would be errors resulting from the reading of the data returned and not the errors returned from the api.

Thoughts?

from go-client.

When we have a non-success status code, my ideal would be:

1. If no response body, return status code only.
2. If response body, return status code and de-serialized JSON body.
3. If response body, and we fail to de-serialize JSON body, return failure message with exception that caused the failure and the original status code from the HTTP response.

1 and 2 are expected scenarios.

Ideally then, we'd try to de-serialize the response body, if it was empty we do nothing, if it fails we set the err to the new message and also print out any exception that was returned by the JSON decoder?

From looking at the example code, it looks like we are trying to deserialize the response body, and if it succeeds, terr will be non null, and then we will set err equal to the new message?

from go-client.

@robotdan I'm going to have to rethink this, io.EOF is going to be expected for body or no body.

Do you know what would cause rc.ErrorRef to not be nil at that point?

from go-client.

Not sure, I may need to test some different APIs with this code and see what we should expect and what a 401 and 404 look like in context of this error path.

from go-client.

Did some testing and more digging, in this case io.EOF is returned for only empty body.

Which we can do the following, that cover each case:

switch {
case terr == io.EOF:
	err = fmt.Errorf("A non-successful status code was returned. Status: %d %s", resp.StatusCode, http.StatusText(resp.StatusCode))
case terr == nil:
	err = fmt.Errorf("A non-successful status code was returned. Status: %d %s Response:: \n\t%v", resp.StatusCode, http.StatusText(resp.StatusCode), rc.ErrorRef)
default:
	err = fmt.Errorf("A non-successful status code was returned. Status: %d %s Error Parsing Response: %s", resp.StatusCode, http.StatusText(resp.StatusCode), terr.Error())
}

What do you think about this?

from go-client.

I wrote some tests so I can better understand what is going on... I'm still learning Go. :-) Someone else wrote this library for us.

I'll test with your suggestion and see if I can provide any sensible feedback.

from go-client.

Do you mind if I create a pr for this in the client builder repo?

from go-client.

Sure, that would be fine.

I have been swamped and haven't gotten back to this. But I wonder if this client in particular has deviated from our normal pattern.

It looks like the base response struct does not include the error type. If I recall, when "we" (someone much smarter than me) wrote the Go client, we may have decided this was a limitation of the go language.

For example, in Java, and other client libs, the response includes the status, an optional exception object, a success and a response type field.

In Go, we have this BaseHTTPResponse which only includes the StatusCode.

type BaseHTTPResponse struct {
  StatusCode int `json:"statusCode,omitempty"`
}

And then a UserResponse for example contains this BaseHTTPResponse.

type UserResponse struct {
  BaseHTTPResponse
  Token                            string                             `json:"token,omitempty"`
  User                             User                               `json:"user,omitempty"`
}

But for reference, here is the Java response.

public class ClientResponse<T, U> {
  public U errorResponse;

  public Exception exception;

  public RESTClient.HTTPMethod method;

  public Object request;

  public int status;

  public T successResponse;

  public URL url;

  public boolean wasSuccessful() {
    return status >= 200 && status <= 299 && exception == null;
  }
}

Ideally the object that is returned from the client should ALWAYS contain the StatusCode and then either a success response, which in the above example would be the User object, or an error object would be of type Errors

type Errors struct {
  FieldErrors                      map[string][]Error                 `json:"fieldErrors,omitempty"`
  GeneralErrors                    []Error                            `json:"generalErrors,omitempty"`
}

And then if an exception occurred, such as a socket timeout or something, we won't have a success or an errors response, but an exception. So then some string or other exception object would be returned. In the Java example it is in the exception field.

from go-client.

Accepted PR.

from go-client.