Skip to content

[go-server] Enhance Go API server with interfaces router binding and services #4038

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 19, 2019

Conversation

Jesse0Michael
Copy link
Contributor

@Jesse0Michael Jesse0Michael commented Oct 3, 2019

Enhance the default go api server generation to define interfaces for an API's routes and services. Handle an endpoint's http binding in the generated router and the skeleton for the service logic in an API service.

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Made some changes to the go-api-server code generations build out router and service interfaces.
The Router interface implementation handles the http request/response binding.
The service interface implementation provides a skeleton for the server logic (which can easily be updated to include any data access services or whatever you might need in your server logic)
I'm not sure if this a minor or major change, but I have set it against 4.2.x

The way of separating the business logic into services and route bindings into routers and using interfaces is closer to how I would normally create apis in go. So I thought this might be helpful to others as well. Please let me know if there anything I am doing incorrectly or anything I should change to match any openapi design principles.

There are still improvements that can be made (like a better way of handling errors by default.) But I thought this was far enough along and working well enough to begin a discussion! Samples generated with go-petstore-server.sh. Thanks!

Enhance the default go api server generation to define interfaces for an API's routes and services. Handle an endpoint's http binding in the generated router and the skeleton for the service logic in an API service.
@Jesse0Michael
Copy link
Contributor Author

@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

@Jesse0Michael Jesse0Michael changed the title Enhance go api server with interfaces router binding and services [go-server] Enhance go api server with interfaces router binding and services Oct 3, 2019
@antihax
Copy link
Contributor

antihax commented Oct 3, 2019

I agree, this looks to allow the logic to be provided by another package and could also be used to supply different versions of BL such as mocks for testing.

It may be worth having it spit out docs of the interfaces to aid in implementation?

@Jesse0Michael
Copy link
Contributor Author

@antihax Added some additional documentation to the interfaces. Let me know if you had something different in mind, I'd be happy to change it.

@Jesse0Michael
Copy link
Contributor Author

Not sure if it's rude to bump, but: @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07) 👆

@wing328 wing328 changed the base branch from 4.2.x to master October 9, 2019 16:32
@wing328
Copy link
Member

wing328 commented Oct 9, 2019

Retargetting the PR to "master", which is for the 4.2.0 release

@wing328
Copy link
Member

wing328 commented Oct 9, 2019

I'm not sure if this a minor or major change, but I have set it against 4.2.x

Is it a breaking changes with or without fallback?

If it's without fallback, then we need to target the change to 5.0.x branch instead.

@Jesse0Michael
Copy link
Contributor Author

Jesse0Michael commented Oct 9, 2019

@wing328 It doesn't break the generated server in anyway. The server generated by this change is still functionally and usably the same.

But if someone was customizing any of the templates before, their customizations might not work the same way with these template changes. Depending on what they were doing.

@Jesse0Michael
Copy link
Contributor Author

Is there anything I should update on this? Is it just needing approval from the technical committee?

@wing328 wing328 changed the title [go-server] Enhance go api server with interfaces router binding and services [go-server] Enhance Go API server with interfaces router binding and services Oct 19, 2019
@wing328 wing328 added this to the 4.2.0 milestone Oct 19, 2019
@wing328 wing328 merged commit 0abb910 into OpenAPITools:master Oct 19, 2019
@Jesse0Michael Jesse0Michael deleted the extend-openapi-go-server branch October 19, 2019 18:38
@wing328
Copy link
Member

wing328 commented Oct 31, 2019

@Jesse0Michael thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

@mapero
Copy link
Contributor

mapero commented Jan 7, 2020

Hi @Jesse0Michael
the abstraction provided in this pull request is a way in the right direction, but this is definitely not a minor change (e.g. methods instead of functions). Since it totally changes the structure of the go code. What also bothers me, is that I miss essential things that should have been included before merging:

  • Custom error codes and messages
  • Non-JSON responses
  • Status codes other than 200

All that is missing. I hope, that this will be added in the near future.

For now, we will stick with 4.1.

Best regrads
mapero

@Jesse0Michael
Copy link
Contributor Author

@mapero I don't believe any of those requested features were possible through the generated server code before my changes.

The Exported functions generated and mapped to the routes previously looked like

func UpdatePetWithForm(w http.ResponseWriter, r *http.Request) {
	w.Header().Set("Content-Type", "application/json; charset=UTF-8")
	w.WriteHeader(http.StatusOK)
}

And continue to default to a 200 "application/json" response.

And the internal go structure has changed to add interfaces for web bindings and service logic. The API server that is generated has no changes externally, and no conflicts, so would not be seen as a major change.

The only conflict would be any custom logic that would override what is generated, and custom logic that overrides whats generated cannot be considered backward compatible.


But I do look forward to your PR for the requested changes, I am all for them!
When I looked into custom responses I found it difficult to build without creating assumptions that do not exist in the OPEN API specification and did not proceed, So I look forward to seeing your approach on the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants