Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultHasOneRepository.get method throws exception if no match found #2472

Closed
shazha opened this issue Feb 26, 2019 · 8 comments
Closed

DefaultHasOneRepository.get method throws exception if no match found #2472

shazha opened this issue Feb 26, 2019 · 8 comments
Labels
developer-experience Issues affecting ease of use and overall experience of LB users help wanted needs discussion

Comments

@shazha
Copy link
Contributor

shazha commented Feb 26, 2019

Description

I have the following two models and HasOne relation for them:

@model()
export class Study extends Entity {
  @property({
    type: 'number',
    id: true,
    generated: true,
  })
  id?: number;

  @hasOne(() => Report)
  report?: Report;

  constructor(data?: Partial<Study>) {
    super(data);
  }
}
@model()
export class Report extends Entity {
  @property({
    type: 'number',
    id: true,
    generated: true,
  })
  id?: number;

  @belongsTo(() => Study)
  studyId: number;

  constructor(data?: Partial<Report>) {
    super(data);
  }
}

When I'm trying to access report with the following code:
const report = await this.studyRepository.report(foundStudy.id).get()
if there is no report for the foundStudy model, it will throw an exception

Current Behavior

Throws exception for HasOne relation if no match found

Expected Behavior

Returns null or undefined for HasOne relation if no match found.

According to the HasOneRepository interface, it should return null if no match found

export interface HasOneRepository<Target extends Entity> {
  /**
   * Find the only target model instance that belongs to the declaring model.
   * @param filter Query filter without a Where condition
   * @param options Options for the operations
   * @returns A promise of the target object or null if not found.
   */
  get(
    filter?: Pick<Filter<Target>, Exclude<keyof Filter<Target>, 'where'>>,
    options?: Options,
  ): Promise<Target>;
}

However, the DefaultHasOneRepository implementation throws an exception if no match found.

export class DefaultHasOneRepository<
  TargetEntity extends Entity,
  TargetID,
  TargetRepository extends EntityCrudRepository<TargetEntity, TargetID>
> implements HasOneRepository<TargetEntity> {

  async get(
    filter?: Pick<Filter<TargetEntity>, Exclude<keyof Filter<TargetEntity>, 'where'>>,
    options?: Options): Promise<TargetEntity> {
   // other codes
    if (found.length < 1) {
      // We don't have a direct access to the foreign key value here :(
      const id = 'constraint ' + JSON.stringify(this.constraint);
      throw new EntityNotFoundError(targetRepository.entityClass, id);
    }
    return found[0];
  }
}
@dhmlau
Copy link
Member

dhmlau commented Feb 28, 2019

@shazha , would you like to submit a patch?
If so, please see our CONTRIBUTING.md.

@shazha
Copy link
Contributor Author

shazha commented Feb 28, 2019

@dhmlau ,sure, I'll do it later this week.

@bajtos
Copy link
Member

bajtos commented Mar 1, 2019

As I commented in #2500, I believe the current implementation is intentional and it's just the tsdoc comment that needs fixing.

You can read through the discussion in #1658 to better understand our reasoning for throwing EntityNotFound error instead of returning undefined or null.

Having said that, I can see how it can be useful to allow hasOne relation to support optional target and return null or undefined when there is no target model associated with the source instance.

Perhaps it's time to resume the work on UncheckedCrudRepository (see #1735) and implement UncheckedHasOneRepository?

@raymondfeng @strongloop/loopback-maintainers thoughts?

@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users needs discussion labels Mar 1, 2019
@bajtos
Copy link
Member

bajtos commented Mar 1, 2019

At the moment, one can write the following code to handle "not found" case:

let report;
try {
  report = await this.studyRepository.report(foundStudy.id).get()
} catch (err) {
  if (err.code === 'ENTITY_NOT_FOUND') {
    report = undefined;
  } else {
    throw err;
  }
}

Besides implementing a new repository class, maybe it's enough to add a new method to the existing HasOne repository, e.g. repo.find(). Another alternative I see is to add a new option to the get method, e.g. repo.get({optional: true}) or repo.get({default: null}). Although I vaguely remember that we have rejected these two approaches in the past discussions.

@bajtos
Copy link
Member

bajtos commented Mar 1, 2019

It is also possible to write a small helper function to convert ENTITY_NOT_FOUND errors to undefined:

async function handleNotFound<T>(result: Promise<T>): Promise<T | undefined> {
  try {
    return await result;
  } catch (err) {
    if (err.code === 'ENTITY_NOT_FOUND') {
      return undefined;
    }
    throw err;
  }
}

// usage
const report = await handleNotFound(
  this.studyRepository.report(foundStudy.id).get()
);

@shazha
Copy link
Contributor Author

shazha commented Mar 4, 2019

@bajtos , thanks for your detailed comments. I totally understand and agree to throwing ENTITY_NOT_FOUND for findById. And yes, I'm currently using try/catch to handle ENTITY_NOT_FOUND error from HasOne repository.

But from a developer's point of view, there is a difference between findById from CRUD repository and get method from HasOne repository. When developers use findById, they're most likely expecting an error to be thrown if the provided id (aka PK) doesn't exist in the database. However, when it comes to HasOne relation and get method from HasOne repository, it's more like findByFK rather than findByPK. In this case, developers are expecting to find a related model instance by using source model's PK, which is the related model's FK. Because not every single source model instance hasOne related model instance, I feel it's more reasonable to return null or undefined rather than throwing an error. Let the business logic (the controller) handle the rest.

Just my 2 cents, correct me if I'm wrong or misunderstood anything😅

@bajtos
Copy link
Member

bajtos commented Apr 18, 2019

@shazha sorry for being late to respond.

I see two different consumers of HasOne repository:

  1. The REST layer (Controller methods) want to return 404 Not Found when the relation target was not found. It's impractical to convert null responses to 404 errors in every controller method.
  2. Custom code that needs an easy way how to detect when the relation target was not found.

Handling of "not found" case affects more than just get method. Both patch and delete methods need to deal with that situation too. At the moment, these methods throw the same "Not Found" error, so that REST API returns 404.

When implementing a different mode, where get methods returns null when not found, how do you propose to handle "not found" errors in patch and delete?

@dougal83
Copy link
Contributor

Closing as appears no longer relevant. Reopen as necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users help wanted needs discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants