feat: adding polymorphic relations support for models

[OnTheThirdDay]

Addressing the feature parity of polymorphic relations from lb3, and the syntax is similar to lb3. The changes are backward compatible with the previous APIs. hasOne, belongsTo, and hasManyThrough relations are supported, while hasMany relation cannot facilitate this feature without a through model.
packages repository and repository-tests are updated.

related to #2487

Signed-off-by: David Zhang [email protected]

Checklist

• DCO (Developer Certificate of Origin) signed in all commits
• 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 👈

API changes:

defining polymorphic models

define the polymorphic models

class Post extends Entity {
id;
}

class Letter extends Post {
}

class Parcel extends Post {
}

define the polymorphic property

class Delivery extends Entity {
// use belonging can be any subclass of Post, i.e. Letter or Parcel
@hasOne(() => Post, {polymorphic: true})
  post: Post;

postType: string;
}

or a customized discriminator

class Delivery extends Entity {
// use belonging can be any subclass of Post, i.e. Letter or Parcel
@hasOne(() => Post, {polymorphic: {discriminator: "post_type"}})
  post: Post;

post_type: string;
}

setting up repository

export class DeliveryRepository extends DefaultCrudRepository {
  public readonly post: HasOneRepositoryFactory<Post, typeof Delivery.prototype.id>;

  constructor(
    dataSource: juggler.DataSource,
    @repository.getter('LetterRepository')
    protected letterRepositoryGetter: Getter<EntityCrudRepository<Post, typeof Post.prototype.id, PostRelations>>,
    @repository.getter('ParcelRepository')
    protected parcelRepositoryGetter: Getter<EntityCrudRepository<Post, typeof Post.prototype.id, PostRelations>>,
  ) {
    super(Delivery, dataSource);
    this.post = this.createHasOneRepositoryFactoryFor(
      'post',
      // use a dictionary of repoGetters instead of a single repoGetter instance
      {Letter: letterRepositoryGetter, Parcel: parcelRepositoryGetter},
    );
    this.registerInclusionResolver('post', this.post.inclusionResolver);
  }
}

Query with inclusions

const result = await deliveryRepo.findById(delivery1.id, {
  include: ['post'],
});

Creation with polymorphic relations

deliveryRepo
.post(deliveryId)
.create(letter1Data, {polymorphicType: "Letter"});

[coveralls]

Pull Request Test Coverage Report for Build 1670301030

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 975 of 1045 (93.3%) changed or added relevant lines in 40 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.8%) to 76.885%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/repository/src/relations/belongs-to/belongs-to.accessor.ts	6	7	85.71%
packages/repository/src/relations/relation.filter.solver.ts	31	32	96.88%
packages/repository/src/relations/relation.helpers.ts	6	7	85.71%
packages/repository/src/repositories/legacy-juggler-bridge.ts	0	1	0.0%
packages/repository/src/relations/belongs-to/belongs-to.repository.ts	24	27	88.89%
packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts	41	44	93.18%
packages/repository-tests/src/crud/relations/acceptance/has-one.relation.polymorphic.acceptance.ts	87	92	94.57%
packages/repository-tests/src/crud/relations/helpers.ts	64	69	92.75%
packages/repository/src/errors/invalid-polymorphism.error.ts	2	8	25.0%
packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts	33	41	80.49%

Totals
Change from base Build 1667482373:	0.8%
Covered Lines:	17773
Relevant Lines:	18691

---

💛 - Coveralls

[achrinza]

Thanks for your contributions, @OnTheThirdDay! The maintainers will try to find time to thoroughly review it; In the mean time, feel reach out here on the #loopback-contributors Slack channel with any queries.

[OnTheThirdDay]

Ahh sorry I think I should check out a new branch before fetching from origin repo to my forked repo... Should we cancel this another workflow job or just let it run?

[achrinza]

No worries; When incorporating changes from master, we'd use git rebase instead of git merge (or git pull --rebase instead of git pull) so as to prevent additional merge commits.

So in this case, could you drop the merge commit, and rebase against master?

Should we cancel this another workflow job or just let it run?

