fix: return 404 when a model was not found

[bajtos]

This is an alternative solution to #1617 that fixes #1469.

• test: fix incorrect usage of rejectedWith assertion. I discovered this problem while fixing tests added as part of this pull request.
• feat(repository): change Model.modelName to a computed property. This ensures modelName is always available and match model definition.
• feat(repository): implement EntityNotFoundError. This error builds a descriptive error message and sets error.code to ENTITY_NOT_FOUND string.
• refactor: rework "findById" to leverage EntityNotFoundError. As a result, "model not found" scenario can be detected by checking whether the error code is ENTITY_NOT_FOUND.
• fix(rest): return 404 when a model was not found. Here we modify the reject action to convert ENTITY_NOT_FOUND error code to response status code 404.
• docs: add Error-handling.md, describe error codes
• feat(rest): add error codes for REST validation errors
  VALIDATION_FAILED, INVALID_PARAMETER_VALUE, MISSING_REQUIRED_PARAMETER

Out of scope of this pull request:

• Update updateById,deleteById to throw EntityNotFound error for consistency - see feat(repository): rework *ById methods to throw if id not found #1723
• Add a factory method allowing people to obtain a modified Repo instance that returns undefined if not found, as described in fix: return 404 when a model was not found #1658 (comment) - see [WIP] feat(repository): add UncheckedCrudRepository #1735

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[hacksparrow]

Maybe declare the type Error & {code?: string} somewhere? We will be using it in other places too.

[bajtos]

Good point 👍

[hacksparrow]

This is our custom error code name, right? We'll have to document this and other possible error types. Eg:

1. Database serve not reachable
2. Authentication failure
3. Authorization failure
4. Runtime errors

[bajtos]

Yes, this is our custom error code. We are using the same code property as Node.js core does, as a result we "inherit" all error codes from Node.js core too - see https://nodejs.org/api/errors.html#errors_node_js_error_codes and https://nodejs.org/api/errors.html#errors_common_system_errors

Database serve not reachable

I think this will typically manifests as ECONNREFUSED, ECONNRESET or ETIMEDOUT.

Authentication failure
Authorization failure
Runtime errors

Agreed. See existing auth error codes in LB 3.x - e.g. common/models/user.js

[hacksparrow]

Should we declare the HttpError & {code?: string} type somewhere?

[hacksparrow]

I am for this approach. It keeps the repository transport-agnostic and gets the job done without any changes in controller code.

It introduces a new Error type with code property, which can be used by users to customize how they handle these types of errors, if they decide to. We will have to document the implemented code value (MODEL_NOT_FOUND), and maybe also other possible values.

[raymondfeng]

While I support the idea to have error mapping capability, I'm not very keen to have findById throws an error if the id does not match any records for a few reasons:

1. Other *ById methods do not throw, such as deleteById and updateById.
2. There should be options for consumers of findById API to handle the not found situation, such as:

• Return null or undefined
• Throw NotFound
• Use a default value

See https://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/repository/CrudRepository.html#findById-ID-

[virkt25]

Other *ById methods do not throw, such as deleteById and updateById.

I would hope / think that the intention would be to keep all the other methods consistent and this is just to demonstrate the idea.

There should be options for consumers of findById API to handle the not found situation, such as:

I'm confused ... how does this approach not allow the consumers of findById API to handle the not found situation? Wouldn't a consumer of this API now be able to try / catch this error and know the issue vs. just getting a null and dealing with it?

EDIT: Just saw the spring.io link. I guess an empty return also makes sense. Looking back at the original issue #1469 -- The original issue was to deal with a "bad" / invalid id which has been fixed. For the other response codes such as Not Found I guess we should decide on an approach and document it so users know what to expect as I don't think there's been a complaint about wrong codes / incorrect behaviour with the current way juggler deals with *ById methods?

If the approach is to return a 404 I like what this PR implements and an alternative option can be for to introduce a strictFind flag ... and we control the default value to be either true or false? If enabled, we throw the 404, if disabled we return an empty object (or null)? --- And ofc this would have to be consistent for all related methods.

[raymondfeng]

I would hope / think that the intention would be to keep all the other methods consistent and this is just to demonstrate the idea.

updateById and deleteId already uses a boolean value to indicate if the id exists without throwing NotFoundError. I would prefer that findById returns null if the id does not exist.

I'm confused ... how does this approach not allow the consumers of findById API to handle the not found situation? Wouldn't a consumer of this API now be able to try / catch this error and know the issue vs. just getting a null and dealing with it?

I don't see a lot of JS developers use try-catch for NotFoundError as testing the error type is not very easy in JS. The null check is much simpler:

const customer = await findById('c01');
if (customer == null) {
  // Not found
}

try { 
  const customer = await findById('c01');
} catch(e) {
  if (e instanceOf NotFoundError) {
    // Not reliable
  }
  if (e.code === 'NotFound') {
    // ...
  }
}

[joeytwiddle]

The mongoose library returns null if findById did not find the document.

As a result, when we used mongoose, we had to check the result of almost every findById call, and throw a suitable error if no document was returned. (Otherwise we would just accept that sometimes we might get an error from the processing code: Cannot read property 'foo' of null)

After this experience, I feel that having a 404 be the default behaviour would be quite convenient, reducing boilerplate and uncertainty in the code. (Boilerplate: checking and throwing, uncertainty: forgetting to check and proceeding with a null.)

But I can see the benefits of Raymond's suggestion to optionally return null or a default value. In some code, especially when using findOne(), we might expect the document to sometimes be missing. (I assume we are aiming for parity between findById() and findOne() also...)

However, a function that can sometimes return null might complicate the return type. Will TypeScript start warning us to check for null results even when the option is set to throw?

If so, would it be worth having a separate function findByIdMaybe() for that situation? This would make the types more accurate, so improve type checking, and it might also be easier to write and review than an option argument. Disadvantage: API bloat.

[bajtos]

Thank you @joeytwiddle for chiming in! It's always great to hear feedback from users 👍

Other *ById methods do not throw, such as deleteById and updateById.

We have recently landed #1621 which changed the default behavior of deleteById to throw 404 error when the target model was not found. See also #1472 for context.

I think we need to rethink "not found" handling in all these three methods and implement a consistent solution.

There should be options for consumers of findById API to handle the not found situation, such as:

• Return null or undefined
• Throw NotFound
• Use a default value
  See https://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/repository/CrudRepository.html#findById-ID-

I fully agree we should provide API that makes it easy for consumers to handle "not found" situation. At the same time, I'd like our API to make it easy for consumers to not have to deal with "not found" situation.

In the past, I had success with providing two methods: get and find. In this case, getById would throw when the model was not found and findById returns a special value indicating unknown model id (e.g. null).

The link to Spring framework is a great discussion starter. Notice that Spring's findById does not return null to signal "not found" case (which would be a perfectly valid API in Java), but uses Optional<T> (also known as Maybe<T> in other languages). This new type has few benefits:

• It forces developers to think about the case when no value was returned. This is enforced by the compiler, because you cannot assign Optional<T> to a variable/argument expecting T.
• Provides a convenient way of detecting "not found" case.
• Provides a value getter that throws an error when no value was set (to avoid "NullReferenceException").
• Can integrate with other language constructs via monadic behavior, e.g. one can define + operator that can add two Optional<number> values in such way that "not found" is treated as "NaN" type, etc.

An example to illustrate the point:

const result = repo.findById(123);

// We are pretty sure the id exist
// Let the framework throw an error if not found
// (the check is implemented in "value" getter)
console.log(result.value);

// We don't know if the id exists
// Handle "not found" case ourselves
if (result.hasValue) {
  console.log(result.value);
} else {
  console.log('not found');
}

Optional<T> is a great solution in typed languages where the compiler can ensure that Optional<T> is not accidentally passed to functions expecting T. I am not sure how much useful it is in JavaScript though, because there is no compiler to tell the developer that they forget to handle "not found" case. (We still do want to support LB4 apps written in pure JavaScript, right?)

Consider the following controller method:

async findTodo(id) {
  return await this.repo.findById(id);
}

The code looks fine, only an experienced LB4 user with a keen eye may notice that this method is going to return model data wrapped in value property, e.g. {value: {id: 1, name: 'Clean my room'}}, instead of the correct payload {id: 1, name: 'Clean my room'}.

I don't see a lot of JS developers use try-catch for NotFoundError as testing the error type is not very easy in JS. The null check is much simpler.

I agree that using try-catch to specifically handle "not found" error can be cumbersome.

