Handle null on findOne function in DefaultCrudRepository class

[hosiduy]

Description / Steps to reproduce / Feature proposal

Hi there,
In DefaultCrudRepository class in /node_modules/@loopback/repository/dist8/src/repositories/legacy-juggler-bridge.js file, we have code:

async findOne(filter, options) {
        const model = await ensurePromise(this.modelClass.findOne(filter, options));
        return this.toEntity(model);
    }
toEntity(model) {
        return new this.entityClass(model.toObject());
    }

Current Behavior

The bug happends when function: findOne return null and pass null to function: toEntiry(). So we have null.toObject() -> Cannot read property 'toObject' of null error.

Expected Behavior

Expect: same as findById function.

async findOne(filter, options) {
        const model = await ensurePromise(this.modelClass.findOne(filter, options));
        if (!model) {
            throw new Error(`no ${this.modelClass.name} found with id "${id}"`);
        }
        return this.toEntity(model);
    }
    async findById(id, filter, options) {
        const model = await ensurePromise(this.modelClass.findById(id, filter, options));
        if (!model) {
            throw new Error(`no ${this.modelClass.name} found with id "${id}"`);
        }
        return this.toEntity(model);
    }

Thanks.

Acceptance Criteria (from @jannyHou)

• Check null for findOne methods in file: https://github.com/strongloop/loopback-next/blob/c553f1171ca86eb13ba7473d73d2e0d1b8de29de/packages/repository/src/repositories/legacy-juggler-bridge.ts
• Either return an null or throw error(story owner can decide which one is proper)
  • For method like findOne, we probably don't want to throw error for empty result, we can handle null in toEntity by returning an empty entity.
  • When throw error, return the proper http error status code.
• test case

See Reporting Issues for more tips on writing good issues

[jannyHou]

@hosiduy Thank you for catching and bringing up the issue.
As a more general solution, I think we should check null or empty array for all CRUD methods in file: https://github.com/strongloop/loopback-next/blob/c553f1171ca86eb13ba7473d73d2e0d1b8de29de/packages/repository/src/repositories/legacy-juggler-bridge.ts

For method like findOne, we probably don't want to throw error for empty result, we can handle null in toEntity by returning an empty entity.

[dhmlau]

@jannyHou @bajtos , do we have enough information to estimate or should put to our backlog directly? Thanks.

[jannyHou]

Acceptance Criteria:

• Check null or empty array for all CRUD methods in file: https://github.com/strongloop/loopback-next/blob/c553f1171ca86eb13ba7473d73d2e0d1b8de29de/packages/repository/src/repositories/legacy-juggler-bridge.ts
• Either return an empty entity or throw error(story owner can decide which one is proper)
  • For method like findOne, we probably don't want to throw error for empty result, we can handle null in toEntity by returning an empty entity.

[jannyHou]

@dhmlau I would suggest we do a quick estimation tomorrow. Then put to our backlog.

[bajtos]

Either return an empty entity or throw error(story owner can decide which one is proper)

IMO, findOne should return null when no entity was found for consistency with the current LB3.x behavior. I am proposing the following fix:

toEntity(model) {
  if (model === null) return null;
  return new this.entityClass(model.toObject());
}