0
\$\begingroup\$

I have used this approach in my Golang MongoDB REST APIs' DAO layer, I need to clarify whether this is a good way or not, code is as follows,

func CreateUser(user *models.User) *commons.RequestError {
    ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
    defer cancel()
    DB := connections.Connect()
    rst, err := DB.Collection("user").InsertOne(ctx, *user)
    if err != nil {
        commons.ErrorLogger.Println(err.Error())
        connections.Disconnect(DB)
        return &commons.RequestError{StatusCode: http.StatusBadRequest, ErrorOccurredIn: "user_dao 
        CreateUser", Err: err.Error()}
    }
    user.Id = rst.InsertedID.(primitive.ObjectID)
    connections.Disconnect(DB)
    return nil
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Right, so there's quite a number of things that I would consider questionable here. Let's go through the code line by line and see what could be done differently:

func CreateUser(user *models.User) *commons.RequestError {

Seeing as you stated that this is a function that is part of a REST API, the common approach here would be to pass in the request context. If for whatever reason the request context is cancelled, you want that cancellation to be propagated throughout. This includes using a context.WithCancel(context.Background()) in your main function, BTW. You're returning a pointer to some *commons.RequestError here, too. That's bad separation of concern. This function is concerned with taking DTO/entities and storing/fetching them from an underlying datastore. A RequestError suggests that the function is there to handle requests, which is the handler's job. I also don't see why you'd return an explicit type as an error. Implement the Error() string function on the commons.RequestError type, and you can simply use it as an error type. All in all, the function should look something more like this:

func CreateUser(ctx context.Context, user *models.User) error {

Now you used to create a context with timeout, which you can still do no problem, but instead of wrapping the context.Background(), you'd wrap the argument passed in, so:

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

Becomes

tCtx, cancel := context.WithTimeout(ctx, 10* time.Second)
defer cancel()

But if you pass in the context from a request, wrapping things in a timeout isn't that big of a requirement, because with a request context, if the connection times out, so does the context, and thus the cancellation of the context will be propagated. Leave the explicit timeout in or not, both are fine

DB := connections.Connect()
rst, err := DB.Collection("user").InsertOne(ctx, *user)
if err != nil {
    commons.ErrorLogger.Println(err.Error())
    connections.Disconnect(DB)
    return &commons.RequestError{StatusCode: http.StatusBadRequest, ErrorOccurredIn: "user_dao 
    CreateUser", Err: err.Error()}
}

This is the biggest issue with the code you've presented here. Worst case scenario, this means that every request that calls CreateUser will open a new connection to the underlying database, get a hard-coded collection (which probably underneath calls getConnectionInfos on mongoDB), just to insert a new document. What would make a lot more sense is to move this function to a type that has access to the collection ready to go:

package repository

type User struct {
    db *mongo.Collection
}

// optionally pass in config, options, etc...
func NewUser(conn *mongo.Database) *User {
    return &User{
        db: conn.Collection("user"),
    }
}

func (u *User) Create(ctx context.Context, user *models.User) error {
    rst, err := u.db.InsertOne(ctx, user)
    if err != nil {
        return err // or some local error/wrapped error. It's up to the handler to return a request error
    }
    user.Id = rst.InsertedID.(primitive.ObjectID)
    return nil
}

With that implemented, we don't need to rely on the connections(DB) call at the end, which I do hope is a connection pool, rather than a function that just spits out new connections every time this gets called.

These are some of the things I'd look in to/reconsider based on a preliminary review. In addition to these things, I do have some other little nit-picks and comments that are more stylistic in nature, but mostly are based on the code review comments from the official golang repo, and are all widely accepted as good practice to follow, to the point where we might as well refer to it as being the ground-rules of idiomatic golang:

  • DB := ...: As a variable name, prefer db.
  • connections.Foo(): implies either a global variable (which is almost always wrong) or a package called connections. If it's a package name, it's not very descriptive. Connections to what? All connections? net/http deals with connections, RPC calls do, too. If you go for the repository approach, DB connections usually are confined to an adaptor, which is probably a better name for this package
  • commons: this is a package name that is strongly advised against, as once again it communicates very little in the way of what can be found here. What's more, having a commons package that is imported everywhere actively encourages bad practices (as exemplified here by the function responsible for storing data returning request errors). Read up on the SOLID principles, most of which are associated with OOP, but they still hold as very sensible concepts to adhere to. Specifically the SRP (Single Responsibility Principle), and apply it to a package: A package should do just one job, and do it well. It should focus solely on doing that one task, have a clear interface through which users can accomplish this task, and that interface should return clear indicators of success/failure. Using commons packages as part of your interface is by definition making the communication between callers more vague/broad/nebulous. Compare the following:

Non-SRP version:

package foo

type Dependency interface {
    Some() error
    Complicated(ctx context.Context, data any, opts ...func()) (any, error)
    Interface() (*SomeType, error)
}

type T struct {
    f Dependency
}

func New(d Dependency) *T {
    return &T{
        f: d,
    }
}

func (t *T) DoSome(ctx context.Context, data *types.Data, opts ...func()) error {
    if len(opts) == 0 {
       if _, err := t.f.Complicated(ctx, data); err != nil {
           return &commons.ErrGenericFault{
               Message: "foo.DoSome failed, no options given",
               Args:    []any{*data},
               Err:     err, // note, this can be a problem 
           }
        }
        return nil
    }
    // let's say passing in opts means we have to check other errors:
    r, err := t.f.Complicated(ctx, data, opts)
    if err != nil {
        // same as above, but append opts to args
    }
    // maybe depending on return value we need to check other errors
    // e.g. the opts specify the call can be performed asynchronously
    // when bulk-inserting or something, at the end we call `t.f.Some` to wait
    // for confirmation everything went well
    if err := t.f.Some(); err != nil {
        return &commons.ErrGenericFault{
            Message: "batch operation failed at some point",
            Args:    []any{*data}, // add opts
            Err:     err,
        }
    }
    return nil
}

Now the caller cannot easily differentiate between faults. They all return the same error type. The only way to really differentiate between any and all errors is by looking at the message itself, which means the caller has to know the internals of the function it is calling, it has to be aware of implementation details in order to accurately determine whether or not the error can/should be handled and how. What's more, I noted in the comment that including the error is risky here, because all errors are pointers to some common.ErrType. What if the Dependency here returns an error from the same commons package? What do I do with the potential multiple layers of custom errors, all packaged up in the same type? How to I extract the information that I really need, and to what extent is the caller served by this information? Does the caller to the above DoSome function need to know about the internal message strings of the foo package as well as all possible error strings associated with the dependencies of foo? That would be atrocious. Compare this to the same time, but with some added variables like this:

var (
    ErrNoOptionsSomeError = errors.New("failed DoSome, no options")
    ErrOptionsSomeError   = errors.New("failed DoSome with options")
)

And instead of the commons errors, you just return these error variables. They're exported, and are part of the contract. This is how this package indicates various specific errors, so it's trivial for the caller to differentiate by checking if the returned error is err == foo.ErrOptionsSomeError, or if you want to include more details: errors.Is(err, foo.ErrOptionsSomeError). To include more information, you'd return an error like so:

if _, err := t.f.Complicated(ctx, data); err != nil {
    return fmt.Errorf("%s: %w", err, ErrNoOptionsSomeError)
}

Where %s prints out the underlying error as a string, and %w wraps the specific error.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for the effort you took to explain the code at this level \$\endgroup\$
    – Nisal Edu
    Sep 19 at 15:30

Your Answer

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

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