I disagree that returning null makes code simpler. In TypeScript with strictNullChecks enabled, a findById method returning Promise<T | null> forces every caller to deal with the "not found" (null) case, even if the caller is pretty sure the model exists (typically in unit tests). We are effectively trading ease of use in code that wants to handle "not found" case for ease of use in code that wants to treat "not found" case as any other exceptional situation (e.g. a database connection problem).

However, a function that can sometimes return null might complicate the return type. Will TypeScript start warning us to check for null results even when the option is set to throw?

This can be handled by function overloads, we already do something similar for context.get, see context.ts.

The idea is to tell TypeScript that the same function returns different types depending on the input arguments.

A mock-up to illustrate my point, it may not work exactly this way.

findById(id: ID): Promise<T>;
findById(id: ID, {optional: false}): Promise<T>;
findById(id: ID, {optional: true}): Promise<T | null>;

findById(id: ID, options: {optional: boolean}): Promise<T | null> {
  const result: T | null = // get data via juggler
  if (result) {
    return result;
  }

  if (options && options.optional) {
      return null;
  }
  
  throw /* not found error */
}

[hacksparrow]

In the the callback pattern (err, result), it is probably the most common practice to pass errors in err and return the results of a query in result. If nothing was found result would likely be undefined or null, depending on the implementation of the querying function. In this pattern conveying a 404 via error would be confusing, even if possible.

Since we are moving on to async-await, rejecting via any means (throwing or calling reject()) should be the convention for conveying the non-finding of a resource. The rejection message can have more details about why it was not found - the resource doesn't actually exist, network failed, not authorized etc.

Ideally, resolution should always return a resource of a single expected type.

[raymondfeng]

For references:

TypeORM (https://github.com/typeorm/typeorm/blob/master/src/repository/Repository.ts#L268)

• findOne returns Promise<Entity|undefined>
• findOneOrFail returns Promise<Entity> and throws if not found

Sails.js (https://sailsjs.com/documentation/reference/waterline-orm/models/find-one)

• findOne returns undefined if not found

BookShelf (http://bookshelfjs.org/#Model-instance-fetch)

• Option (require=false) Boolean: Reject the returned response with a NotFoundError if results are empty. Default to false.

[marioestradarosa]

Why not delegate this error type transformation to the controller and leave the current undefined if not found to the artifacts .

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    return await this.daisyRepository.findById(id);
  }

like such:

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    let result =  await this.daisyRepository.find(filter);
    if (result === undefined || result === null) { 
             throw new HttpErrors. NotFound(`id ${id} was not found`);
       } 
    return result;
  }

At the end, 404 status is really something expected from a client application such as the following. The problem is that if we don't receive an http status error, then the logic should change in this snippet.

      this.dummyService.findDaisy(arg).toPromise().then(data => {
         this.totalRecords = data;
        // this.paginator.length = data;
      }).catch(reason => { // do something }) ;

[raymondfeng]

Why not delegate this error type transformation to the controller and leave the current undefined if not found to the artifacts .

That's how I would draw the line too. The repository API just returns null (or a better object) if not found and we leave the mapping to controllers, such as null --> 404.

[bajtos]

Why not delegate this error type transformation to the controller and leave the current undefined if not found to the artifacts .

That's how I would draw the line too. The repository API just returns null (or a better object) if not found and we leave the mapping to controllers, such as null --> 404.

While a null-returning API is practical for users that are expecting the model instance to not exist (e.g. a Controller method), it's very impractical for users that are certain the instance should exist (e.g. unit/integration tests). See my earlier comment:

I fully agree we should provide API that makes it easy for consumers to handle "not found" situation. At the same time, I'd like our API to make it easy for consumers to not have to deal with "not found" situation.

In the light of the discussion above, I am proposing the following plan going forward:

1. Add a new method getById that throws an error when the model was not found.
2. Modify findById to return null instead of throwing an error. Modify tests expecting the model instance exists to use getById instead of findById.
3. Add a helper for constructing "model instance id XYZ was not found" error
4. Modify controller template & examples to handle "not found" error by leveraging the two mechanisms described above

[hacksparrow]

@marioestradarosa we considered that too. The problem with that we will have repetitive code in controllers - all *ById methods have to implement 404 handlers of their own. Even if we create a common helper function for that purpose, we will end up with an odd-looking experience in the controller methods.

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    return await this.daisyRepository.findById(id);
  }

vs

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    return callThe404Helper(await this.daisyRepository.findById(id));
  }

That's not intuitive.

Also, it requires a change in EntityCrudRepository :

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

should become

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

[hacksparrow]

Modify controller template & examples to handle "not found" error by leveraging the two mechanisms described above

So, if we use getById(), our code will look like this:

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    return await this.daisyRepository.findById(id);
  }

