Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate chi based http handler #51

Merged
merged 18 commits into from
Sep 3, 2019

Conversation

dannymidnight
Copy link
Contributor

@dannymidnight dannymidnight commented Aug 19, 2019

This is a first pass at adding chi support following on from #2 to generate an alternative to the echo based routing. There's still a bunch of missing pieces but I wanted to push it up early for general feels.

Why?

The current http handler solution is built around echo and requires the calling application to bring along this dependency. Chi is "100% compatible with net/http" making it self-contained and simple to integrate with existing go applications whilst also leaning heavily into the core context API making it more "go-like".

Example

Here's an example of how you could go about registering your operations. You'll note that there's no peer dependency on chi as ChiHandler returns a http.Handler.

Implementation is definitely a little verbose having raw request, response and passing around Context but I think the http.Handler interface will be more flexible in the long run and should be familiar for most go developers.

type Operations struct {}

func (*Operations) FindPets(w http.ResponseWriter, r *http.Request) {
  params := petstore.ParamsForFindPets(r.Context())
  ...
}

func (*Operations) DeletePet(w http.ResponseWriter, r *http.Request) {
  params := petstore.ParamsForFindPets(r.Context())
  ...
}

func (*Operations) AddPet(w http.ResponseWriter, r *http.Request) {
  ...
}

func (*Operations) FindPetById(w http.ResponseWriter, r *http.Request) {
  ...
}

func Handler() {
  myApi := Operations{}
  petstore.ChiHandler(myApi)
}

Questions

  • Is this something you're open to incorporating?! 馃榿
  • Rename ChiHandler to Handler? Chi is really an implementation detail that doesn't need to be exposed externally.

TODO:

  • Separate out templates from existing echo based ones.
  • Add new flag for generating chi http handler
  • Add example implementation
  • Add some test coverage!

@deepmap-marcinr
Copy link
Contributor

I've been thinking about this, and it's hard to do it cleanly, but your approach looks pretty nice.

Right now, we have a fair bit of code that's router agonstic (all the type generation), and what's echo specific is the server interface (the client interface is agnostic). So, I would refactor the functionality so that the entirety of Echo bindings are in, say, echo-serverinterface.tmpl and echo-wrappers.tmpl, and you could produce chi-specific templates. You would select the backend via a command line option, which would substitute in chi-whatever.tmpl in place of those two files.

I initially chose Echo because it's quick, and it simplifies a lot of stuff, and it works for us, but I'm not particularly attached it to one way or another.

@deepmap-marcinr
Copy link
Contributor

One practical thing that I've found is that it's good to have an interface to implement in your generated code - for mocking, or whatever. So your operations{} struct might want to implement an interface.

I'm totally open to incorporating this, if we can constrain the chi-specific code to a few .tmpl files, just as we've done with Echo.

@dannymidnight
Copy link
Contributor Author

Great. I'll try clean things up a bit and let you know when it's ready for review.

@dannymidnight
Copy link
Contributor Author

One practical thing that I've found is that it's good to have an interface to implement in your generated code - for mocking, or whatever. So your operations{} struct might want to implement an interface.

Yep makes sense, in my petstore example the operations struct is implementing this interface.

@dannymidnight
Copy link
Contributor Author

Hey @deepmap-marcinr I think this is now ready for review.

Major changes since last time:

  • There's now a chi-server flag for generating the new implementation and I've kept the echo codepath using server for backwards compatibility.
  • I've added an example implementation and test runner following a similar approach to the echo based tests.
  • I've split the petstore examples into the following structure with the client split out from the server implementations:
- petstore
-- chi
-- echo
-- petstore-client.gen.go

Let me know what you think! Any and all feedback welcome.

Copy link
Contributor

@deepmap-marcinr deepmap-marcinr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work! Thank you.

store := api.NewPetStore()
h := api.Handler(store)

t.Run("Add pet", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's an interesting way to structure tests. I've not seen this approach before.

@deepmap-marcinr deepmap-marcinr merged commit 026c5c9 into oapi-codegen:master Sep 3, 2019
@dannymidnight
Copy link
Contributor Author

Cheers @deepmap-marcinr!

@ashwinrs
Copy link
Contributor

Thank you for this @dannymidnight

adrianpk pushed a commit to foorester/oapi-codegen that referenced this pull request Jan 16, 2024
* Generate go-chi routes

* Add http handler with registered chi routes

* Add go-chi import

* Fix registering handler

* Remove regex for path params

* Add param parsing middleware

* Add methods for retrieving params

* Add errors

* Include headers and cookies in params

* Fix some bad templates

* Add context middleware for required params

* Rename param method for clarity and better naming.

* Split chi and echo code generation

* Generate code

* Add example chi server implementation

* Test example chi server

* Remove "chi" prefix from handler

* Generate latest code

* Use test groups for petstore test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants