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(repository): add InclusionResolver type and includeRelatedModels #3517

Merged
merged 1 commit into from Aug 19, 2019

Conversation

@agnes512
Copy link
Contributor

commented Aug 7, 2019

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 馃憟

@agnes512 agnes512 force-pushed the inclusionresolver branch from 2ac78a0 to a8d066f Aug 8, 2019

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch 3 times, most recently from 2467176 to 273646d Aug 9, 2019

@agnes512 agnes512 force-pushed the inclusionresolver branch 2 times, most recently from ebd6559 to 9151086 Aug 12, 2019

@agnes512 agnes512 marked this pull request as ready for review Aug 12, 2019

@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Aug 12, 2019

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch 3 times, most recently from fe33565 to f455a65 Aug 12, 2019

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch from 602aa1b to 724a09c Aug 14, 2019

@nabdelgadir nabdelgadir changed the title feat(repository): add interface inclusionResolver and the helper feat(repository): add InclusionResolver type and the helper Aug 14, 2019

@nabdelgadir nabdelgadir changed the title feat(repository): add InclusionResolver type and the helper feat(repository): add InclusionResolver type and its helpers Aug 14, 2019

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch from 724a09c to 02fe727 Aug 14, 2019

@nabdelgadir nabdelgadir changed the title feat(repository): add InclusionResolver type and its helpers feat(repository): add InclusionResolver type and includeRelatedModels Aug 14, 2019

@@ -100,6 +101,8 @@ export class DefaultCrudRepository<
> implements EntityCrudRepository<T, ID, Relations> {
modelClass: juggler.PersistedModelClass;

public inclusionResolvers: Map<string, InclusionResolver<T, Entity>>;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 14, 2019

Member
public readonly inclusionResolvers: Map<string, InclusionResolver<T, Entity>> = new Map();
/**
* List of source models as returned by the first database query.
*/
sourceEntities: S[],

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 14, 2019

Member

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Aug 14, 2019

Contributor

@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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 14, 2019

Member

I see. It sounds good then.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Aug 14, 2019

Contributor

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.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 14, 2019

Member

The current signature with S seems to be better.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 15, 2019

Member

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:

export type InclusionResolver = (
/**
* List of source models as returned by the first database query.
*/
sourceEntities: Entity[],
/**
* Inclusion requested by the user (e.g. scope constraints to apply).
*/
inclusion: Inclusion,
/**
* Generic options object, e.g. carrying the Transaction object.
*/
options?: Options,
) => Promise<(Entity | undefined)[] | (Entity[] | undefined)[]>;

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.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 15, 2019

Member

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?

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 16, 2019

Contributor

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.

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch from 02fe727 to 8969d4c Aug 14, 2019

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch from 8969d4c to 50fb53f Aug 15, 2019

@bajtos
Copy link
Member

left a comment

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


describe('findByForeignKeys', () => {
describe('relation helpers', () => {

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 15, 2019

Member

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.

/**
* List of source models as returned by the first database query.
*/
sourceEntities: S[],

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 15, 2019

Member

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:

export type InclusionResolver = (
/**
* List of source models as returned by the first database query.
*/
sourceEntities: Entity[],
/**
* Inclusion requested by the user (e.g. scope constraints to apply).
*/
inclusion: Inclusion,
/**
* Generic options object, e.g. carrying the Transaction object.
*/
options?: Options,
) => Promise<(Entity | undefined)[] | (Entity[] | undefined)[]>;

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.

/**
* List of source models as returned by the first database query.
*/
sourceEntities: S[],

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 15, 2019

Member

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 nabdelgadir force-pushed the inclusionresolver branch 4 times, most recently from 4a445c8 to 82a7a23 Aug 16, 2019

@bajtos
bajtos approved these changes Aug 19, 2019
feat(repository): add InclusionResolver type and includeRelatedModels鈥
鈥 helper function

Add InclusionResolver type, includeRelatedModels and isInclusionAllowed helpers.

Co-authored-by: Nora <nora.abdelgadir@ibm.com>
Co-authored-by: Miroslav Bajto拧 <mbajtoss@gmail.com>

@nabdelgadir nabdelgadir force-pushed the inclusionresolver branch from 82a7a23 to 7d8e85d Aug 19, 2019

@jannyHou
Copy link
Contributor

left a comment

馃憤 馃殺

@nabdelgadir nabdelgadir merged commit c9c39c9 into master Aug 19, 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.001%) to 91.447%
Details

@nabdelgadir nabdelgadir deleted the inclusionresolver branch Aug 19, 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.