I am building an API server in golang with mongodb. My main.go
file looks something like this:
func main() {
r := mux.NewRouter()
question := r.PathPrefix("/question").Subrouter()
question.HandleFunc("", handler.PostQuestion).Methods("POST")
question.HandleFunc("/{id}", handler.UpdateQuestion).Methods("PUT")
question.HandleFunc("/{id}", handler.DeleteQuestion).Methods("DELETE")
question.HandleFunc("/{id}", handler.GetQuestion).Methods("GET")
r.HandleFunc("/questions", handler.GetAllQuestions).Methods("GET")
if err := http.ListenAndServe(":8080", r); err != nil {
log.Fatalf("could not listen on port 8080 %v", err)
}
}
Every handler has some logic which changes data in MongoDB. Regardless of what I am doing with the question collection, this is common in all the handlers.
client, _ := mongo.NewClient(options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
err := client.Connect(ctx)
if err != nil {
panic(err)
}
defer client.Disconnect(ctx)
collection := client.Database("myapp").Collection("questions")
These functions are in handler/handler.go
.
As you can see, I am using the official Mongo driver. What is the best kind of refactor which can be done?
-
\$\begingroup\$ Is the title any better? \$\endgroup\$Santosh Kumar– Santosh Kumar2020年05月05日 16:32:57 +00:00Commented May 5, 2020 at 16:32
1 Answer 1
You say this is common in all handlers:
client, _ := mongo.NewClient(options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
err := client.Connect(ctx)
if err != nil {
panic(err)
}
defer client.Disconnect(ctx)
collection := client.Database("myapp").Collection("questions")
So this means you're connecting and disconnecting to MongoDB for each request. That's incredibly wasteful. The MongoDB client is safe for concurrent use, so you can reuse the connection. Because I see code like this:
question.HandleFunc("", handler.PostQuestion).Methods("POST")
I know your handler functions are methods, so why not add the client to your handler type:
type yourHandler struct {
coll *mongo.Collection
}
Then, in your handlers you can simply access:
func (h *yourHandler) PostQuestion(w http.ResponseWriter, r *http.Request) {
if err := h.coll.InsertOne(r.Context(), ...); err != nil {
// write error response
return
}
}
Now your main function will look a bit different, seeing as we're not disconnecting the mongo client in the handler anymore, and we're not using a context with an arbitrary timeout. I suggest something like this:
func main() {
// create the base context, the context in which your application runs
ctx, cfunc := context.WithCancel(ctx.Background())
defer cfunc()
client, err := mongo.NewClient(
options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
if err != nil {
log.Fatalf("Error creating mongo client: %+v", err)
}
defer client.Disconnect(ctx) // add disconnect call here
if err := client.Connect(ctx); err != nil {
log.Fatalf("Failed to connect to MongoDB: %+v", err)
}
// set collection on handler
handler.coll = client.Database("myapp").Collection("questions")
// rest of the code here
}
So now, whenever the main function returns, the application context is cancelled (which is used to connect the client), and client.Disconnect
is called. The handler has access to the collection you're using, removing a lot of duplicate code, and removing the overhead of constantly connecting and disconnecting to MongoDB.
Inside your handler, I'm not using context.WithTimeout(context.Background(), 10*time.Second)
. Instead I'm using the request context, which means once the request context is cancelled, the query context is. You could still set a time-limit if you want:
func (h *yourHandler) PostQuestion(w http.ResponseWriter, r *http.Request) {
// context expires if request context does, or times out in 3 seconds
ctx, cfunc := context.WithTimeout(r.Context(), time.Second * 3)
defer cfunc() // good form to add this
if err := h.coll.InsertOne(ctx, ...); err != nil {
// error response etc...
return
}
// success response
}
Currently our handler requires the field coll
to be of the type *mongo.Collection
, which makes it harder to test your code. Instead, you might want to change that field to take an interface:
//go:generate go run github.com/golang/mock/mockgen -destination mocks/collection_interface_mock.go -package mocks your.module.io/path/to/handler/package Collection
type Collection interface{
InsertOne(ctx context.Context, document interface{}, opts ...*options.InsertOneOptions) (*mongo.InsertOneResult, error)
Name() string
// all methods you need
}
In your unit tests, you can now inject a mock collection interface, that you can control, allowing you to simulate error returns, etc...
Instead of exposing the coll
field on your handler, then, you'll also want to create a constructor function to inject the dependencies of what I have in the main function above (handler.coll = ...
):
package service
type Collection interface{} // interface as above, with go generate comment
type handler struct {
coll Collection
}
func NewHandler(c Collection) *handler {
return &handler{
coll: c,
}
}
I'll leave you with this as a starting point. Just one thing I've not yet mentioned: you're using log.Fatalf
when http.ListenAndServe
returns an error. That's fine. Why are you using panic(err)
when client.Connect(ctx)
fails?
Panic is something you should avoid as much as possible. Especially here: it adds very little (to no) value: the only information a panic dump will yield is whereabouts in the mongo package something went awry. You don't control that package, so just log.Fatalf("Mongo error: %+v", err)
is to be preferred.
More details
Seeing as your handler functions are actually functions in a package, rather than methods (which I initially thought they were):
I'd refactor the handler
package to contain a constructor, and have the actual handler functions as methods on a type. Something like:
package handler
type Collection interface{
InsertOne(ctx context.Context, document interface{}, opts ...*options.InsertOneOptions) (*mongo.InsertOneResult, error)
Name() string
// and so on
}
// main handler type
type Handler struct {
coll Collection
}
// New returns the handler
func New(c Collection) *Handler {
return &Handler{
coll: c,
}
}
func (h *Handler) PostQuestion(w http.ResponseWriter, r *http.Request) {
if err := h.coll.InsertOne(r.Context(), ...); err != nil {
log.Errorf("Insert question error: %+v", err)
// write response
return
}
// success response
}
From your main
package, this will look like this:
func main() {
ctx, cfunc := context.WithCancel(ctx.Background())
defer cfunc()
client, err := mongo.NewClient(
options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
if err != nil {
log.Fatalf("Error creating mongo client: %+v", err)
}
defer client.Disconnect(ctx) // add disconnect call here
if err := client.Connect(ctx); err != nil {
log.Fatalf("Failed to connect to MongoDB: %+v", err)
}
// create handler with collection injected
myHandler := handler.New(client.Database("myapp").Collection("questions"))
// configure router to call methods on myHandler as handler functions
// the handlers will now have access to the collection we've set up here
r := mux.NewRouter()
question := r.PathPrefix("/question").Subrouter()
question.HandleFunc("", myHandler.PostQuestion).Methods("POST")
// and so on
}
-
\$\begingroup\$ Thank you for your kind reply. I am relatively new to the golang (coming from the python) and your answer is very insightful. One question. When you did
handler.coll = ...
in main.go. Did you meanyourHandler.coll
? Because I have a package of the same name. \$\endgroup\$Santosh Kumar– Santosh Kumar2020年05月08日 11:31:04 +00:00Commented May 8, 2020 at 11:31 -
\$\begingroup\$ And when I do
handler.MyHandler.coll = ...
, the linter complaintstype handler.MyHandler has no field or method collection
. \$\endgroup\$Santosh Kumar– Santosh Kumar2020年05月08日 11:32:25 +00:00Commented May 8, 2020 at 11:32 -
\$\begingroup\$ @SantoshKumar If
handler
is a package name, then your handler functions clearly are just functions, not receiver functions/methods. Declare a type in the handler package, have aNew
function in htere to which you pass the collection (constructor style injection), and then assign (e.g.myHandler := handler.New(collection)
). Then replace handler functions with methods (handler.PostQuestion
->myHandler.PostQuestion
) \$\endgroup\$Elias Van Ootegem– Elias Van Ootegem2020年05月08日 12:47:48 +00:00Commented May 8, 2020 at 12:47 -
\$\begingroup\$ Added a bit more details to the answer, showing what the type in the handler package would look like, and how to use it in your
main
func. Accessing a typehandler.MyHandler.coll
isn't correct (you access fields on an instance, not on a type name). thecoll
field is (rightfully) not exported, and cannot be set from themain
package, so you'll need to pass that dependency via a constructor (like I did). \$\endgroup\$Elias Van Ootegem– Elias Van Ootegem2020年05月08日 12:57:51 +00:00Commented May 8, 2020 at 12:57 -
\$\begingroup\$ I am getting an error at Collection interface definition. On
bson.M
I'm gettingexpected type, found '.'
Is this the same signature from mongo docs, because in Mongo docs, InsertOne returns two value? \$\endgroup\$Santosh Kumar– Santosh Kumar2020年05月08日 17:40:18 +00:00Commented May 8, 2020 at 17:40