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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 21, 2019

Conversation

Projects
None yet
3 participants
@nabdelgadir
Copy link
Contributor

commented Jun 17, 2019

See #2633.

  • Overwrote find and findById functions in TodoRepository, TodoListRepository, TodoListImage to include a hard-coded retrieval of related models.
    • #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 馃憟

@nabdelgadir nabdelgadir self-assigned this Jun 17, 2019

@nabdelgadir nabdelgadir force-pushed the nav-todo branch 4 times, most recently from 672b0ed to 62637cc Jun 18, 2019

@bajtos bajtos referenced this pull request Jun 20, 2019

Merged

feat(repository): include navigation properties in JSON #3188

2 of 3 tasks complete

@bajtos bajtos added this to the June 2019 milestone milestone Jun 20, 2019

@bajtos

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

feat: include relations in Model json data

See #3188

@nabdelgadir nabdelgadir force-pushed the nav-todo branch 2 times, most recently from fba488c to 82a9c3d Jun 20, 2019

@nabdelgadir nabdelgadir marked this pull request as ready for review Jun 20, 2019

@nabdelgadir nabdelgadir requested review from bajtos and raymondfeng as code owners Jun 20, 2019

@bajtos
Copy link
Member

left a comment

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.

`schema`.

In `src/models/todo-list.controller.ts`, first import `getModelSchemaRef` from
`@loopback/rest`.

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 21, 2019

Member

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

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

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 21, 2019

Author Contributor

Created #3204

Show resolved Hide resolved docs/site/tutorials/todo-list/todo-list-tutorial-repository.md Outdated
Show resolved Hide resolved docs/site/tutorials/todo-list/todo-list-tutorial-repository.md
Show resolved Hide resolved docs/site/tutorials/todo-list/todo-list-tutorial-repository.md Outdated
Show resolved Hide resolved docs/site/tutorials/todo-list/todo-list-tutorial-repository.md
Show resolved Hide resolved examples/todo-list/src/__tests__/integration/todo-list.integration.ts Outdated
Show resolved Hide resolved examples/todo-list/src/__tests__/integration/todo.integration.ts Outdated
let todoListRepo: TodoListRepository;

before(async () => {
app = await givenRunningApplicationWithCustomConfiguration();

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 21, 2019

Member

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:

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".

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 21, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 24, 2019

Member

@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.)

Show resolved Hide resolved examples/todo-list/src/repositories/todo-list-image.repository.ts Outdated
options?: Options,
): Promise<TodoWithRelations> {
// Prevent juggler for applying "include" filter
// Juggler is not aware of LB4 relations

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 21, 2019

Member

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 21, 2019

Member

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:

const codeToStatusCodeMap: {[key: string]: number} = {
ENTITY_NOT_FOUND: 404,
};

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 21, 2019

Member

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?

This comment has been minimized.

Copy link
@b-admike

b-admike Jun 21, 2019

Member

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

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 21, 2019

Author Contributor

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

@nabdelgadir nabdelgadir referenced this pull request Jun 21, 2019

Merged

fix: update sample todo and todo-lists #3201

3 of 7 tasks complete

@nabdelgadir nabdelgadir force-pushed the nav-todo branch 2 times, most recently from 0f98b97 to 0e865d5 Jun 21, 2019

@bajtos

bajtos approved these changes Jun 21, 2019

Copy link
Member

left a comment

LGTM.

@nabdelgadir nabdelgadir force-pushed the nav-todo branch from 0e865d5 to 0ad1a23 Jun 21, 2019

@b-admike
Copy link
Member

left a comment

Nice work 馃憦LGTM.

@nabdelgadir nabdelgadir force-pushed the nav-todo branch from 0ad1a23 to 5d27a85 Jun 21, 2019

feat: add navigational properties to todo-list example
- Overwrote find and findById functions in TodoRepository, TodoListRepository, TodoListImage to include a hard-coded retrieval of related models. #3195 is for improvement of this
- Updated response schemas for controller methods find and findById to leverage getModelSchemaRef and includeRelations
- Updated TodoList tutorial

Co-authored-by: Miroslav Bajto拧 <mbajtoss@gmail.com>

@nabdelgadir nabdelgadir force-pushed the nav-todo branch from 5d27a85 to 3b2b334 Jun 21, 2019

@nabdelgadir nabdelgadir merged commit fedb6a7 into master Jun 21, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 91.813%
Details

@delete-merged-branch delete-merged-branch bot deleted the nav-todo branch Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.