[go-server] Allow user to specify how errors are handled #9764

lwj5 · 2021-06-14T13:29:11Z

Added error.go with an error handler
Updated controller template to use the error handler
Backwards compatible with previous version

Tested on both go-server with mux and chi routers

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master, 5.1.x, 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Hi @antihax @grokify @kemokemo @jirikuncar @ph4r5h4d, please help to take a look at this PR

lwj5 · 2021-06-17T06:45:57Z

continuous-integration/appveyor/pr looks like a c# error and is unrelated

ph4r5h4d · 2021-06-17T19:27:34Z

I'd like to see more feedback on this PR. I'm not getting what the error handler is trying to solve or improve.

lwj5 · 2021-06-18T06:09:01Z

Sure! Let's assume there are 2 types of users of the generator:

those that edit the generated files, and
those that extend (or embed) the generated files.

For type 1 users

If their specifications require them to return errors with fields like title and details. You would need to replace all instances of w.WriteHeader(http.StatusBadRequest) with your own. This is tedious and will require you to exclude the file from further regeneration as it will be overwritten. If the concern of error handling is separated from the controller, it would mean that most (if not all) of the time, the controller can be left as-is.

Modifying error.go provides a way convenient to customize the error responses without changing the controller, separating their concerns.

Second, let's say you have a debug mode in your application. If debug is turned on, the details of the error responses are more verbose, and if off, it merely returns "unexpected error". error.go provides a convenient place to process the errors returned from the servicer before the response is sent.

error.go provides a centralized place to process the errors returned from the servicer.

Case 2 is where it shines

Where I work, we do not modify any of the generated files. Instead, we use overlays that embed the servicer. If we have such a spec as laid out in the first point, we will need to reimplement the controller. In addition to the tediousness, this process is extremely error-prone especially if the specs are changing. The concerns could be cleanly separated with an injection of an ErrorHandler.

Instead of reimplementing the controller, an appropriate ErrorHandler can be injected into the controller together with the servicer NewController(s, errorHandlerOpt)

To sum it up, ErrorHandler has a very specific job of handling error, like how controller and servicer have very specific roles. By separating the concerns and implementing an injection pattern, it makes the generated server more extensible.

Aside from this, EncodeJSONResponse(err.Error(), &result.Code, result.Headers, w) is quite concerning as most errors are plain text. But with this merged, it's easier to swap it out in the next PR.

Let me know if you have any questions!

ph4r5h4d · 2021-06-20T15:37:15Z

Thank you @lwj5, Now I get a clearer picture of what you had in mind when you submitted this PR.
I'll try my best to test it in my local and verify the functionality as fast as possible (hence I'm a bit busy these days and sorry for that).

P.S: In the meantime, I'd like to know others' opinions regarding this change.

lwj5 · 2021-06-21T05:19:22Z

No worries, thanks for doing the review

wing328 · 2021-06-21T05:30:46Z

@lwj5 thanks for the detailed explanation. One minor suggestion is to add some comments in https://github.com/OpenAPITools/openapi-generator/pull/9764/files#diff-40523ac4e3f7daf99f002d095c93bb305acc3a313f05b8160654207a3661a942 explaining what those functions do so as to onboard new Go users more easily.

lwj5 · 2021-06-22T04:28:45Z

I rebased it with the master, the last 3 commits are new.

Added docs
Remove variadic leads to cleaner code
Regenerated files

@lwj5 thanks for the detailed explanation. One minor suggestion is to add some comments in https://github.com/OpenAPITools/openapi-generator/pull/9764/files#diff-40523ac4e3f7daf99f002d095c93bb305acc3a313f05b8160654207a3661a942 explaining what those functions do so as to onboard new Go users more easily.

Done 👍

lwj5 · 2021-06-28T12:57:21Z

I've been using this branch with a custom error handler for a bit, it is working as intended and I've managed to inject debug and stack trace information to err responses originating from the servicers. This is all done without the need for the controller or the servicer to know whether the debug flag is set.

lwj5 · 2021-07-09T05:50:23Z

@wing328 @ph4r5h4d any updates on this? I would like this merged so I can continue working on #9647 to further the goal of #426 and hopefully to meet #1931, #3547

ph4r5h4d · 2021-07-09T07:42:49Z

@wing328 @ph4r5h4d any updates on this? I would like this merged so I can continue working on #9647 to further the goal of #426 and hopefully to meet #1931, #3547

I'm sorry for the delay. It's been a hectic couple of weeks for me. I'll plan to test it today or at most tomorrow.

modules/openapi-generator/src/main/resources/go-server/controller-api.mustache

ph4r5h4d · 2021-07-12T09:12:01Z

samples/server/petstore/go-api-server/go/error.go