and if we use findById(), it looks something like this:

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    let result =  await this.daisyRepository.find(filter);
    if (result === undefined || result === null) { 
             throw new HttpErrors. NotFound(`id ${id} was not found`);
       } 
    return result;
  }

or

@get('/daisies/{id}')
  async findById(@param.path.number('id') id: number): Promise<Daisy> {
    return callThe404Helper(await this.daisyRepository.findById(id));
  }

Is my understanding correct?

[bajtos]

@hacksparrow here is an example from the code I am working on:

export class TodoController {
  // ...

  @get('/todos/{id}')
  async findTodoById(
    @param.path.number('id') id: number,
    @param.query.boolean('items') items?: boolean,
  ): Promise<Todo> {
    const maybeResult = await this.todoRepo.findById(id);
    if (maybeResult) return maybeResult;
    throw new EntityNotFoundError(Todo, id, {statusCode: 404});
  }
}

[bajtos]

I reworked my pull request per my previous comment. I am keeping the original code in the git history for posterity, I'll rebase the first two commits away before landing this pull request.

@raymondfeng @hacksparrow @marioestradarosa @joeytwiddle please review

[marioestradarosa]

@hacksparrow You analysis is good and valid, but let me share with you that we are implementing some batch processes that will run periodically from unix cron.

We are using the context and the Juggler from LB4 as they are independent packages, for this scenario developers are used to check the result for null or undefined and it is not related to controllers.

The code in the controller as @bajtos is proposing below makes sense for keeping both worlds happy. It is repetitive ?, yes but it is generated and developers can changed it if they want to, however, I don't see any reason so far why they should remove this throw new EntityNotFoundError as it is related to web client apps in need of it.

Wrapping it up inside the internal libraries doesn't allow this flexibility mentioned above.

if (maybeResult) return maybeResult;
    throw new EntityNotFoundError(Todo, id, {statusCode: 404});
  }

[bajtos]

If we decide to add findByIdOrThrow, we should also add updateByIdOrThrow and deleteByIdOrThrow.

I agree that we should add those other methods too. Maybe deleteByIdOrThrow is not necessary, because when the model was not found, then it means it no longer exists and the intention to delete it has been fulfilled.

The number of methods will grow quickly with these variants.

That is true. What other options are there? The discussion above mentions:

• Optional return type. It does not map well to JavaScript and I also don't see how it would apply to updateById and deleteById.
• A new argument options, with a property like default or throwIfNotFound allowing the developer to pick the error handling mode they prefer. This has been rejected: No magic via options.throwIfNotFound or options.default. For example, options.default is often not very useful if it only accepts a constant value.

As as I see it, we have to make a trade-off here and accept that neither solution is going to be perfect.

1. A single findById method, always returning undefined when the model was not found. Downside: test code has to explicitly throw an error or rely on not very helpful TypeError.
2. Two methods findById and findByIdOrThrow. The downside: the number of repository methods can grow too much.
3. A single method findById with an options object controlling how "not found" is handled. The downside: @raymondfeng considers this as magic that should be avoided.

As I wrote the list above, I actually like the last variant based on options object most.

It is following the pattern we are already using in Context, where one can use ctx.get(KEY) or ctx.get(KEY, {optional: true}), where the first one throws on not found and the second returns undefined when not found.

https://github.com/strongloop/loopback-next/blob/13e8f99402c9b6f43a61a30acd2e7237e20d468b/packages/context/src/context.ts#L258

https://github.com/strongloop/loopback-next/blob/13e8f99402c9b6f43a61a30acd2e7237e20d468b/packages/context/src/context.ts#L281-L284

It is also easy to apply to other methods like updateById and can be easily extended in the future by adding more options properties (as opposed to more explicitly named methods like findByIdOrThrow).

[hacksparrow]