I've went ahead and executed the workflow jobs; After your first PR is merged into this repo, future PRs won't require manual approval. Generally, we encourage users to push as often as they need to test their changes as needed.

For testing individual packages locally, we can run:

npx lerna run --scope='@loopback/repository' test

This can be helpful if you want to quickly test changes without running the entire test suite.

The different ways lerna can be used to scope packages and dependencies can be found here: https://www.npmjs.com/package/@lerna/filter-options

[OnTheThirdDay]

Thanks for the information. I have rebased it.

[achrinza]

Really sorry that I couldn't review this earlier. I've checked the PR and there doesn't seem to be any issue with the code and tests. I've pinged the other maintainers to get their input.

[OnTheThirdDay]

Cool. If you find that the current implementation is ok, I will update the doc/cli/example, and add the feature to support polymorphic types search in inclusion filter.

[OnTheThirdDay]

Previously(the initial commit to support polymorphic types), if an included relation points to a polymorphic type, then the repository only sees it as the base type, so that deeper inclusion is not supported. This commit tries to support a finer-grained inclusion filter by specifying targetType of the polymorphic relation. e.g. we have Person and Student as its subclass, then we want to include studentId field for Student polymorphic type when including a person relation, we can do

include: [
  {
    relation: 'person',
    scope: {
      include: [
        {
          relation: 'studentId',
          targetType: 'Student',
        },
      ],
    },
  },
]

[OnTheThirdDay]

I am half way updating the cli, but I am recently too busy to finish it soon. How about I update the docs first after you finish reviewing the code, and later add the support for cli? The change will not break the current usage.

[achrinza]

Sorry for the delay on getting back on this (Most maintainers are catching up with their own December backlog); I've pinged them and there wasn't any objection to this PR so we should be good to merge.

[OnTheThirdDay]

Cool. I will update the doc later. btw, should it be a single section for polymorphic types or mention this feature in individual relations?

[achrinza]

It should be it's own page, similar to the other relations.

[roberttaylortech]

@OnTheThirdDay Hoping to find some time over the next week or so to review this whole feature and the PR and see how it has come together. :)

Also... could you and I get a little chat going via email? rtaylor.info (at) gmail Some cool things to chat about as well.

[lygstate]

can this be merged now?

[OnTheThirdDay]

can this be merged now?

@lygstate Code-wise I think so, but haven't finish the documentation...

Edit: Just added the documentation.

[OnTheThirdDay]

I have added the documentation. I feel this feature is very straightforward. @achrinza Please have a check.

[achrinza]

The docs LGTM 👍

Assuming there's no test failures, the last step is to squash it into a single commit.

If you'd like to continue the work in adding CLI scaffolding support, we can start a new PR after this is merged.

[OnTheThirdDay]

@achrinza Done :)

[achrinza]

Thanks for your contributions, @OnTheThirdDay! Your PR has been merged 🎉

[mikeevstropov]

Hi everyone. We have a difference betwen belongsTo and hasOne/hasMany in the semantic. BelongsTo adds the foreignKey on the source where hasOne/hasMany will look it on the target. Thats why we can resolve relations in both direction. Relations design came from LB3 is a great reference. But as I undestood (only for the polymorphic version of belongTo) now we have a hasOne decorator that adds foreignKey and foreignType to the source instead of looking into the target. Is it right?

[achrinza]

Hi @mikeevstropov, the semantics of the @hasOne decorator has not changed - Even with polymorphic relations, the foreign key is not at the source. The difference is the addition of a discriminator at the source model

Some examples can be found in the docs: https://loopback.io/doc/en/lb4/Polymorphic-relation.html

Edit: #8667 indicates this may not be the case; Will confirm this in that issue.

[mikeevstropov]

@achrinza Hi! Thank you for reply. As you said, example shows hasOne decorator in the same model with discriminator, and it looks weird. Just to be clear, previously, hasOne/hasMany may be applied to the model B only if the model A has foreignKey and discriminator defined by BelongsTo, because hasOne/hasMany is used to resolve relation in the opposite direction only.

[OnTheThirdDay]

Hi, in LB4 the polymorphism is extended to the owners. In LB3, A belongsTo B, B hasOne A, the B is a polymorphic type(imageable either Product or Employee); while in LB4, either A or B can be polymorphic.

