[Golang] code smell - sql.DB/sqlx

Totally unrelated to the blog…

Introduction

This blog focuses on how things I would point out in a code review, the reasons behind why I point them out, and how I would go about fixing them. The reasoning behind these decisions put a heavy emphasis on making the code hard to misuse for someone new coming into Go and making the code review experience easier for the future developers of the system.

Not using context.Context

Always look for a method that takes context even when you don’t need to cancel or timeout the calls. When a method takes context, it communicates to the user that this method can be canceled and you almost always want an ability to cancel a remote call like SQL queries.

Imagine a scenario where you’re working on an API that gets some information from the database, could be a list of orders placed by a user. The user opens a webpage that calls out to your API, and the query takes 10 seconds to run. The user gets impatient and closes the browser after 2 seconds. Would you want the query to keep going until it either gets timed out by the database connection, or to complete even when no one will be there to read the result? No, is the correct answer. Therefore, ALWAYS choose the …Context version of the methods to use when using the sql and sqlx package. This also applies with any package that makes remote calls like aws-sdk.

Consider these this code

func (s *server) getUsers() ([]user, error) {
    var users []user{}
    _, err := s.db.Select(&users, “SELECT * from users;”)
    if err != nil {
        return []user{}, fmt.Errorf(“failed to get all users: %w”, err)
    }
    return users,  nil
}

To the caller of this function, especially someone new to Go, it is not obvious on how things like timeouts and cancellations are handles.there is way to control how long the query can run, and to force an early exit for the query (i.e. canceling the request). By not allowing context to be passed in by the caller, the code is taking away a critical control knob that the caller may want to use.

A better thing to do is to utilize context.

func (s *server) getUsers(ctx context.Context) ([]user, error) {
    var users []user{}
    _, err := s.db.SelectContext(ctx, &users, “SELECT * from users;”)
    if err != nil {
        return []user{}, fmt.Errorf(“failed to get all users: %w”, err)
    }
    return users,  nil
}

Even if the user can still pass in context.Background, it is much more difficult to misuse since that'll be obvious to the reviewer when context.Background() randomly shows up in the call chain.

It is the difference between these 2 lines showing up in the diff

...
users, err := s.getUsers()
...

versus

...
users, err := s.getUsers(context.Background())
...

That context.Background() sticks out to the reviewer without having to know the implementation of getUsers.

So when reveiwing code interacting with SQL (with sqlx), always flag down every calls that don't take context and switch it to the context version.

---

Closing transaction in a different scope it begins

Transactions can be tricky to deal with. Forgetting to commit/rollback a transaction can cause a lot of problems from the hung transaction holding onto locks. My rule around handling transaction is to ONLY close the transaction where it begins. NEVER rollback/commit a transaction that gets passed in as a function parameter.

Consider the following code

func deleteAddress(tx *sql.Tx, userID string) error {
    const query = `DELETE * FROM address WHERE user_id=:id`
    _, err := tx.ExecContext(ctx, query, userID)
    if err != nil {
        tErr := tx.Rollback()
if tErr != nil {
    // handle error here
}
        return fmt.Errorf("failed to delete address for user id=<%s>: %w", userID, err)
    }
    return nil
}

Might look reasonable at first to rollback a transaction when there's an error; however, this can cause a lot of confusion to the caller.

When this shows up in the diff, I need to trace back to the caller of this method to make sure the caller does not attempt to rollback the transaction again, or is handling the sql.ErrTxDone properly. Instead of this being a simple code review, I now have to look for every usage of this method, and also have to remember to deal with the same issue for any future usages of this method. Moreover, this also trains developers' brains to not close the transaction and lets the lower level function close it for them. An example being

func (s *server) DeleteUserInfo(ctx context.Context, userId string) error {
    tx, err := s.db.BeginTxx(ctx, sql.TxOption{})
    if err != nil {
        return fmt.Errorf(“failed to open a transaction to delete User: %w”, err) 
}
err = deleteUserAddress(ctx, userId) // this rollback on error
if err != nil {
    return fmt.Errorf(“failure in DeleteUser trying to delete user address: %w”, err)
} 

err = deleteUser(ctx, userId) // this rollback on error
if err != nil {
    return fmt.Errorf(“failure in DeleteUser trying to delete user: %w”, err)
} 

err = tx.Commit()
if err != nil {
    return fmt.Errorf(“fail to commit transaction to delete a user: %w”, err)
}
return nil
}

It is difficult to guarantee that every single methods that take a transaction will it back on error for you. Also, what if I don't want the transaction to stop? What if I'd like to continue with the transaction even when it partially fails? This makes it all but impossible for the caller to make that decision for their own.

When reviewing code, anytime I see the beginning of a transaction, I expect a following defer method that rolls it back.

tx, err := s.db.BeginTxx(ctx, sql.TxOption{})
    if err != nil {
        return fmt.Errorf(“failed to open a transaction to delete User: %w”, err) 
}
defer func(){
    tErr := tx.Close()
    if !errors.Is(tErr, sql.ErrTxDone){
        // handle unexpected error here
}
}()

Now it is impossible to leave a transaction hanging since the code will try to roll it back once the method exits. Rolling back a committed transaction simply returns a sql.ErrTxDone error.

After ensuring that every BeginTxx call has a defer that rollsback the transaction, I can safely refactor out all the Rollback() calls sprayed in different places making code review much simpler. It'll also make adding new methods easier since it eliminates a whole class of code review signal developers have to look for - no one EVER calls Rollback() unless you're in a defer block that comes right after a BeginTxx.