Result-returning version of `send_file` or alternate error handling mechanism? #2

Open

Open opened by

Hi, found this library and it seems to be just about right for my use case! I was having trouble figuring out a clean way to do a certain kind of pattern though. I'm also pretty new at Gleam so maybe I'm just going about it the wrong way, but I thought I should ask here in case I'm missing something.

In my request handler, I'd like to add some simple logging for all requests, including some error details if they occur. e.g. something like this:

case path {
  "/" | "/index.html" -> {
    generate_index_html(package:)
    |> element.to_document_string
    |> smol.send_html
    // would like to log a 200 OK
  }
  "/" <> rest -> {
    let path = "./" <> rest
    smol.send_file(path, offset: 0, limit: None, or: fn(err) {
      smol.send_status(404)
      // would like to log `err`
    })
    // would like to log a 200 OK, if send_file succeeded
  }
  _ -> {
    smol.send_string("missing/invalid path")
    |> smol.with_status(400)
    // would like to log "missing/invalid path"
  }
}

// ideally, maybe just have a single log() call here or something like that

I'm having trouble figuring out a clean way to write this so that I could do logging in a consistent way. My instinct was to write it in such a way that each branch returns a #(Promise(Response), Result(Nil, _)), but I don't see an easy way to do that with send_file's signature of or fallback: fn(FileError) -> Promise(Response).

What I've got so far is something like this, where every branch calls its own logger, but it feels pretty hacky:

  smol.send_file(path, offset: 0, limit: None, or: fn(err) {
    smol.send_string(string.inspect(err))
    |> smol.with_status(404)
    |> promise.tap(log_response(path, _, Error(err)))
  })
  |> promise.tap(fn(response) {
    case response {
      Response(status: 200, ..) -> log_response(path, response, Ok(Nil))
      _ -> Nil
    }
  })

Is there a better way I can restructure my code for this kind of scenario? Or if not, would it make sense to expose something like a fn send_file_2(...) -> Result(...) to make this kind of usage more ergonomic?

Thanks!

becca.monster 2mo ago

Hi!

I think you're conflating 2 things here. It's common to do some generic request/response logging, typically by writing a middleware function to put on the top of your middleware stack:

fn log(request: Request, next: fn() -> Promise(Response)) -> Promise(Response) {
  use response <- promise.tap(next())
  // ... log request / response somehow ...
}

fn handle(request: Request) -> Promise(Response) {
  use <- log(request)
  use <- smol.rescue(fn(_) { smol.send_status(500) })
  use request <- smol.csrf_known_header_protection(request)
  use request <- smol.handle_head(request)
  // ...
  case request.method, request.path_segments(request) {
    // ...
    http.Get, [] -> todo as "serve index"
  }
}

This is logging in the http servers domain, so it only deals with http requests, ignoring any business logic you might have. It's common to apply heuristics here to, e.g. you might sample successful requests, flag unusually slow responses, or omit routine 404s from the logs. For a file server, those 404s often come from bots probing paths like /wp-admin.php, so you may want to filter them out anyways. The requested path is often enough to identify the missing resource too.

The second is handling and logging of your business/domain outcomes. The general advice from wisp applies here too: Instead of modelling things as Result(Response, Response) or similar, try to think of your request handler as 3 distinct phases:

fn handle_create_user(request: Request) -> Promise(Response) {
  // 1. parse the request into some domain object
  use new_user <- smol.read_json(request, user.decoder(), up_to: 1024*1024)

  // 2. handle the request within our domain
  use result <- promise.await(user.create(new_user))
  
  // 3. serialise the domain outcome into a response
  case result {
    Ok(user) -> smol.send_json(user.to_json(user))
    Error(InvalidEmailAddress) -> smol.send_json(...) |> smol.with_status(400)
    Error(UsernameAlreadyExists) -> smol.send_json(...) |> smol.with_status(400)
    Error(PasswordTooShort) -> smol.send_json(...) |> smol.with_status(400)
    // ...
  }
}

Here, logging the outcome of your domain use case should happen in step 2 as it relates to your application, not doing http server work. I think this is where you're making life a bit harder on yourself, by using this #(Promise(Response), Result(Nil, Error)) tuple instead of more clearly separating those concerns.

send_file now is causing friction because it surfaces something the other functions hide a bit better: Many of smols functions can fail, and return an error response (or have to just abort) instead of calling your next handler. This is usually no problem and is hardly ever noticed, because those too are failures in the "domain" of doing http server stuff, and doesn't really concern your applications business logic. However, send_file lets you handle some of these errors, to allow you to add different kinds of fallbacks (for example, if the request fails with IsDir, try path <> "/index.html" next).

You decided then that these kinds of errors are part of your applications domain, while smol thinks they are part of the http servers domain. You're carrying application-specific information alongside the HTTP response, while smol's helpers are designed to return just the response. I think that's a reasonable boundary for smol to keep. You're right that send_file doesn't currently expose lower-level primitives, but for this particular case, I think logging the file error inside the fallback is a reasonable tradeoff for this one function, even though it means a separate logging call.

Anyways I'd like you to try structuring things like that first to see how it feels! There is technically already this more low-level readable_stream API right now that i deliberately don't expose, so unfortunately I'd like to pass on that until we've found enough warts to justify a version 3 of smol.

I'd keep the issue open for now, thank you though!

ian-h-chamberlain.com 2mo ago

Hey, thanks for the quick + detailed response! Your explanation of separating domain logic + error handling from the HTTP-level error handling makes sense, and after trying it out with just a separate log message, I don't hate the outcome. My goal for these logs was to identify why certain requests were failing, e.g. correlate the path on disk with the request path. It seems like the best way to do that is probably via x-request-id or something similar.

Really appreciate the middleware example, that's exactly what I was looking for! Looking at my code with fresh eyes and your suggestions, I think this works. Being able to log each request uniformly feels way cleaner, and

I think logging the file error inside the fallback is a reasonable tradeoff for this one function, even though it means a separate logging call.

agreed! I think you've answered my question, so feel free to close this if you want.