[WIP] feat(repository): hasManyThrough #4438

derdeka · 2020-01-15T14:29:30Z

Just another try / draft / poc to implement hasManyThrough
as the original implementation has birthday in a few days.

Early feedback welcome.

This PR is a continuation of #2359.

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 👈

packages/repository/src/relations/has-many/has-many-through.repository.ts

packages/repository/src/repositories/legacy-juggler-bridge.ts

bajtos · 2020-02-07T16:55:11Z

Thank you @derdeka for the pull request.

@agnes512 & @hacksparrow can you please review?

agnes512

Thank you for adding the skeleton of has many through!

Do we want to implement other details such as resolveHasManyThroughMetadata in this PR or separate them out?

agnes512 · 2020-02-07T19:57:43Z

packages/repository/src/relations/has-many/has-many-through.repository.ts

+    const targetRepository = await this.getTargetRepository();
+    const throughRepository = await this.getThroughRepository();
+    const throughConstraint = this.getThroughConstraint();
+    // TODO(derdeka): How to delete throughInstances?


Do we want to delete the through instances? I was expecting this method to delete the target instances. WDYT?

I think it should delete the through instance too. When the target is deleted, the FK in the through model will no longer reference a valid target model.

On a relational database this can be easily done by a fk trigger on delete cascade. But what should happen on a non-relational database or if there is no trigger?

IMO, this should delete through instances first and then delete the target instances second. That way:

We avoid violating referential integrity enforced by SQL databases by deleting target models that are still references by through models.

We avoid leaving orphaned "through" model instances in the databases. By an orphaned instance I mean an instance that's referencing a target model that no longer exists.

Please make sure to include acceptance-level test for this feature!

Here are two scenario to consider & cover by tests, I'll use "Source", "Target" and "Through" as model names instead of Customer/Seller/Order.

Given through links: {sourceId: 1, targetId: 1} and {sourceId: 2, targetId: 2}.
When I delete all models belonging to source id 1,
then the target model id 1 and the first link is deleted.

Given trough links {sourceId: 1, targetId: 1}, {sourceId: 2, targetId: 1} and {sourceId: 2, targetId: 2}.
When I delete all models belonging to source id 1,
then the target model id 1 and the first two links are deleted. Only {sourceId: 2, targetId: 2} link is preserved.

packages/repository/src/repositories/legacy-juggler-bridge.ts

agnes512 · 2020-02-07T20:02:45Z

packages/repository/src/relations/has-many/has-many-through.repository.ts

@@ -98,3 +105,129 @@ export interface HasManyThroughRepository<
    },
  ): Promise<void>;
 }
+
+export class DefaultHasManyThroughRepository<


Let's add API docs for it.

+1, also include "experimental feature" warning.

derdeka · 2020-03-04T13:31:56Z

packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts

+      const seller = await customerRepo
+        .sellers(existingCustomerId)
+        .create(sellerData, {
+          throughData: orderData,
+        });
+      expect(toJSON(seller)).containDeep(toJSON(sellerData));


Creating a seller though order is not the best example / test usecase for hasManyThrough.
Can anyone give a hint, which models / usecase we could use?

How about Category-Product domain, where each category can have multiple products and each product can belong to multiple categories? The "through" model can be CategoryProduct (or CategoryProductLink?) and the operation to test is "create a new product in the given category".

Alternatively, we can reverse the relation to "Seller has many Customer through Order". I think in such case it's more reasonable to create a new customer via the seller?

hacksparrow · 2020-03-04T16:21:07Z

packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts

+} from '../fixtures/models';
+import {givenBoundCrudRepositories} from '../helpers';
+
+export function hasManyRelationAcceptance(


Why do need to export this function? Also, I don't see any place where it is called.

Same as other relation tests, which are picked up by:
https://github.com/strongloop/loopback-next/blob/098dc971f1fcb00321ca9226a2475c62aa5d00d0/packages/repository-tests/src/crud-test-suite.ts#L57-L58

packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts

hacksparrow · 2020-03-05T10:18:20Z

packages/repository/src/relations/relation.types.ts

@@ -87,12 +87,12 @@ export interface HasManyThroughDefinition extends RelationDefinitionBase {
  /**
   * The foreign key in the source model, e.g. Customer#id.
   */
-  keyFrom: string;
+  keyFrom?: string;


Why make these keys optional? If justified, this change should be PR of its own to minimize the size of the PR.

I'll happy to extract this into a PR if there is agreement in overall structure.

I am ok with these changes, I see they are consistent with what we already have in place for HasManyDefinition (see above in the source code file).

I agree a new PR would allow us to land these fixes sooner, on the other hand such PR would be a bit anemic - since no code is using HasManyThroughDefinition yet, there will be no way how to verify correctness of such small PR.

In that light, I think it may be better better to keep these changes inside this bigger PR.

Cross-posting from #4438 (comment):

I think it would be best to extract the changes in relation definitions, the implementation bits of resolving through relations and the accompanying unit tests into a new pull request, to get a smaller patch that's easier to review & iterate on.

I think this is the best way how to get a PR changing relation definition that's smaller but still meaningful.

…y stub

…FactoryFor

…ository

amitgiri0001 · 2020-03-10T10:02:19Z

Do we have any ETA for this?

dhmlau · 2020-03-27T21:31:52Z

@derdeka, if your PR is ready for review, could you please make it a non-draft PR? Thanks!

bajtos

Great stuff, @derdeka! I like that your pull request includes a code coverage for the different bits involved in new relation type.

I found a possible design problem in the way how definition interfaces for hasManyThrough are designed, see my comments towards the bottom. Considering that this pull request is already quite large at 918 new lines, I think it would be best to to extract the changes in relation definitions, the implementation bits of resolving through relations and the accompanying unit tests into a new pull request, to get a smaller patch that's easier to review & iterate on.

bajtos · 2020-04-02T06:31:20Z

packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts

+      const seller = await customerRepo
+        .sellers(existingCustomerId)
+        .create(sellerData, {
+          throughData: orderData,
+        });
+      expect(toJSON(seller)).containDeep(toJSON(sellerData));


How about Category-Product domain, where each category can have multiple products and each product can belong to multiple categories? The "through" model can be CategoryProduct (or CategoryProductLink?) and the operation to test is "create a new product in the given category".

bajtos · 2020-04-02T06:43:23Z

packages/repository-tests/src/crud/relations/helpers.ts

+  // customerRepo.inclusionResolvers.set(
+  //   'sellers',
+  //   customerRepo.sellers.inclusionResolver,
+  // );


We don't keep code that's commented out. Please uncomment or remove this block before the PR is landed.

Personally, I am fine to leave inclusion resolvers out of scope of this pull request if it enables us to get the initial implementation landed sooner.

bajtos · 2020-04-02T06:45:25Z

packages/repository-tests/src/crud/relations/helpers.ts

+    async () => customerRepo,
+    async () => orderRepo,


Have you considered using Getter.fromValue?

Suggested change

async () => customerRepo,

async () => orderRepo,

Getter.fromValue(customerRepo),

Getter.fromValue(orderRepo),

bajtos · 2020-04-02T06:52:29Z

packages/repository/src/relations/has-many/has-many-through-repository.factory.ts

+  ThroughEntity extends Entity,
+  ForeignKeyType
+> = (
+  fkValue: ForeignKeyType,


Can you please clarify which model (source or target) is this foreign-key type referring to?

Let's say I have a Category model using number as the primary key and a Product model using string (ObjectID) as the primary key. What type should I use for ForeignKeyType?

Also isn't the foreign key type always the same as SourceID or TargetID? As I understand has-many-through, the through table contains rows of (sourceId, targetId) pairs. Are you perhaps trying to support a more advanced use case, where the link is established using a non-PK field, e.g. using Category.name instead of Category.id as the foreign key? I believe such use case is not supported in existing relations (hasMany, belongsTo) yet, I think we should leave it out of the initial implementation of hasManyThrough.

I think ForeignKeyType should be the same type as SourceID. If that's true then I think it's better to call this generic/template parameter as SourceID to make it immediately clear which type is expected.

Let's discuss.

bajtos · 2020-04-02T06:53:59Z

packages/repository/src/relations/has-many/has-many-through-repository.factory.ts

+  HasManyThroughRepository,
+} from './has-many-through.repository';
+
+export type HasManyThroughRepositoryFactory<


Please add a tsdoc comment to all newly created public interfaces and include a warning about experimental status. See e.g. this place for inspiration:

https://github.com/strongloop/loopback-next/blob/2b94fd9cd4cdd3030c83023535c28c8376d67905/packages/repository/src/relations/relation.types.ts#L79-L82

@bajtos We might want to also utilize the tsdoc @beta modifier.

I think @alpha would be more appropriate, see https://api-extractor.com/pages/tsdoc/doc_comment_syntax/#release-tags

Indicates that an API item is eventually intended to be public, but currently is in an early stage of development. Third parties should not use “alpha” APIs.

AFAIK, we are not using these modifiers yet, I have no idea how are they rendered in our API docs (if they are rendered at all).

bajtos · 2020-04-02T07:32:09Z

packages/repository/src/relations/has-many/has-many-through.repository.ts

+  async link(
+    targetModelId: TargetID,
+    options?: Options & {
+      throughData?: DataObject<ThroughEntity>;
+      throughOptions?: Options;
+    },
+  ): Promise<TargetEntity> {
+    throw new Error('Method not implemented.');
+  }
+
+  async unlink(
+    targetModelId: TargetID,
+    options?: Options & {
+      throughOptions?: Options;
+    },
+  ): Promise<void> {
+    throw new Error('Method not implemented.');
+  }


What are your plans for these two not-implemented methods?

I slightly prefer to include their implementation in the initial pull request to verify that the current design of DefaultHasManyThroughRepository has all necessary pieces required to implement link/unlink functionality.

However, if you prefer to leave this out, then I am fine with that too, as long as this limitation is clearly documented somewhere and we have a follow-up task (or a new issue) tracked on GitHub.

bajtos · 2020-04-02T07:36:54Z

packages/repository/src/relations/relation.types.ts

@@ -87,12 +87,12 @@ export interface HasManyThroughDefinition extends RelationDefinitionBase {
  /**
   * The foreign key in the source model, e.g. Customer#id.
   */
-  keyFrom: string;
+  keyFrom?: string;


I am ok with these changes, I see they are consistent with what we already have in place for HasManyDefinition (see above in the source code file).

I agree a new PR would allow us to land these fixes sooner, on the other hand such PR would be a bit anemic - since no code is using HasManyThroughDefinition yet, there will be no way how to verify correctness of such small PR.

In that light, I think it may be better better to keep these changes inside this bigger PR.

bajtos · 2020-04-02T07:40:56Z

packages/repository/src/relations/relation.types.ts


  /**
   * The primary key of the target model, e.g Seller#id.
   */
-  keyTo: string;
+  keyTo?: string;

  through: {


A side note: in the future, I would like to add support for convenience shortcuts, e.g.

@hasMany(() => Seller, {through: () => Order})

See #2359 (comment)

This is out of scope of this pull request though.

bajtos · 2020-04-02T07:42:13Z

packages/repository/src/relations/has-many/has-many.decorator.ts

@@ -17,7 +21,7 @@ import {HasManyDefinition, RelationType} from '../relation.types';
 */
 export function hasMany<T extends Entity>(
  targetResolver: EntityResolver<T>,
-  definition?: Partial<HasManyDefinition>,
+  definition?: Partial<HasManyDefinition | HasManyThroughDefinition>,
 ) {
  return function(decoratedTarget: object, key: string) {
    const meta: HasManyDefinition = Object.assign(


This is not correct for "has-many-through" case, is it?

See the discussion in for #2359 (comment) for more context.

bajtos · 2020-04-02T07:50:54Z

packages/repository/src/relations/relation.types.ts


  /**
   * The primary key of the target model, e.g Seller#id.
   */
-  keyTo: string;
+  keyTo?: string;


While we are discussing HasManyThroughDefinition. I am not sure if it's actually a good idea to have such interface, see the discussion in #2359 (comment) for more context.

Having two different interfaces for hasMany and hasManyThrough relations is already creating extra complexity as can be seen in my comment above related to @hasMany() decorator.

Cross-posting from #2359 (comment):

So far, we are using type to discriminate between different interfaces inheriting from RelationDefinitionBase. See https://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions

If we want to keep using hasMany value for has-many-through relations, then I am fine with that, but let's capture that in the type system too.

We can either get rid of HasManyThroughDefinition completely and move all members to HasManyDefinition, or else modify HasManyThroughDefinition to inherit from HasManyDefinition.

Personally, I like most the option where there is a single HasManyDefinition interface with an optional through field that's used to distinguish between hasMany and hasManyThrough cases. Similarly, HasManyResolvedDefinition can contain an optional through field that's used to distinguish hasManyThrough case, this field can use a type like ThroughResolvedDefinition to tell the compiler that optional properties were resolved and will be always present.

I think it would be best to extract the changes in relation definitions, the implementation bits of resolving through relations and the accompanying unit tests into a new pull request, to get a smaller patch that's easier to review & iterate on.

bajtos · 2020-04-02T08:00:45Z

@codejamninja Ping 👋 Since you are the author of #2359, which kick-started the implementation of hasManyThrough, would you like to join the discussion and/or review the proposed implementation too?

MacGyver26 · 2020-04-09T15:29:01Z

Hello guys !

Do you know when this feature is going to be available in the framework ? thanks in advance.

agnes512 · 2020-05-19T15:37:00Z

hasManyThrough helpers and definitions have been modified/implemented in #5354 . Will work on factory next.

agnes512 · 2020-06-29T19:04:49Z

The code this PR covered are being landed, though the HasManyThrough relation is still in an experimental stage (a bug found in #5852). The documentation will be added in the near future.

derdeka added community-contribution Relations Model relations (has many, etc.) labels Jan 15, 2020

derdeka force-pushed the has-many-through branch from e48642d to cc8d66e Compare January 21, 2020 15:03

KalleV reviewed Jan 27, 2020

View reviewed changes

packages/repository/src/relations/has-many/has-many-through.repository.ts Outdated Show resolved Hide resolved

derdeka force-pushed the has-many-through branch from cc8d66e to cf18e08 Compare January 28, 2020 09:34

derdeka commented Jan 28, 2020

View reviewed changes

packages/repository/src/repositories/legacy-juggler-bridge.ts Show resolved Hide resolved

bajtos requested review from agnes512 and hacksparrow February 7, 2020 16:54

agnes512 reviewed Feb 7, 2020

View reviewed changes

derdeka force-pushed the has-many-through branch 2 times, most recently from 9bd8f60 to 05c1bfe Compare March 3, 2020 20:15

derdeka commented Mar 4, 2020

View reviewed changes

hacksparrow reviewed Mar 4, 2020

View reviewed changes

packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts Show resolved Hide resolved

hacksparrow reviewed Mar 5, 2020

View reviewed changes

derdeka added 10 commits March 6, 2020 17:29

feat(repository): hasManyThrough - modify hasMany decorator

6eccb8d

feat(repository): hasManyThrough - add DefaultHasManyThroughRepositor…

d41faf0

…y stub

feat(repository): hasManyThrough - add HasManyThroughRepositoryFactory

b781b77

feat(repository): hasManyThrough - add createHasManyThroughRepository…

c5d21c5

…FactoryFor

feat(repository): hasManyThrough - implement DefaultHasManyThroughRep…

088e7b5

…ository

feat(repository): hasManyThrough - fix

66c0ab9

feat(repository): make properties optional in HasManyThroughDefinition

3831a1e

feat(repository): implement resolveHasManyThroughMetadata

f1cccf9

feat(repository-tests): add tests for hasManyThrough

c50841c

fix(repository): apply feedback

2b94fd9

derdeka force-pushed the has-many-through branch from 69193a7 to 2b94fd9 Compare March 6, 2020 16:30

derdeka mentioned this pull request Mar 27, 2020

[DO NOT MERGE] feat(docs): add April milestone #4906

Closed

7 tasks

dhmlau added this to the April 2020 milestone Mar 27, 2020

This was referenced Mar 31, 2020

HasManyThrough using HasMany #2359

Closed

Monthly Milestone - April 2020 💪😷🐣 #5010

Closed

bajtos suggested changes Apr 2, 2020

View reviewed changes

bajtos added feature feature parity Repository Issues related to @loopback/repository package labels Apr 2, 2020

jannyHou mentioned this pull request Apr 30, 2020

Monthly Milestone - May 2020 ☀️ 🌎 🍀 #5301

Closed

39 tasks

dhmlau modified the milestones: April 2020, May 2020 May 2, 2020

agnes512 mentioned this pull request May 7, 2020

feat(repository): adding hasManyThrough (through model) to hasMany and its helpers #5354

Merged

7 tasks

dhmlau modified the milestones: May 2020, Jun 2020 May 28, 2020

hacksparrow mentioned this pull request May 29, 2020

Monthly Milestone - June 2020 💪🏻 🌏 🌳 #5607

Closed

13 tasks

This was referenced Jun 1, 2020

feat(repository): add more helpers for HasManyThrough #5605

Merged

feat(repository): CRUD implementation and tests for hasManyThrough Repository #5674

Merged

feat(repository): add repo factory and tests for hasManyThrough #5606

Merged

agnes512 mentioned this pull request Jun 18, 2020

feat(repository): add link and unlink methods for hasManyThrough repository #5719

Merged

7 tasks

This was referenced Jun 26, 2020

Monthly Milestone - July 2020 ☀️🍉🍺 #5837

Closed

test(repository-tests): adding acceptance tests for hasManyThrough relation #5765

Merged

agnes512 closed this Jun 29, 2020

[WIP] feat(repository): hasManyThrough #4438

[WIP] feat(repository): hasManyThrough #4438

Conversation

derdeka commented Jan 15, 2020

Checklist

bajtos commented Feb 7, 2020

agnes512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitgiri0001 commented Mar 10, 2020

dhmlau commented Mar 27, 2020

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Apr 2, 2020

MacGyver26 commented Apr 9, 2020

agnes512 commented May 19, 2020

agnes512 commented Jun 29, 2020