Controller-Service-Repository Pattern

[TangoYankee]

Description

We are generally moving toward a controller-service-repository pattern. However, we may have different understandings of where to draw the lines between each of these components. This discussion will give a brief overview on the responsibilities of each component and then go through an example endpoint, detailing where to draw the lines between each component.

Discussion strategy

This top-level comment will hold the latest consensus among the participants. As comments are added, this comment will be updated to reflect the changed consensus

Architecture overview

Controllers

Controllers are responsible for routing traffic. They accept incoming routes and inspect the http methods used on that route. Based on these routes, they match the request to the necessary service. Based on the result of the service function, they also return the appropriate response- to include the appropriate status code and body.

Repositories

Repositories are responsible for interacting with databases. Transactions and queries should be held within the repository. In drizzle, we can also create prepared statements with placeholders to improve performance.

Services

Services handle the "business logic" of the application. They often coordinate requests from controllers with results from repositories. If a repository response needs to be formatted to meet an api-specific structure, it should be transformed in the service

Endpoint

The tax-lots/<bbl>/geojson and tax-lots/<bbl>/zoning-districts endpoints provide an opportunities to explore how these responsibilities

tax-lots/<bbl>/geojson

tax-lots/<bbl>/geojson accepts a get method and 10 character url parameter (the bbl). The controller will listen for these two pieces of information. It will call the appropriate service function with the bbl parameter. The service will check the bbl is valid. If it is invalid, the service will throw an error that will bubble up to the controller. The control will detect this error and return a corresponding 400 error response. If it is valid, the service will then call the repository.

The repository will make a request to the database. If an error occurs during the request, the repository will catch and rethrow an error that will bubble up to the service. The service will also rethrow an error. Finally, the controller will catch this error and return a corresponding 500.

If the repository successfully makes its request, it will return the data as-is (no transformations). If there are no rows in the response, the service will throw an error. The controller will catch the error and return a corresponding 404. If there is a row in the response, the service will structure the raw response to meet the required format and then return it to the controller. The controller will return the data as the body of a 200 response.

The spatial data requires two operations during its processing- a database ST_AsGeoJSON and a JS JSON.parse. The repository will contain the ST_AsGeoJSON. The service will apply the JSON.parse

tax-lots/<bbl>/zoning-districts

tax-lots/<bbl>/zoning-districts accepts a get method and 10 character url parameter (the bbl). The controller will listen for these two pieces of information. It will call the appropriate service function with the bbl parameter. The service will check the bbl is valid. If it is invalid, the service will throw an error that will bubble up to the controller. The control will detect this error and return a corresponding 400 error response. If it is valid, the service will then call the repository.

The service will make up to two calls to the database. The first call is a check of whether the selected tax lot exists. If the repository returns an empty response, the service will throw an error. The controller will catch the error and return a 404. If the repository returns a response with data, the service will then make a subsequent call to the repository to get the zoning district data. The repository will return the zoning district data as is. The service will format the data, such as taking the array and placing it into an object ie) { zoningDistricts: Array<zoningDistrict> }.

If at any point the database throws an error, the repository will throw an error. The Service will catch this error and rethrow it to the controller. The controller will catch it and return a corresponding 500 error.

[TangoYankee]

@TylerMatteo how closely does this follow what you had in mind? (After we get these responsibilities sorted out, we can refactor the existing code. And once we have the code refactored, we can start to implement tests of each component)

[TangoYankee]

I also made a exp/repo branch to show these endpoints in a more concrete way. It implements the tax-lots endpoints using a TaxLotRepo for the Drizzle endpoints. Files of interest are the tax-lot.service.ts and tax-lot.repo.ts.

[TylerMatteo]

Your overall description of the separation of responsibility sounds good to me.

I looked over the changes in your example branch and it looks pretty good with one exception (pun extremely intended) - it looks like you're throwing Nest's built-in exceptions like InternalServerErrorException and NotFoundException in your controllers and services. In my mind, those exceptions are intentionally coupled to HTTP request concepts, so they belong in the controller. I don't necessarily have a problem with where/how you're throwing exceptions in the services and repos, but I think we should throw more generic ones at those levels, which the controllers can catch and map to the appropriate HTTP-specific exception.

[TangoYankee]

I understand your point about separation of concerns for the exceptions. There is a path forward on making a more robust exception-handling system, where the controller has the main responsibility for returning the http response. However, I think there are still open questions on how exactly this system would work. These questions deserve their own discussion.

For now, I'd like to move forward with the repository refactor with the current system of exception handling. Once we have the repositories in place and an idea of what this new exception system should look like, we can refactor the exceptions too.

[TylerMatteo]

I agree that this probably warrants having one approach now to potentially be revisited later. I gave some more thought to what a short term path forward looks like here and wonder what you think of this approach:

