1
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ Is the title any better? \$\endgroup\$ May 5, 2020 at 16:32

1 Answer 1

3
\$\begingroup\$

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
}
\$\endgroup\$
6
  • \$\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 mean yourHandler.coll? Because I have a package of the same name. \$\endgroup\$ May 8, 2020 at 11:31
  • \$\begingroup\$ And when I do handler.MyHandler.coll = ..., the linter complaints type handler.MyHandler has no field or method collection. \$\endgroup\$ 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 a New 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\$ 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 type handler.MyHandler.coll isn't correct (you access fields on an instance, not on a type name). the coll field is (rightfully) not exported, and cannot be set from the main package, so you'll need to pass that dependency via a constructor (like I did). \$\endgroup\$ May 8, 2020 at 12:57
  • \$\begingroup\$ I am getting an error at Collection interface definition. On bson.M I'm getting expected type, found '.' Is this the same signature from mongo docs, because in Mongo docs, InsertOne returns two value? \$\endgroup\$ May 8, 2020 at 17:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.