getAlby / getAlby/hub

Improve error handling

Open
#88 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
280
Forks
132
Avg merge
5d 18h
Merged PRs (30d)
8

Description

From @rdmitr :

> In my code, I prefer to have well-defined, layer-specific errors on every layer for the common cases, and ad-hoc, fmt.Errorf()-style errors for “something went wrong and we don’t know how to deal with it” cases. Layer-specific errors returned from the lower layer are processed in the higher layer and never bubble up (unless the higher layer does not expect them and does not know how to deal with them). Simple example: if the DB throws an sql.ErrNoRows, the API layer converts it to a layer-specific EntityNotFound, which becomes a proper HTTP 404 in the HTTP controller layer. On the other hand, if the DB spews up something unexpected like a deadlock, I wrap it with fmt.Errorf() to add context and let it bubble up to the controller, where it is logged properly and returned to the client as HTTP 503.

The idea is very simple actually. It boils down to checking errors whenever you receive them and acting accordingly.
I’ll try to give a concrete example, starting with what I see as problematic approach. All the code below was not checked anywhere, so consider it to be Go-ish pseudocode :grin:
The core of the issue is embodied in this snippet we often see in Go:
```
if err := foo(); err != nil {
return err
}
```

To give a more concrete example, here’s how the same idiom could look somewhere in the DB layer. Some function loads a record that matches a key, number:
```
func LoadRecord(number int) (*Record, error) {
// Some record type to read data into
var record Record

err := db.QueryRow("select * from data where number = ?", number).Scan(&record)
if err != nil {
return nil, err
}

return record, nil
}
```
As it happens, on top of the DB layer you have the service layer which calls the DB layer functions to fetch some important data, and then processes it. This function does the same naïve error handling:
```
func DoTheScience(number int) (*ScientificResult, error) {
record, err := mydb.LoadRecord(number)
if err != nil {
return nil, err
}

// process record...

err = mydb.UpdateRecord(record)
if err != nil {
return nil, err
}

// ...
}
```

Finally, there’s an HTTP handler, maybe (or a different kind of a controller):
```
func ScienceHandler(w http.ResponseWriter, r *http.Request) {

// Unmarshal the number from the request...
number := ...

nerdyStuff, err := science.DoTheScience(number)
if err != nil {
// OMG something is broken!
w.WriteHeader(http.StatusInternalServerError)
}

w.WriteHeader(http.StatusOK)
io.WriteAll(nerdyStuff)
}
```

The issue is obvious at this point. The HTTP handler gets an opaque error, err, which it is unable to understand and act accordingly. What exactly did go wrong? Maybe no database records matching number were found? Or there was a database connection failure? In fact, which of the database layer functions failed — LoadRecords() or UpdateAllRecords()? We have no clue and neither does the client of our HTTP service.
Let’s try improving this. First, in the DB layer, we will try to distinguish a missing record from a generic DB failure:
```
// We do not want to carry any additional context,
// so we can get away with a simple global error
// instead of having to define a specific error type.
// This is actually idiomatic in Go, and you can see it
// all over the standard library.
var ErrRecordNotFound = errors.New("record not found")

// ...

func LoadRecord(number int) (*Record, error) {
// Some record type to read data into
var record Record

err := db.QueryRow("select * from data where number = ?", number).Scan(&record)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
// Not an internal failure:
// the requested record does not exist!
return nil, ErrRecordNotFound
}
// This is an internal error, we add some context
// ("failed to fetch record") to make it clear
// what exactly (and where) went wrong:
return nil, fmt.Errorf("failed to fetch record: %w", err)
}

return record, nil
}
```

Good, now the callers of LoadRecord() can understand what exactly is going wrong. In a similar manner, we can improve the service layer:

```
// Like in the DB layer, we define a specific error here.
var ErrNumberIsUnsupported = errors.New("the number is not known to science")

// ...

func DoTheScience(number int) (*ScientificResult, error) {
record, err := mydb.LoadRecord(number)
if err != nil {
if errors.Is(err, mydb.ErrRecordNotFound) {
// The number is not found in the database
// and we cannot process it.
return nil, ErrNumberIsUnsupported
}
return nil, fmt.Errorf("failed to look up number in the database: %w", err)
}

// process record...

err = mydb.UpdateRecord(record)
if err != nil {
// Maybe this operation can never return a DB-layer-specific error,
// so we can treat any error as unexpected
return nil, fmt.Errorf("failed to update the number's record: %w" err)
}

// ...
}
```

Finally, we can use the fine-grained error details to give better feedback to our clients in the HTTP handler:

```
func ScienceHandler(w http.ResponseWriter, r *http.Request) {

// Unmarshal the number from the request...
number := ...

nerdyStuff, err := science.DoTheScience(number)
if err != nil {
if errors.Is(err, science.ErrNumberIsUnsupported) {
w.WriteHeader(http.StatusBadRequest)
io.WriteAll(w, "your number is not known to the science, try another one")

// Return from the function after writing the response.
return
}

// Now something is REALLY broken!
// Usually, we do not leak the internal error details to the client;
// We log err instead:
log.Error("internal science failure: %s", err.String())
w.WriteHeader(http.StatusInternalServerError)
io.WriteAll(w, "science is broken and will be back shortly")
}

w.WriteHeader(http.StatusOK)
io.WriteAll(w, nerdyStuff)
}
```

Sure, implementing layer-specific errors in every layer like above seems tedious. Maybe it is tempting to return mydb.ErrRecordNotFound as is in the service layer, without introducing the service-specific ErrNumberIsUnsupported error. Or maybe even let the database error pass up the call chain to the controller, and have a check like if errors.Is(err, sql.ErrNoRows) { ... } there, right in the HTTP handler. I would recommend against this. Such code quickly becomes messy and unmaintainable.
Finally, you likely have seen it already, but if you haven’t, I really recommend reading the Go docs on error handling in the errors package in the standard library: https://pkg.go.dev/errors :slightly_smiling_face:

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.