[BUG][GO][Client] Returned err is never nil

[thiagoarrais]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• Have you tested with the latest master to confirm the issue still exists?
• Have you searched for related issues/PRs?
• What's the actual output vs expected output?

Description

When calling any generated Execute() function, a non-nil error is always returned. Instead of using the usual Go idiom:

result, _, err := r.Execute()
if err != nil {
	/* deal with error */
}

I need to check the actual error message in order to detect errors:

result, _, err := r.Execute()
if err != nil && err.Error() != "" {
	/* deal with error */
}

openapi-generator version

c322084 (current main branch)

Related issues/PRs

I believe this was introduced by #8137. Maybe it is intended behaviour. Care to weigh in here, @code-lucidal58?

Suggest a fix

Successful execution of *Execute() should not return a non-nil error.

[code-lucidal58]

Execute() function returns 3 values: Go object that you are trying to create, the response and error (if any). The last value is of type GenericOpenAPIError. struct code is here. Instead of checking the nullablity of this error (since this is not Golang error), the error property of GenericOpenAPIError should be verified. This property is of type string and will be empty in the situation when no error has occurred. The usage has been shown here #8137 (comment)

[thiagoarrais]

What if we return a *GenericOpenAPIError pointer (allowing it to be nil)? That way users will be able to use the usual err != nil idiom. It may even be declared as error in the function header since it satisfies the error interface. Will that interfere with the response body?

[code-lucidal58]

pointer to a struct (or any variable) will never be nil. https://play.golang.org/p/53vSwOYcCV0 even if it's an empty struct. The return type was earlier error. GenericOpenAPIError was typecasted to error. This lead to the loss of values stored in body and model. error supports only one method Error(), neitherBody() nor Model().

[thiagoarrais]

But one can (knowing that the generated code's error is actually a GenericOpenAPIError) define an interface to use those methods (playground link). I may be misunderstanding your point, though...

[cheshir]

The 5.0.0-beta3 version has native for go developers behaviour – methods return error type. Not the custom structure.
GenericOpenAPIError.Error() method confuses because it uses the same signature as an error interface. So I and many other developers think that we can use it in the same way but its not true.

It would be awesome if you will alow users to configure generator to select desired behaviour: generate methods with custom error or with error interface that covered custom error type.

[alholm]

since this is not Golang error.

Excuse me, but what does it mean? Which one this error is then? It implements standard lib error interface, so one would expect standard behaviour, including nil comparison. Since this is literally the breaking change, may it at least be done as in previous versions - without local var executionError and returning nil on successful path?

[thiagoarrais]

Since there seems to be some interest here, I may be able to offer a PR reverting the return type back to error while still keeping the response body accessible. As I understand, this is the main issue raised by #8136 anyway.

[cheshir]

@thiagoarrais I believe there should not be a problem.

In most of the cases we don't need access to the response body if we got an error.
But we can use type assertion if we need to do this.

[thiagoarrais]

For my use case I do use the Model() method, since the API I'm working with returns a JSON object with details in case of non-2xx responses (and I believe that is common). The model is read from the response body, but yeah I don't need access to Body() directly.

For now I'm using the strategy I've delineated in the playground link above and that has been enough for me.

I've thought about publishing a detailed error interface within the generated client:

type OpenapiError interface {
	Error() string
	Model() interface{}
	Body() []byte
}

and returning it instead of error:

func (a *MyService) MyOperationExecute(r MyOperationRequest) (MyResponse, *_nethttp.Response, OpenapiError) {
	// ...
} 

But I'm not really sure if that will be confusing. It would work like an everyday error (respecting comparison with nil and all), but would give access to those specialized methods without type assertion.

[code-lucidal58]

since this is not Golang error.

Excuse me, but what does it mean? Which one this error is then? It implements standard lib error interface, so one would expect standard behaviour, including nil comparison. Since this is literally the breaking change, may it at least be done as in previous versions - without local var executionError and returning nil on successful path?

GenericOpenAPIError is not an implementation to the GoLang error. This is a custom error struct. I need the reponse body to understand the cause for the non-2xx status code.

@cheshir Body is required for non-2xx status code response because it will have the message which will indicate the cause to the failure. Using type assertion to convert GenericOpenAPIError to error will trim the values in body and model. the second type assertion after Execute, from error to GenericOpenAPIError will be useless.

@thiagoarrais Why not by keeping everybody's interest in mind, return a pointer to an empty struct of GenericOpenAPIErrorwhen status code is 2xx, change the return type of Execute to *GenericOpenAPIError in place of GenericOpenAPIError and leave the remaining as it is. This way if no error occurred you can use err==nil and if not not nill, check for reasons in body and model. Even passing an additional parameter showError to Execute will not be required

[cheshir]

@code-lucidal58 thanks for the detailed response. Can I ask you to provide example of trimming data after type assertion? I can't imagine what it could be.

Here is my example of how it could be: example.

The idea of returning wide interface (like @thiagoarrais proposed) is also ok to me.

[code-lucidal58]

@cheshir Thank you for correcting my concept here. I still think that *GenericOpenAPIError as return type is better than error, as that will prevent type assertions or type checks after Execute function ends with non 2xx code.

[cheshir]

@code-lucidal58, the semantic of golang error will be broken. Some part of the community uses codegen to generate wrappers around the API clients for writing logs, metrics, traces etc. They highly depend on the error type.
E.g. gowrap template for opentracing.

Let's look closer to the idea of returning error type:
If you care about readability – we can use a helper function to extract necessary data using type assertion.
If you care about performance – I'm sure that single type assertion Is not the main performance issue in call processing.

It would be great to hear thoughts of the other community members.

[zippolyte]

I'm in favor of reverting to the previous behaviour to keep the semantics of golang error.

I think the helper function is a good way to improve readability and avoid too many code duplications when dealing with errors.
This is what is done in our terraform provider for instance, which uses a generated go client:

Cheers

[Alphasite]

This current approach can lead to bugs elsewhere in the code, it leads to really nasty boilerplate everywhere (even worse than the occasional cast):

	ns, _, err := client.NamespaceApi.GetNamespace(ctx, namespace).Execute()
	if err.Error() != "" {
		return ns, err
	}

	return ns, nil

If you really want to make the Error easily accessible, embed it in an http.Response like object, which is already being returned.

[thiagoarrais]

That is very close to what we did on our terraform provider too, @zippolyte (can't link to code because it is still closed source, though). And we ran for some time without the helper function because the Error() message was sufficient.

In my PR (#8344) I've included sample code with that strategy to make it clear how to get the Body() and Model() fields from the returned error. I'll even change the type assertion to GenericOpenAPIError (I have been using a client-owned interface) just like datadog's terraform provider, since it saves some keystrokes.

[wing328]

I've filed #8427 to revert #8137 . Please review when you guys have time.

[thiagoarrais]

Closing because (as of #8427) this is fixed in the main branch . But #8439 is important to avoid regressing the original issue (#8136).