+		EncodeJSONResponse(err.Error(), func(i int) *int { return &i }(http.StatusBadRequest), map[string][]string{}, w)
+	} else {
+		// Handle all other errors
+		EncodeJSONResponse(err.Error(), &result.Code, result.Headers, w)


The same can be done like the if part.

similar to the above. I'm ok with changing it

lwj5 · 2021-07-12T09:26:15Z

samples/server/petstore/go-api-server/go/error.go

+func DefaultErrorHandler(w http.ResponseWriter, r *http.Request, err error, result *ImplResponse) {
+	if _, ok := err.(*ParsingError); ok {
+		// Handle parsing errors
+		EncodeJSONResponse(err.Error(), func(i int) *int { return &i }(http.StatusBadRequest), map[string][]string{}, w)


This is how all errors are being handled by the controller prior to the PR. I'm okay with changing it, but it would mean a change in the implementation for those currently using the generator.

ph4r5h4d · 2021-07-12T09:31:49Z

samples/server/petstore/go-api-server/go/error.go

+	if _, ok := err.(*ParsingError); ok {
+		// Handle parsing errors
+		EncodeJSONResponse(err.Error(), func(i int) *int { return &i }(http.StatusBadRequest), map[string][]string{}, w)
+	} else {


It's up to you then :)

ph4r5h4d · 2021-07-12T09:33:51Z

This PR LGTM, I've tested it locally, and it works fine.
I can approve it now @lwj5 if all the changes are in place.

And sorry for the delay :)

lwj5 · 2021-07-12T09:37:29Z

Yes, changes completed. It's better to create a new PR for changing the default error handling (if anyone wants to).

No worries, thank you very much for reviewing

ph4r5h4d · 2021-07-12T09:38:30Z

@lwj5 I think you need to resubmit the samples.
Please run 'bin/generate-samples.sh' locally and commit changes (UNCOMMITTED CHANGES ERROR)
One pipeline is failing, kindly run the commands locally and see if they are changes to submit.

lwj5 · 2021-07-12T09:38:58Z

Let me run that now

ph4r5h4d · 2021-07-12T11:53:48Z

@wing328 LGTM

lwj5 · 2021-07-28T04:35:59Z

@wing328

wing328 · 2021-07-28T09:19:25Z

CircleCI failure not related to this change.

lwj5 marked this pull request as draft June 14, 2021 13:33

lwj5 force-pushed the goservererror branch from e2ba3d3 to d9a395b Compare June 14, 2021 14:11

lwj5 marked this pull request as ready for review June 14, 2021 14:11

lwj5 force-pushed the goservererror branch from d9a395b to db536c9 Compare June 16, 2021 15:33

wing328 added Enhancement: General Server: Go labels Jun 21, 2021

lwj5 added 6 commits June 22, 2021 10:37

Added error.go

b1cc9a5

Updated controller to use error

f84a9ab

Fix redeclared func

93d889b

Add generated files

b35358e

Merge

b0df55e

Added docs

280a0ab

lwj5 force-pushed the goservererror branch from db536c9 to 2dbba9d Compare June 22, 2021 04:24

lwj5 added 2 commits June 22, 2021 12:26

Remove variadic

3d8efc7

Regenerate files

cf24d19

lwj5 force-pushed the goservererror branch from 2dbba9d to cf24d19 Compare June 22, 2021 04:27

ph4r5h4d reviewed Jul 12, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/go-server/controller-api.mustache Outdated Show resolved Hide resolved

ph4r5h4d reviewed Jul 12, 2021

View reviewed changes

lwj5 commented Jul 12, 2021

View reviewed changes

Updated go doc to meet standard

ea3851d

ph4r5h4d reviewed Jul 12, 2021

View reviewed changes

Updated doc to meet go standard

fcd045f

Updated

4d6daa3

ph4r5h4d approved these changes Jul 12, 2021

View reviewed changes

wing328 added this to the 5.2.1 milestone Jul 28, 2021

wing328 merged commit 870ef3a into OpenAPITools:master Jul 28, 2021

lwj5 mentioned this pull request Aug 3, 2021

[REQ] [go-server] Services knowing about HTTP is questionable #9647

Open

lwj5 deleted the goservererror branch March 31, 2023 06:05

lwj5 restored the goservererror branch March 31, 2023 06:05

lwj5 deleted the goservererror branch March 31, 2023 06:06

lwj5 restored the goservererror branch March 31, 2023 06:06

lwj5 deleted the goservererror branch March 31, 2023 06:07

Uh oh!

[go-server] Allow user to specify how errors are handled #9764

[go-server] Allow user to specify how errors are handled #9764

Uh oh!

Conversation

lwj5 commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

lwj5 commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ph4r5h4d commented Jun 17, 2021

Uh oh!

lwj5 commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For type 1 users

Case 2 is where it shines

Uh oh!

ph4r5h4d commented Jun 20, 2021

Uh oh!

lwj5 commented Jun 21, 2021

Uh oh!

wing328 commented Jun 21, 2021

Uh oh!

lwj5 commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lwj5 commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lwj5 commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ph4r5h4d commented Jul 9, 2021

Uh oh!

Uh oh!

ph4r5h4d Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

lwj5 Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwj5 Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ph4r5h4d Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

ph4r5h4d commented Jul 12, 2021

Uh oh!

lwj5 commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ph4r5h4d commented Jul 12, 2021

Uh oh!

lwj5 commented Jul 12, 2021

Uh oh!

ph4r5h4d commented Jul 12, 2021

Uh oh!

lwj5 commented Jul 28, 2021

Uh oh!

wing328 commented Jul 28, 2021

Uh oh!

Uh oh!

lwj5 commented Jun 14, 2021 •

edited

Loading

lwj5 commented Jun 17, 2021 •

edited

Loading

lwj5 commented Jun 18, 2021 •

edited

Loading

lwj5 commented Jun 22, 2021 •

edited

Loading

lwj5 commented Jun 28, 2021 •

edited

Loading

lwj5 commented Jul 9, 2021 •

edited

Loading

lwj5 Jul 12, 2021 •

edited

Loading

lwj5 Jul 12, 2021 •

edited

Loading

lwj5 commented Jul 12, 2021 •

edited

Loading