Add onError callback to all airtable http methods

[jasoncavanaugh]

In a previous PR, a change was made to abstract http requests into a bunch of custom methods (i.e. airtableGET, airtablePATCH, etc). However, if an error occurs in any of these requests, we are not handling the error correctly. We need to add an onError callback to these methods. For basically every method, it will look something like onError: () => res.status(INTERNAL_SERVER_ERROR)

[mtctxd]

As I understand you want something like in code below. Why it should be hook? Dont think this is good idea. Error should not be handled in this utility functions but inside router. In that case would be nice to make some kind utility method that will send some standart error, or even propagate this error outside of router (but im not sure about both of this cases).

async function airtablePATCH<T>({
  url,
  body,
onError,
}: {
  url: string;
  body: object;
  onError: () => void
}) {
  try {
    const res = await airtableFetch<T>({ url: url, method: "PATCH", body: body });
    return res
  } catch {
    onError()
  }
}

[jasoncavanaugh]

The main problem is that we need to have res.status(<ERROR_STATUS_CODE_HERE>). This is because this line in errorMIddleware.ts is causing us problems

  const statusCode = res.statusCode || INTERNAL_SERVER_ERROR;

because res.statusCode is set to 200 by default. This means that the response to the frontend is 200, which means that the UI doesn't error out correctly.

We can also take care of the error in each endpoint, but that would mean something like this afaict.

    let futureEventsAirtableResp = null;
    try {
      futureEventsAirtableResp = await airtableGET<Event>({ url: url });
    } catch (e: unknown) {
      res.status(500);
      throw e;
    }

It seems a bit cumbersome to have a try catch in both the endpoint and the utility function, but I can also see how the callback might seem like an odd pattern.

[mtctxd]

In my personal project I was trying to do something like this for fixing this. Was trying to make usage of decorators, and it did work nicely. But yea, you had to throw error inside of your utility functions anyway, also you should create your own errors for each case. But anyway you are get rid of wrapping everything inside your controller every time, and still have abbility to handle it inside of controller if you need. But I dont know if it is nice approach, did work for me but idk.

// decorator itself
export const WithErrorCatching =
  () => (target: any, propertyKey: string, descriptor: PropertyDescriptor) => {
    const originalMethod = descriptor.value

    descriptor.value = async function (...args: any[]) {
      try {
        const result = await originalMethod.apply(this, args)
        return result
      } catch (error) {
        const next = args[args.length - 1]
        if (typeof next === "function") {
          next(error)
        }
      }
    }

    return descriptor
  }

// usage inside controller
  @WithErrorCatching()
  async createUser(req: Request, res: Response, _next: NextFunction) {
    const userDTO = await zParse(userCreateDTO, req.body)
    const user = await this.usersService.create(userDTO)

    return res.send(user)
  }

 //  middleware
 import { NextFunction, Request, Response } from "express"
import { QueryFailedError } from "typeorm"
import { ZodError } from "zod"
import { HttpStatus } from "../types/HttpStatus"

export const errorHandler = (
  err: unknown,
  req: Request,
  res: Response,
  next: NextFunction
) => {
  if (err instanceof ZodError) {
    return res.status(HttpStatus.BAD_REQUEST).send(err)
  }

  if (err instanceof QueryFailedError) {
    return res.status(HttpStatus.BAD_REQUEST).send(err)
  }

  return res.sendStatus(HttpStatus.INTERNAL_SERVER_ERROR)
}

[jasoncavanaugh]

Well we currently don't use classes at all in the codebase, so a decorator pattern would probably not be feasible. Tbh, this code looks like a step up in complexity and abstraction. I'm not sure what the advantage would be to adopting something like this instead of going with a simpler solution. But curious to hear if any other people have thoughts @mattsahn @6mp

[mattsahn]

@jasoncavanaugh / @mtctxd - I'm a fan of keeping it simple for now. Just add some commenting explaining the approach.

[6mp]

We could utilize a result type of sorts so if there is an error it has to be handled correctly, an example of this would be like so

export type AirtableResponse<T> =
  | { kind: "success"; records: Record<T>[]; error?: never }
  | { kind: "error"; error: string; records?: never };

And using this would look something like this:

async function airtableFetch<T>({
  url,
  method,
  body,
}: {
  url: string;
  method: HttpVerb;
  body: object;
}): Promise<AirtableResponse<T>> {
  let resp = null;
  if (method === "GET") {
    resp = await fetch(url, {
      method: method,
      headers: { Authorization: `Bearer ${process.env.AIRTABLE_API_KEY}` },
    });
  } else {
    resp = await fetch(url, {
      method: method,
      headers: {
        Authorization: `Bearer ${process.env.AIRTABLE_API_KEY}`,
        "Content-Type": "application/json",
      },
      body: JSON.stringify(body),
    });
  }
  if (!resp.ok) {
    const data = await resp.json();
    logger.error("Airtable response: ", data);
    return { kind: "error", error: data };
    //throw new Error(AIRTABLE_ERROR_MESSAGE);
  }
  return { kind: "data", records: await resp.json() };
}

Then when using the function it is not possible to access the actual records without checking the error due to the type. Trying to use it without checking the presence of error or success will result in a type error.

router.route("/api/messaging/last-texts-sent").get(
  protect,
  asyncHandler(async (req: Request, res: Response) => {
    // get messages sent in last 7 days
    const url =
      `${AIRTABLE_URL_BASE}/Text Automation History` +
      `?filterByFormula=DATETIME_DIFF(NOW(), {Date}, 'days') <= 7`;

    const data = await airtableGET<TextAutomation>({ url });

    if (data.kind === "error") {
      res.status(500).json(data.error);
      return;
    }

    const fields = data.records.map((record) => record.fields);

    // only sends fields of each one
    res.status(200).json(fields);
  })
);

Doing this would require alot of refactoring throughout the server codebase though to handle all these errors

[jasoncavanaugh]

If we did things this way, would we be able to just remove the error middleware entirely? Or at least most of the errors would not be handled by the middleware? Tbh, I don't love the current control flow where we have exceptions being thrown and then taken care of in the middleware. I think explicit error handling would be a lot nicer, but as you mentioned, it would require some manual work in all of the endpoints. But if you would like to change the code to use this approach, I would be in favor of it.

[mtctxd]

If we did things this way, would we be able to just remove the error middleware entirely? Or at least most of the errors would not be handled by the middleware? Tbh, I don't love the current control flow where we have exceptions being thrown and then taken care of in the middleware. I think explicit error handling would be a lot nicer, but as you mentioned, it would require some manual work in all of the endpoints. But if you would like to change the code to use this approach, I would be in favor of it.

Don't think removing error middleware is good idea. Error could occure in any part of the code usualy, not only in controllers.

UPD: Oh. What Im talking? nwm :D

[6mp]

@jasoncavanaugh Yeah thats correct, with this method there would be no need for the error middleware in regards to airtable fetches. As @mtctxd mentioned there still is a risk of errors coming from other code but I prefer errors are handled where they arise from which this allows for.

I have implemented this change on #113 for someone to look at.