Skip to content

Handle specially API endpoints with only one successful response status code #276

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
mitar opened this issue Jan 12, 2021 · 4 comments
Open

Comments

@mitar
Copy link
Contributor

mitar commented Jan 12, 2021

When I generate API I get such response structure for our calls, example:

type GetClientResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	JSON200      *AppClient
	JSON400      *ApiError
	JSON401      *ApiError
	JSON402      *ApiError
	JSON404      *ApiError
	JSON500      *ApiError
}

I would prefer something like:

type GetClientResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	Success      *AppClient
	Error        *ApiError
}

First, I would not have to know exactly which 2xx status code is returned. Second, I can call response.StatusCode() to obtain the status code, there is no point in having that also part of the field name. Given that this for generating HTTP API it is pretty reasonable to assume 2xx codes are success and everything else is failure. So if there is only one successful response status code defined in the API, make it simply be named Success. And if all errors have the same type, collapse them as well into Error.

Currently I have to have such ugly code to make sense of this:

func getResponseError(response interface{}, statusCode int, body []byte) error {
	r := reflect.ValueOf(response)
	f := reflect.Indirect(r).FieldByName(fmt.Sprintf("JSON%d", statusCode))
	if !f.IsValid() {
		return errors.Errorf("Unexpected status code %d: %s", statusCode, body)
	} else if f.IsNil() {
		return errors.Errorf("Error response %d", statusCode)
	} else {
		errorMessage := f.Interface().(*ApiError).Error
		if errorMessage != nil {
			return errors.Errorf("Error response %d: %s", statusCode, *errorMessage)
		} else {
			return errors.Errorf("Error response %d", statusCode)
		}
	}
}

func GetAppClientFromResponse(response *GetClientResponse) (*AppClient, error) {
	if response.JSON200 != nil {
		return response.JSON200, nil
	} else {
		return nil, getResponseError(response, response.StatusCode(), response.Body)
	}
}

Alternatively, there could be some methods on the struct (for backwards compatibility) which would be returning those success or error values.

@deepmap-marcinr
Copy link
Contributor

I'm not a fan of the ClientWithResponses. I use the Client and unmarshal myself.

Each response code can produce a different result. This code is very naive, and just generates that list dutifully. We'd have to do some type analysis to squash responses with the same response schema. So then, how do we know to name it "successResponse" or something like that. You'd get one field for some responses, another for some more, and you'd still have to switch on response code.

I'm honestly unsure of how to make this API look nice.

@mitar
Copy link
Contributor Author

mitar commented Oct 27, 2021

I like that generator makes a boilerplate here. Maybe the easiest would be to parse into something like:

type GetClientResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	AppClient    *AppClient
	ApiError     *ApiError
}

So no need to figure out what is the meaning of each response type, just expose it directly. Also, it does not matter which status code it came through (if somebody cares, they can check the HTTPResponse).

@6ecuk
Copy link

6ecuk commented Oct 31, 2021

What about using a map with status code as key and error struct as value. That gives us more flexibility for working with errors. Currently, it's really hard to get the right errors by status code.

type GetClientResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	AppClient    *AppClient
	Error          map[int] *ApiError
}

@mitar
Copy link
Contributor Author

mitar commented Nov 3, 2021

I am not sure what why would a map help here? For every response there would always be just one entry in that map? So that can then be simply two fields instead of a map, a StatusCode int and Error *ApiError. But StatusCode is easy to get from HTTPResponse.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants