feat(example-todo-list): add navigational properties to todo-list example

[nabdelgadir]

See #2633.

• Overwrote find and findById functions in TodoRepository, TodoListRepository, TodoListImage to include a hard-coded retrieval of related models.
  • Improve Inclusion Resolver #3195 is for improvement of this
• Updated response schemas for controller methods find and findById to leverage getModelSchemaRef and includeRelations
• Updated TodoList tutorial

Note: since HasOneRepository<T> doesn't have a find function, the image property wasn't included in the TodoList instances as part of this PR.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• 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

👉 Check out how to submit a PR 👈

[bajtos]

feat: include relations in Model json data

See #3188

[bajtos]

This is great!

The changes look very good at high level. There are few high-level things that will need improvement, but as I commented below, we should leave them out of scope of this initial patch and create new issues.

[bajtos]

FYI: In the future, these changes should be made by lb4 controller or lb4 relation command automatically, see the following template files:

• packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs
• packages/cli/generators/relation/templates/controller-relation-template-has-many.ts.ejs
• packages/cli/generators/relation/templates/controller-relation-template-belongs-to.ts.ejs

Could you please check if we have a story to make those changes? If we don't have it yet, then please create a new one and add it to the epic #1352 Inclusion of related models

[nabdelgadir]

Created #3204

[bajtos]

Ideally, integration tests should not depend on the entire application, they should create Repository instances via new. Only acceptance tests start the full application so that they can make HTTP requests.

See our docs:

• Test your repositories against a real database
• Create a test datasource
• and let's not forget to Clean the database before each test.

I think we may need few iterations to find the best way how to write the integration tests for example-todo-list repositories. Let's leave this refactor out of scope of this pull request, so that we can land it sooner. Having said that, I'd like to keep #2633 open until the refactor is done. I edited PR description and changed the keyword "Closes" to "See".

[nabdelgadir]

I updated the setup in 0ad1a23 wdyt?

Edit: I'm going to squash the commits and land this because I know it's blocking your spike but the changes are in the *.integration.ts files. Let me know if there's something else I can do so we can close #2633.

[bajtos]

@nabdelgadir I think the current version is good enough and we can close #2633 as done 👍 (Assuming that all items in Acceptance criteria are done now.)

[bajtos]

Since juggler is not aware of LB4 relations, perhaps we should move the code deleting filter.include to the base repository class DefaultCrudRepository, so that we don't have to repeat this logic everywhere.

What do you think?

/cc @raymondfeng @strongloop/loopback-maintainers

[bajtos]

The possible downside I see in such move:

Existing application don't have code to handle filter.include, it's passed down to juggler which rejects it and as a result, the requests end with an error (hopefully 400 Bad Request, possibly 500 Internal Server Error). The client knows that the include filter property is not supported.

If we change DefaultCrudRepository to discard include, then juggler will happily run the query and return back results, but with no related models. From the client's perspective, the include argument seems to be silently ignored :( I consider that a poor user experience.

I am proposing the following solution:

Modify DefaultCrudRepository to understand filter.include, call a new method that custom Repository implementations can override to supply their own inclusion resolution, and implement that new method by throwing a helpful error.

Implementation in DefaultCrudRepository:

{
  async find(
    filter?: Filter<T>,
    options?: Options,
  ): Promise<(T & Relations)[]> {
    const jugglerFilter = {...filter, include: undefined} as legacy.Filter;
    const models = await ensurePromise(
      this.modelClass.find(jugglerFilter, options),
    );
    const entities = this.toEntities(models);
    await this._includeRelatedModels(entities, filter, options);
    return entities;
  }

  // also modify findOne and findById similarly

  protected async _includeRelatedModels(
    entities: (T & Relations)[],
    filter: Filter<T>,
    options?: Options,
  ): Promise<void> {
    const msg = 'Inclusion of related models is not supported yet. ' + 
      'Please remove "include" property from the "filter" parameter.';
    const err = new Error(msg);
    err.code = 'FILTER_INCLUDE_NOT_SUPPORTED';
    throw err;
  }
}

With this change in place, the repositories in our example-todo-list application can supply custom implementation of _includeRelatedModels and don't have to overwrite all find* methods.

The missing piece is mapping FILTER_INCLUDE_NOT_SUPPORTED error code to HTTP status code, this should be done in reject.provider.ts:

https://github.com/strongloop/loopback-next/blob/23f91c19844d803347fa517fb2c4d66391692148/packages/rest/src/providers/reject.provider.ts#L14-L16

[bajtos]

On the second thoughts, maybe the duplication is ok in this stage, because the duplicated code is only temporary and will be removed when we figure out the inclusion resolver? By keeping the changes in example-todo-list only and not modifying DefaultCrudRepository, we will keep our options open. If we realize that we need a different API for _includeRelatedModels or even a different design, then we have free hands to implement anything we like.

Thoughts?

[b-admike]

I actually like the proposal to add _includeRelatedModels in DefaultCrudRepository and require custom repositories to supply their own version of this function.

[nabdelgadir]

I think we should wait until #3195 is done especially so +1 to waiting in case we want a different approach.

[bajtos]

LGTM.

[b-admike]

Nice work 👏LGTM.