[mikeevstropov]

@OnTheThirdDay Hi! If so, do we have the diffirence between belongsTo and hasOne? If no, how to resolve relation in the opposite direction (get Delivery from Letter or Parcel)? In lb3 you are using hasOne/hasMany for that.

[OnTheThirdDay]

Hi, the doc is currently not showing it(because I wanted to show the difference between a normal relation and polymorphic relation, but might've caused confusing) -- Letter or Parcel have a deliveryId field. Here is an example https://github.com/OnTheThirdDay/lb4-polymorphic-example

@OnTheThirdDay Hi! If so, do we have the diffirence between belongsTo and hasOne? If no, how to resolve relation in the opposite direction (get Delivery from Letter or Parcel)? In lb3 you are using hasOne/hasMany for that.

[mikeevstropov]

Okay, got it. New feature of hasOne adds discriminator to the source and fk to the target and this approach is not matched to relations design of LB2/3. Based on this, the relation between hasOne/hasMany <-> belongsTo is violated. Also we can't migrate to LB4 with polymorphic feature without change the data, because previously the discriminator and fk was at the same model. Guys, do the LB4 trying to save backward compatible with the previous version?

[OnTheThirdDay]

Hi, I am not sure if I am getting it right -- do you mean in LB2/3 A belongsTo B(superclass of C and D), discriminator is on A? I think this is identical to the LB4 implementation. For fk, I think we still have keyTo on A.

[mikeevstropov]

BelongsTo adds a foreignKey to the source.
HasOne/hasMany looks at the foreignKey in target.

Polymorphic belongsTo adds a foreignKey and discriminator to the source.
Polymorphic hasOne/hasMany just looks at the foreignKey and discriminator in target.

HasOne/hasMany is used on the “other side” of a belongsTo relation.
HasOne/hasMany does not extend the source or target by any extra fields.

Polymorphic belongsTo adds imageableId and imageableType to the source - Picture.

{
  "name": "Picture",
  "base": "PersistedModel",
  ...
  "relations": {
    "imageable": {
      "type": "belongsTo",
      "polymorphic": true
    }
  },
...

Picture with related Employee.

{
  "id": "1-picture",
  "imageableId": "1-employee",
  "imageableType": "Employee"
  "imageable": {
    "id": "1-employee",
    "name": "Miroslav Bajtoš"
  }
}

Polymorphic hasOne/hasMany looks at the imageableId and imageableType fields stored in target model. The hasMany definition is almost same as hasOne, the only difference is type should be hasMany (to get list of pictures).

{
  "name": "Employee",
  "base": "PersistedModel",
  ...
  "relations": {
    "picture": {
      "type": "hasOne",
      "model": "Picture",
      "polymorphic": "imageable"
    }
  }
...

Employee with related Picture.

{
  "id": "1-employee",
  "name": "Miroslav Bajtoš"
  "picture": {
    "id": "1-picture",
    "imageableId": "1-employee",
    "imageableType": "Employee"
  }
},

HasOne relation is a degenerate case of a hasMany. Does the new inplementation of polymorphic hasOne resolves the target by foreignKey and discriminator stored in target? Do we have polymorphic belongsTo to store a foreignKey and discriminator at source?

[OnTheThirdDay]

I think we are not adding new fields for hasOne and hasMany when they are only served as the opposite direction of a polymorphic belongsTo.

[mikeevstropov]

@OnTheThirdDay Okay, does the new inplementation of polymorphic hasOne resolves the target by foreignKey and discriminator stored in target?

[OnTheThirdDay]

I think currently the discriminator is not checked, which may cause some problem. I will write a fix.

[mikeevstropov]

I think we are not adding new fields for hasOne and hasMany when they are only served as the opposite direction of a polymorphic belongsTo.

Polymorphic hasOne/hasMany were designed only to resolve opposite direction, i guess
https://loopback.io/doc/en/lb3/Polymorphic-relations.html#hasonepolymorphic-relations

[OnTheThirdDay]

Sorry that I am too busy recently, so still not being able to find time to make a fix to this issue. Hopefully I can find some time in the coming few months. Also happy to assist if anyone's willing to pick it up.