What I'm proposing is that we continue to throw the HTTP based exceptions and only those exceptions, but that we do it from controllers, instead of services or repositories. From looking at your example branch, it seems to me that the services and repositories would not need to actually catch exceptions to achieve the functionality we're after, at least in the majority of cases.

For instance, take the code the path for the /tax-lots/:bbl endpoint in your example branch.

The controller calls the service method (with no try/catch)

@Get("/:bbl")
  async findDetailsByBbl(@Param() params: { bbl: string }) {
    return this.taxLotService.findTaxLotByBbl(params.bbl);
  }

The service calls the repository. It doesn't catch but it does throw NotFoundException if the repo gives back undefined because it couldn't find a tax lot with the bbl

async findTaxLotByBbl(bbl: string) {
    if (this.featureFlagConfig.useDrizzle) {
      if (!taxLotBblSchemaD.safeParse({ bbl }).success)
        throw InvalidRequestParameterException;

      const result = await this.taxLotRepo.findTaxLotByBbl(bbl);
      if (result === undefined) throw new NotFoundException();
      return result;
    } else {
      return this.taxLotRepository.findOne(
        { bbl },
        {
          fields: ["bbl", "borough", "landUse", "block", "lot", "address"],
          populate: ["borough", "landUse"],
        },
      );
    }
  }

Finally, the repo wraps the db call in a try catch that catches any error thrown and throws InternalServerErrorException.

async findTaxLotByBbl(bbl: string) {
    try {
      return await this.db.query.taxLot.findFirst({
        columns: {
          boroughId: false,
          landUseId: false,
          wgs84: false,
          liFt: false,
        },
        where: (taxLot, { eq }) => eq(taxLot.bbl, bbl),
        with: {
          borough: true,
          landUse: true,
        },
      });
    } catch {
      throw new InternalServerErrorException("Error retrieving data");
    }
  }

What I'm suggesting is that, instead of only doing try/catches in repos (and sometimes services), we only do them in controllers. We would still be using same HTTP-based exceptions, but calling them in the controller layer which is better from a separation of concerns point of view, and future proofs for scenarios that might come up later where we feel the need to try/catch in repos or services in addition to controllers. Put another way, I think what I'm suggesting is functionally the same as what you have in your example branch. With what I have in mind, the repo method would look something like this:

async findTaxLotByBbl(bbl: string) {
      return await this.db.query.taxLot.findFirst({
        columns: {
          boroughId: false,
          landUseId: false,
          wgs84: false,
          liFt: false,
        },
        where: (taxLot, { eq }) => eq(taxLot.bbl, bbl),
        with: {
          borough: true,
          landUse: true,
        },
      });
  }

The service would look something like

async findTaxLotByBbl(bbl: string) {
    if (this.featureFlagConfig.useDrizzle) {
      if (!taxLotBblSchemaD.safeParse({ bbl }).success)
        throw InvalidRequestParameterException;

      const result = await this.taxLotRepo.findTaxLotByBbl(bbl);
      // Note it no longer throws an exception.
      // we might want to coerce the undefined we get from Drizzle to null in this scenario, but that's a small detail in this context
      return result; 
    } else {
      return this.taxLotRepository.findOne(
        { bbl },
        {
          fields: ["bbl", "borough", "landUse", "block", "lot", "address"],
          populate: ["borough", "landUse"],
        },
      );
    }
  }

And finally, the controller method with the exception throwing looks something like:

@Get("/:bbl")
async findDetailsByBbl(@Param() params: { bbl: string }) {
  try {
    const taxLot = await this.taxLotService.findTaxLotByBbl(params.bbl);
    // I think you might actually have to rethrow this in the catch?
    // In any case, the point remains that you're throwing this from the controller
    if (taxLot === undefined) throw new NotFoundException(); 
  } catch {
    throw new InternalServerErrorException("Error retrieving data");
  }
}

A key thing in common is that both approaches have methods that don't include try/catches. The difference is that the former leaves them out of the controller while the latter leaves them out of the repository. Given that the exceptions we have and are most concerned with now are HTTP-specific, I think it makes sense to do it at the controller level.

Thoughts? This could be something we do as part of the repository factory or as a separate thing to come before or after that work.

[TangoYankee]

Documenting are in-person discussion:

The controller doesn't have enough context on why the error was thrown to confidently attribute a specific type of error to it. Also, in using a try-catch that sits on top of a long chain of logic, we run the risk of throwing a more specific error on a lower level only to have it caught and coerced to a more generic or inaccurate error message at the higher level. There are ways to alleviate this. But, the difficulty is in the details of those alleviation. This brings us back to the original spot- discussing a robust exception handling framework.

[TylerMatteo]

Adding for posterity - I stumbled up on this SO thread that I think summarizes thoughts on what sort of exceptions ought to go at which layer of the architecture: https://softwareengineering.stackexchange.com/questions/393307/where-would-you-handle-exceptions-controller-service-repository. This doesn't necessarily describe the approach we're taking now, just documenting the research.

[TangoYankee]

#104