Or can we have a property in the app context, which configures how *ById behaves?

[bajtos]

In a chat with @raymondfeng @hacksparrow and @virkt25, we proposed the following way forward:

(1) findById is going to throw if not found.

(2) We will add a factory method allowing people to obtain a modified Repo instance that returns undefined if not found. The mockup bellow illustrates the idea, we need to find a better name than "relaxed":

const repo = new ProductRepository(Product, db);
// findById throws by default

const repo = new ProductCrudRepo(repo); 
const relaxedRepo: RelaxedCrudRepository<ProductRepository> 
  = repo.createRelaxedRepo(); // find a better name!
// relaxedRepo.findById(id) -> returns undefined not found

We should also update updateById,deleteById for consistency.

(3) Add a type-guard for detecting whether an error is an EntityNotFoundError instance, to make it easier for consumers to detect the "not found error" situation.

(4) Keep controller methods simple, just return this.repo.findById() and iImplement the status code lookup based on err.code in reject action as was shown in the first commit 6891c34.

[bajtos]

I have reworked the pull request to apply the direction outlined in the previous comment.

To get these important changes landed sooner, I decided to leave the following feature out of scope of this pull request:

• Update updateById and deleteById to throw EntityNotFound error for consistency with findById.
• Add a factory method allowing people to obtain a modified Repo instance that returns undefined if not found.

My intention is to start working on these two items as soon as this initial pull request is merged.

As far as I am concerned, this pull request is ready for final review and landing. The changes are split into 5 commits, the review may be easier if you follow commit-by-commit.

@raymondfeng @strongloop/sq-lb-apex @hacksparrow @marioestradarosa @joeytwiddle PTAL, LGTY now?

[bajtos]

coverage/coveralls — Coverage decreased (-0.04%) to 89.569%

The decrease is caused by my changes in CrudRepositoryImpl<T> which does not come with any test coverage from the beginning. Please note this repository is not used anywhere. Our templates, examples and docs are using juggler-backed DefaultCrudRepository which does have test coverage.

[b-admike]

Although I haven't been involved in the discussions, I've read up on some of the conversation and reviewed your commits. Overall LGTM, but I've left some (minor) comments.

[b-admike]

can we leverage this.model.modelName() here?

[bajtos]

I am going to fix the type of the constructor argument model and use this.model (let EntityNotFoundError to obtain the model name).

[b-admike]

Can we add these new error codes to the mapping you created for the 404 case?

[bajtos]

These errors are created inside REST layer, where we know about HTTP status codes and thus error.statusCode is already set. No need to set statusCode in the reject action. In fact, the mapping preserves statusCode if it's already set, and uses the lookup table only when no status code was provided by the error. Adding these new error codes to the lookup table would be a no-op that may confuse users IMO.

[virkt25]

Excellent PR!! Can't wait till this + the follow up PRs land and are released 😄

I do have a concern w.r.t this PR, IIUC we removed the await / async from the controller so the error is passed to the sequence which will delegate to send/reject - Right?

If so, either in this PR / in a follow-up PR can we change all controller methods to return a promise instead of async / await as I find one method not using it very weird and as a user I might think that it's an oversight and the async / await keyword need to be added there. Apart from this, looks 💯

[virkt25]

Rename this file for consistency please.

[virkt25]

I'm confused about the need for this change. All other controller methods are still using async/await. Why does this one need to be treated differently?

[bajtos]

+1 to be consistent.

I am going to revert this change and keep the original async method with return await statement.

[virkt25]

Should we set this to quotedId or not?

[bajtos]

No. The purpose of quotedId is to have a value that's easy to parse in a string, to make it easy for readers to distinguish between e.g. a number 1 and a string "1".

Error properties like entityId and entityName can be serialized in different ways in the HTTP response. When using JSON, the distinction between different types (undefined, null, a number, a string, an object/array) is preserved.

[raymondfeng]

@virkt25 async is just a flag for a function to use await so that the vm can suspend/resume the function execution. From the caller’s perspective, the async function is not different from a function returning a Promise. If your function does not use await, there is no need to mark it async.

[hacksparrow]

allow apps and extensions to contribute additional mappings.

Just a note: allow add, disallow edit and delete.

[hacksparrow]

Looks pretty good to land.

[bajtos]

Landed 🎉