feat(repository): add InclusionResolver type and includeRelatedModels

[agnes512]

resolves #3445

When reviewing, select hide white space changes would make it easier to review 😄

• Add InclusionResolver interface, includeRelatedModels and isInclusionAllowed helpers.
• Used stubbed resolvers for the unit tests.

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 👈

[jannyHou]

😎 Great start! I left a few comments.

[raymondfeng]

public readonly inclusionResolvers: Map<string, InclusionResolver<T, Entity>> = new Map();

[raymondfeng]

I wonder if sourceEntities needs to have a generic type S. It's not helping the compiler much.

[jannyHou]

@raymondfeng I suggested to add a generic type S to avoid specifying the source entity's type like

const resolver: InclusionResolver = async entities => {
        // you need to specify Category....
        const c = entities[0] as Category;
        const product: Product[] = await categoryRepo.products(c.id).find();
        return [product];
      };

      categoryRepo.inclusionResolvers.set('products', resolver);

In the resolver function.

See the current resolver function

vs

previous resolver function

[raymondfeng]

I see. It sounds good then.

[jannyHou]

On a second thought...maybe the resolver should be written as

const hasManyResolver: InclusionResolver = async (entities: Category[]) => {
    const products: Product[] = [];

    for (const category of entities) {
      const product = await categoryRepo.products(category.id).find();
      products.push(product);
    }
    return products;
  };

?
Then we don't need the generic type.

[raymondfeng]

The current signature with S seems to be better.

[bajtos]

IIRC, generic arguments are pretty useless here. At high level, we need a registry of resolvers, mapping from relation name (e.g. products) to a resolver. This registry cannot use generic types and has to fall back to Entity.

That's why I am using the following definition in my spike:

https://github.com/strongloop/loopback-next/blob/2fa5df67181cdcd23a5dce90c9c640fe75943cb8/packages/repository/src/relations/relation.types.ts#L121-L134

I am concerned that the current design creates a false sense of safety - the resolver is implemented with the assumption that it will be called with certain S and T types, but in reality the code calling it will always allow any Entity.

Having said that, I see how the stubbed resolvers are easier to implement when generic arguments are in place.

I don't have enough insight now to be able to judge what aspect is more important.

[bajtos]

Ha, I take my comment back. When I reviewed code below, I found that it is able to provide a meaningful value for S.

public readonly inclusionResolvers: Map<
  string,
  InclusionResolver<T, Entity>
> = new Map();

Nice trick! I agree that we should keep the generic argument for the type of the source entity.

The remaining question is whether we need the second generic argument for the type of the target (related) entity?

[nabdelgadir]

It would help the developer with the return type for each resolver (e.g. restricting to type Product), but I don't think it's too important.

[bajtos]

I reviewed the production code, will try to review the tests tomorrow.

[bajtos]

This test file is growing quite large. Can we split it into two files please? One test file for findByForeignKeys (this can be hopefully just a rename from relation.helpers.unit.ts to something like find-by-foreing-key.ts or relations-helpers/find-by-foreign.keys, with no code changes needed), another test file for the new helper includeRelatedModels.

[bajtos]

IIRC, generic arguments are pretty useless here. At high level, we need a registry of resolvers, mapping from relation name (e.g. products) to a resolver. This registry cannot use generic types and has to fall back to Entity.

That's why I am using the following definition in my spike:

https://github.com/strongloop/loopback-next/blob/2fa5df67181cdcd23a5dce90c9c640fe75943cb8/packages/repository/src/relations/relation.types.ts#L121-L134

I am concerned that the current design creates a false sense of safety - the resolver is implemented with the assumption that it will be called with certain S and T types, but in reality the code calling it will always allow any Entity.

Having said that, I see how the stubbed resolvers are easier to implement when generic arguments are in place.

I don't have enough insight now to be able to judge what aspect is more important.

[bajtos]

Ha, I take my comment back. When I reviewed code below, I found that it is able to provide a meaningful value for S.

public readonly inclusionResolvers: Map<
  string,
  InclusionResolver<T, Entity>
> = new Map();

Nice trick! I agree that we should keep the generic argument for the type of the source entity.

The remaining question is whether we need the second generic argument for the type of the target (related) entity?

[jannyHou]

👍 🚢