fix: send 404 for findById(), if resource is not found

[hacksparrow]

For fixing #1472 and #1469.

Update: Since the causes of #1472 and #1469 are worlds apart. I will open a new PR for addressing #1472.

[raymondfeng]

@hacksparrow Did you miss some files in the PR? I only see changes in tests.

[hacksparrow]

If legacy-juggler-bridge throws the specific error instead of the generic Error,
we won't need to try-catch here.

However, there is a catch: legacy-juggler-bridge needs to import @loopback/rest,
which would create a cyclic dependency.

Maybe HttpErrors should be a package of it's own?

[hacksparrow]

This one's for fixing #1469.

[raymondfeng]

Does juggler throw errors with status or statusCode set for http? If so, we can check that pattern instead of forcing HttpErrors.

[raymondfeng]

BTW, HttpErrors is from its own module. See https://github.com/strongloop/loopback-next/blob/3731d0f1ee18a12b36a6ec3785f295008b50d339/packages/rest/src/index.ts#L36. @loopback/rest just re-export it.

[hacksparrow]

Juggler does not throw, it just returns null. I am exploring the possibilities at Juggler level, because in another case it throws with code and statusCode.

[hacksparrow]

This fixes #1472.

[hacksparrow]

@raymondfeng pushed two implementation changes.

[hacksparrow]

@raymondfeng what's your comment on https://github.com/strongloop/loopback-next/pull/1617/files#r211287088?

[virkt25]

How are the delete tests related to #1469? I understand the demonstration but I'm guessing the fix will live in legacy-juggler?

[virkt25]

found

[hacksparrow]

This is just a quick hack to demonstrate the problem.

😃

[hacksparrow]

We should for a solution which requires no change in controllers.

[hacksparrow]

This PR should land for this to work.

[bajtos]

Cross-posting my comment from loopbackio/loopback-datasource-juggler#1620:

• Juggler as the data-access layer should not be coupled with the specific requirements of transport layers like REST.
• In LoopBack 1.x-3.x, we are able to convert null result to 404 error. I am sure we can do that in LB4 too.

A link to the solution we use in LoopBack 3.x: https://github.com/strongloop/loopback/blob/master/lib/persisted-model.js#L85-L95

In my opinion, the solution to this problem is to modify the generated CRUD Controller method findTodoById to convert null result into an HTTP error.

Using our TodoController as an example:

export class TodoController {
  // ...

  @get('/todos/{id}')
  async findTodoById(
    @param.path.number('id') id: number,
  ): Promise<Todo> {
    const result = await this.todoRepo.findById(id);
    if (result == null) {
      throw new HttpError(
        404, 
        `Todo #${id} was not found.`,
        {code: 'MODEL_NOT_FOUND'}
      );
    }
    return result;
  }
}

To avoid unnecessary repetition of the code handling "null to 404" conversion, we can extract a shared helper function into @loopback/rest package.

import {convertNullToNotFoundError} from '@loopback/rest';

export class TodoController {
  // ...

  @get('/todos/{id}')
  async findTodoById(
    @param.path.number('id') id: number,
  ): Promise<Todo> {
    const result = await this.todoRepo.findById(id);
    convertNullToNotFoundError(result);
    return result;
  }
}

Thoughts?

[hacksparrow]

@virkt25 I have update the PR with only the relevant changes.

[hacksparrow]

@bajtos lovely solution! I will go ahead with it.

[bajtos]

@hacksparrow cool :) Maybe we can find a better name than convertNullToNotFoundError, perhaps throwNotFoundErrorWhenNull?

[virkt25]

Should this be configurable at the @model() level similar to strict and strictDelete?

[hacksparrow]

Here is what is happening in the issue this PR is targeted at:

legacy-juggler-bridge throws a generic error when findById() does not find the model. Since this generic error is not an HttpError, it is being interpreted as a 500 error and is sent in the response.

Making legacy-juggler-bridge throw a HttpErrors.NotFound fixes the problem. However, we want to keep repositories transport-agnostic and don't want the solution implemented at the repository level.

We could make Juggler throw HttpErrors.NotFound, but then we want to keep it also transport-agnostic.

This PR implements the solution at the controller level (to initiate the discussion). However, requiring to add the http-errors module, and try-catching in controller functions don't look elegant. I haven't confirmed yet, but all operations involving *byId() might be affected and we have to try-catch any controllers calling them. This will result in a messy-looking controller file.

So the ideal solution should be at the framework-level but unexposed or barely-exposed to the user.

Miroslav and I have been exploring various options but have not found a good solution yet. It would be great to hear from you all.

There are the possible starting points:

1. legacy-juggler-bridge throws a generic error => We have the problem I have described above. This is the current state.
2. legacy-juggler-bridge throws a HttpErrors.NotFound error => Repository is not transport-agnostic anymore.
3. legacy-juggler-bridge returns null => This is basically resolving a promise with null. A resolved promise is expected to have something, not null. This unexpected behavior can have very undesirable consequences. Besides, this will require change at the EntityCrudRepository level.

---

With reference to legacy-juggler-bridge:

Why is the return type of findOne() Promise<T | null>, but the return type of findById() Promise<T>? If findOne() fails to find something, it resolves the promise with a null; if findById() fails to find something to rejects the promise. Isn't this inconsistent behavior? Should it be a reject it in both the cases?

[virkt25]

What does the error from juggler look like?

I don’t think we can just convert all errors from Juggler to a 404 because what if the juggler didn’t find the model because of a database connection issue?

My guess is that the reason findOne returns null is since it takes a filter for a search and returns the first item matching the criteria and nothing matching isn’t necessarily a NotFoundError but maybe it is?

I think as pointed out, we should aim for consistency and the findByID method is inconsistent and should return null if no item exists (now we know there wasn’t a database error). In the controller we can introduce a helper function to return a 404 for *ByID methods if the result is undefined — which can live in the rest package.

return returnModel(await this.repo.findByID(id))

returnModel(model) {
  if (!model) return HttpErrors.NotFound;
  retrun model;
}

Let’s see what others think before proceeding.

[hacksparrow]

What does the error from juggler look like?

If the model is not found, it returns/resolves null; does not throw or reject promise for not-found.

[hacksparrow]

In the controller we can introduce a helper function to return a 404 for *ByID methods if the result is undefined — which can live in the rest package.

This is exactly what we were trying for. But this requires change in EntityCrudRepository:

findById(id: ID, filter?: Filter, options?: Options): Promise<T>;

should become

findById(id: ID, filter?: Filter, options?: Options): Promise<T | null>;

To me, a model-querying promise resolving two different types seem illogical; and especially if it resolves null. You either find by resolving with a model, or don't find by rejecting the promise.

[hacksparrow]

@raymondfeng any suggestions?

[b-admike]

@hacksparrow After reading through this thread, I think the solutions suggested by @bajtos at #1617 (comment) and @virkt25 at #1617 (comment) are sensible. From the 3 starting points you listed, I don't think 2) and 3) are good approaches for the reasons you mentioned.

[raymondfeng]

I'm leaning toward findById(...): Promise<T | null> for the juggler bridge and doing the 404 mapping inside the controller.

[bajtos]

Another option to consider:

• Modify legacy-juggler-bridge to set err.code = 'MODEL_NOT_FOUND'. We should be setting a machine-readable code property on all errors we are throwing, the code MODEL_NOT_FOUND is already produced by LB 3.x - see lib/persisted-model.js. The place to change in LB4: https://github.com/strongloop/loopback-next/blob/0364b59409ce3691babf5f17b5cd444b4e7f72aa/packages/repository/src/repositories/legacy-juggler-bridge.ts#L213-L215

• Modify the reject action to detect the new code and set err.statusCode = 404 in such case. This modification should be probably done in the default implementation: https://github.com/strongloop/loopback-next/blob/0364b59409ce3691babf5f17b5cd444b4e7f72aa/packages/rest/src/providers/reject.provider.ts#L24-L26

  A mock-up to illustrate my point:

  const codeToStatusCodeMapping = {
    MODEL_NOT_FOUND: 404,
  };
  
  export class RejectProvider implements Provider<Reject> {
    // ...
    action({request, response}: HandlerContext, error: Error) {
      const err = <HttpError>error;
  
      if (!err.status && !error.statusCode && err.code) {
        const proposedStatus = codeToStatusCodeMapping[err.code];
        if (proposedStatus) {
          err.statusCode = proposedStatus;
        }
      }
  
      const statusCode = err.statusCode || err.status || 500;
      // etc.
  }

  Later, we should find a way how to allow applications and extensions to contribute additional entries to codeToStatusCodeMapping table.

• Other transports like gRPC and GraphQL can implement their own algorithm for dealing with this special class of errors in their reject action. (Note that both the Sequence and most if not all sequence actions are transport specific.)

IMO, the proposed API findById(...): Promise<T | null> is not ergonomic, as it requires everybody to handle the "not found" case, even in code where we are fairly certain the instance will be found (e.g. test cases).

[bajtos]

I went ahead and implemented my proposal in #1658.

[hacksparrow]

Closing in favor of #1658.