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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model relation: hasManyThrough #2264

Closed
dhmlau opened this issue Jan 17, 2019 · 37 comments
Closed

Model relation: hasManyThrough #2264

dhmlau opened this issue Jan 17, 2019 · 37 comments
Assignees
Labels
feature parity feature Relations Model relations (has many, etc.)

Comments

@dhmlau
Copy link
Member

dhmlau commented Jan 17, 2019

Open a separate issue to track hasManyThrough per request from @RaphaelDrai in
#2043 (comment)

Hello @bajtos,
It is important for us to have in LB4 the hasAndBelongsToMany (the direct many-to-many relation) that we are using in our LB3 application. We are actually in a transition process to move our LB3 application to LB4 and hasAndBelongsToMany is considered as an high priority.
May I suggest to have a new and separated case for the hasManyThrough in order to avoid confusion with this current case?

@clayrisser
Copy link
Contributor

As suggested by @bajtos, I'm going to start working on the hasManyThrough before finishing the implementation of hasAndBelongsToMany.

#2308

@clayrisser
Copy link
Contributor

I attempted to reimplement hasManyThrough using the hasMany relation with a through property. After implementing it, I now think it might not be the best approach. Below are some of the issues I ran into.

The hasManyThrough has to make a database query before creating a constraint. This forces the createHasManyRepositoryFactory to return an asynchronous function, which breaks the existing API.

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-repository.factory.ts#L66-L79

To keep backwards compatibility, I tried to make the createHasManyRepositoryFactory only return a promise if through is set. This allows the return type to be the following. I should mention that the following return type does give some errors currently.

HasManyRepository<Target> | Promise<HasManyRepository<Target>>

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-repository.factory.ts#L24

@clayrisser
Copy link
Contributor

clayrisser commented Feb 9, 2019

Another issue I ran into is the generics. hasManyThrough needs access to the through repository. This requires passing the repository type using generics, very similar to the way the target repository type gets passed through the generic.

export interface Through extends Entity {}

export function createHasManyRepositoryFactory<
  Target extends Entity,
  TargetID,
  ForeignKeyType,
  Through = Through,
  ThroughID = string
>(
  relationMetadata: HasManyDefinition,
  targetRepositoryGetter: Getter<EntityCrudRepository<Target, TargetID>>,
  throughRepositoryGetter?: Getter<EntityCrudRepository<Through, ThroughID>>,
) {}

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-repository.factory.ts#L45-L46

The challenge with this is that many times, hasMany is not using a through model and repository, so the through generic needs to be optional. This requires setting default generic values, and in my opinion is not ideal.

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-repository.factory.ts#L45-L46

Because the through generics are optional, this means they must be listed last. I would prefer the ForeignKeyType to be listed last.

This is why I think hasManyThrough should be defined in a separate relation altogether.

@clayrisser
Copy link
Contributor

clayrisser commented Feb 9, 2019

I would like to mention that using the hasManyThrough relation will be a little odd no matter how it is implemented because the constraint depends on asynchronous database queries. Using it will look something like the following.

Notice the two awaits

const patients = await (await this.physicianRepository.patients(physicianId)).find();

@clayrisser
Copy link
Contributor

It would be great if ya'll could give me feedback on this.

@clayrisser
Copy link
Contributor

clayrisser commented Feb 9, 2019

I was just thinking, maybe the constraint for hasManyThrough could be built in the hasManyThrough repository (for example in the actual find method). This would most definitely require the hasManyThrough to be implemented as a separate relation.

@clayrisser
Copy link
Contributor

Ok, so I actually was able to get hasMany to work in a fully backwards compatible way with a through property being passed into it. Basically, instead of getting the constraint in the factory function, I pass an asynchronous constraint getter into the repository.

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-repository.factory.ts#L55-L74
https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many.repository.ts#L92

Because I can do it with an asynchronous getConstraint function, I think I will be able to stick with implementing this in the hasMany repository. Currently, all the unit tests are passing in the following pull request.

#2359

@clayrisser
Copy link
Contributor

This also fixes the double await issue.

@clayrisser
Copy link
Contributor

I have a very basic example of hasManyThrough working.
https://github.com/codejamninja/medical-practice-api

You will need to use the code from the following remote and branch to get this to work.
https://github.com/codejamninja/loopback-next/tree/codejamninja/has-many-through-using-has-many

cd loopback-next/packages/repository
yarn build
yarn link
cd medical-practice-api
yarn link @loopback/repository
yarn start
    this.physicians = this.createHasManyRepositoryFactoryFor(
      'physicians',
      getPhysicianRepository,
      getAppointmentRepository
    );

https://github.com/codejamninja/medical-practice-api/blob/master/src/repositories/patient.repository.ts#L32-L36

  @hasMany(() => Physician, { through: () => Appointment })
  physicians: Physician[];

https://github.com/codejamninja/medical-practice-api/blob/master/src/models/patient.model.ts#L18-L19

@clayrisser
Copy link
Contributor

While I do believe the hasMany through relation should be built as part of the hasMany relation to maintain consistent behaviour with loopback 3, the repository and factory logic gets really messy and complex.

IMO, the messiest part is dealing with the through repository factory. The factory function needs a generic type referencing the through entity. However, when not using a hasManyThrough relation, the generic type for the through entity should not be passed through. Origionally I solved this delima by simply not passing the through entity generic and instead directly referenced entity. This is certianly not ideal as it limits the type checking when using through with the hasMany factory.

I decided that the best compromise is to still have hasMany contain the hasManyThrough realtion, but separated the relation repository factories. So, in summary if you want to use hasManyThrough, you just add the through property in your hasMany decorator in your model. But, to create the repository factory, you will use createHasManyThroughRepositoryFactory. Regular hasMany relations will still use the createHasManyRepositoryFactory function.

https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/relations/has-many/has-many-through-repository.factory.ts#L50
https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/repositories/legacy-juggler-bridge.ts#L278-L279
https://github.com/codejamninja/loopback-next/blob/codejamninja/has-many-through-using-has-many/packages/repository/src/repositories/legacy-juggler-bridge.ts#L284

https://loopback.io/doc/en/lb3/HasManyThrough-relations.html#defining-a-hasmanythrough-relation

@clayrisser
Copy link
Contributor

clayrisser commented Feb 12, 2019

If my previous comments are too technical, here are two simple examples of a hasMany relationship. One without a through model, and one with a through model.

hasMany w/o through

This works exactly how hasMany has worked in the past with loopback 4

models/patient.model.ts

import { Entity, model, property, hasMany } from '@loopback/repository';
import { Patient } from '../models';

@model()
export class Physician extends Entity {
  @property({
    type: 'string',
    id: true
  })
  id?: string;

  @hasMany(() => Patient)
  patients: Patient[];
}

repositories/patient.repository.ts

import {
  DefaultCrudRepository,
  HasManyRepositoryFactory,
  repository
} from '@loopback/repository';
import { inject, Getter } from '@loopback/core';
import { MemoryDataSource } from '../datasources';
import { Patient, Physician } from '../models';
import { PhysicianRepository } from '../repositories';

export class PatientRepository extends DefaultCrudRepository<
  Patient,
  typeof Patient.prototype.id
> {
  public readonly physicians: HasManyRepositoryFactory<
    Physician,
    typeof Patient.prototype.id
  >;

  constructor(
    @inject('datasources.memory')
    dataSource: MemoryDataSource,
    @repository.getter('PhysicianRepository')
    getPhysicianRepository: Getter<PhysicianRepository>
  ) {
    super(Patient, dataSource);
    this.physicians = this.createHasManyRepositoryFactoryFor(
      'physicians',
      getPhysicianRepository
    );
  }
}

hasMany w/ through

Notice the factory function is different

models/patient.model.ts

import { Entity, model, property, hasMany } from '@loopback/repository';
import { Appointment, Patient } from '../models';

@model()
export class Physician extends Entity {
  @property({
    type: 'string',
    id: true
  })
  id?: string;

  @hasMany(() => Patient, { through: () => Appointment })
  patients: Patient[];
}

repositories/patient.repository.ts

import {
  DefaultCrudRepository,
  HasManyThroughRepositoryFactory,
  repository
} from '@loopback/repository';
import { inject, Getter } from '@loopback/core';
import { MemoryDataSource } from '../datasources';
import { Patient, Physician } from '../models';
import { AppointmentRepository, PhysicianRepository } from '../repositories';

export class PatientRepository extends DefaultCrudRepository<
  Patient,
  typeof Patient.prototype.id
> {
  public readonly physicians: HasManyThroughRepositoryFactory<
    Physician,
    typeof Patient.prototype.id
  >;

  constructor(
    @inject('datasources.memory')
    dataSource: MemoryDataSource,
    @repository.getter('AppointmentRepository')
    getAppointmentRepository: Getter<AppointmentRepository>,
    @repository.getter('PhysicianRepository')
    getPhysicianRepository: Getter<PhysicianRepository>
  ) {
    super(Patient, dataSource);
    this.physicians = this.createHasManyThroughRepositoryFactoryFor(
      'physicians',
      getPhysicianRepository,
      getAppointmentRepository // notice the through repository getter
    );
  }
}

@clayrisser
Copy link
Contributor

clayrisser commented Feb 12, 2019

I thought I might upload the following file to help clarify the code and variable names in the hasManyThrough implementation

hasone

@dhmlau
Copy link
Member Author

dhmlau commented Feb 12, 2019

@codejamninja, thanks for taking up this story. Is it ok if I assign this issue to you? I've already marked it as community contribution. Thanks.

@clayrisser
Copy link
Contributor

@dhmlau absolutely, thanks!

@clayrisser
Copy link
Contributor

Anyone wanting to get early access to the hasManyThrough relation (or anyone wanting to test it), you can install it the following way.

npm install --save @loopback/repository@git+https://[email protected]/codejamninja/loopback-next.git#npm/has-many-through

or add the following to your package.json file

{
  "dependencies": {
    "@loopback/repository": "git+https://[email protected]/codejamninja/loopback-next.git#npm/has-many-through"
  }
}

@privateOmega
Copy link

@codejamninja Any updates on this feature?

@clayrisser
Copy link
Contributor

I will have updates before the end of the month. I'm a bit crunched with work at the moment.

@clayrisser
Copy link
Contributor

My pull request for this at #2359 is ready.

For those interested in trying it out, you can install it the following way.

npm install --save @loopback/repository@git+https://[email protected]/codejamninja/loopback-next.git#npm/codejamninja/[email protected]

or add the following to your package.json file

{
  "dependencies": {
    "@loopback/repository": "git+https://[email protected]/codejamninja/loopback-next.git#npm/codejamninja/[email protected]"
  }
}

@foxinni
Copy link

foxinni commented Oct 18, 2019

Hi, Tried to run the medical example but ran into this issue:

src/controllers/appointment.controller.ts:51:52 - error TS2345: Argument of type 'typeof Appointment' is not assignable to parameter of type 'typeof Model'.

image

@foxinni
Copy link

foxinni commented Oct 18, 2019

Nevermind. Run npm install --save @loopback/repository@git+https://[email protected]/codejamninja/loopback-next.git#npm/codejamninja/[email protected] and got it working.

@jacksonhyde
Copy link

jacksonhyde commented Jul 1, 2020

What's the status of this feature branch? Any plans to get this merged?

@hacksparrow
Copy link
Contributor

We are currently working on landing it via smaller PRs. A few of them have already landed - https://github.com/strongloop/loopback-next/pulls?q=is%3Apr+is%3Aclosed+hasManyThrough.

@dhmlau
Copy link
Member Author

dhmlau commented Jul 3, 2020

Thanks for the interest in the hasManyThrough relation type. The core implementation is done. What's remaining are adding docs and cli.

cc @agnes512

@MohammedAlasaad
Copy link

MohammedAlasaad commented Jul 3, 2020

Hi @dhmlau , Do you mean its on latest version now? can we use it like..
@hasMany(() => Patient, { through: () => Appointment })
I have latest version "@loopback/core": "^2.9.1", and I tried but still showing error on through.
Property 'model' is missing in type '() => typeof MemberMemberships' but required in type '{ model: TypeResolver<Entity, typeof Entity>; keyFrom?: string | undefined; keyTo?: string | undefined; }'.ts(2741)

@agnes512
Copy link
Contributor

agnes512 commented Jul 3, 2020

@MohammedAlasaad The code are released in "@loopback/repository": "^2.9.0". Notice that it is still an experimental feature, and we are fixing a bug in #5858.

@savan-thakkar
Copy link

savan-thakkar commented Jul 21, 2020

@agnes512 Just updated the application dependencies and now the latest version is "@loopback/core": "^2.9.2" but still not able to create hasManyThrough relation like @hasmany(() => Patient, { through: () => Appointment })
So still its under development or is there any other way to achive it ?
Thanks in advance

@agnes512
Copy link
Contributor

@savan-thakkar Hi, could you check if you have the dependency "@loopback/repository": "^2.10.0" ( the bug is fixed). Notice it is under repository, not core.

@savan-thakkar
Copy link

savan-thakkar commented Jul 21, 2020

@agnes512 Thanks for quick reply and yes its there "@loopback/repository": "^2.10.0" but still I am not able to define the relation.

@agnes512
Copy link
Contributor

@savan-thakkar these are the dependencies:

 "devDependencies": {
    "@loopback/build": "^1.7.1",
    "@loopback/repository": "^2.9.0",
    "@types/debug": "^4.1.5",
    "@types/lodash": "^4.14.157",
    "@types/node": "^10.17.27", 
    "lodash": "^4.17.19"
  },
  "dependencies": {
    "@loopback/core": "^2.9.1",
    "@loopback/testlab": "^3.2.0",
    "@types/debug": "^4.1.5",
    "debug": "^4.1.1",
    "tslib": "^2.0.0"
  },

@savan-thakkar
Copy link

savan-thakkar commented Jul 21, 2020

@agnes512 Thanks for the update. I have checked all the dependencies and everything looks okay but still face issue saying 'Property model is missing in type' when I define the relation like "through: () => MyModelName".

@agnes512
Copy link
Contributor

Closing as done. Feel free to leave comment or open issues if you have questions or find anything wrong. Thanks.

@agnes512
Copy link
Contributor

@savan-thakkar any luck?

@savan-thakkar
Copy link

@agnes512 Yes its fixed by defining the relation like @hasmany(() => TargetModel, {through: {model: () => ThroughModel}}).
And also thanks for the latest cli update as well.

But apart from this I have one more doubt/issue related to saving additional fields of pivot table.
For example if pivot table has more fields other than source and target ids then how we can store those fields in same create api ?

@agnes512
Copy link
Contributor

agnes512 commented Aug 10, 2020

@savan-thakkar
I guess you're asking how to create though model instances with additional info e.g

Appointment{doctorId: 1, patientId: 1, description: 'additional info'}

The create and link methods of hasManyThrough repository are able to specify the throughData by options.throughData.

@savan-thakkar
Copy link

savan-thakkar commented Aug 10, 2020

@agnes512 Yes exactly, I am asking for the same. But is it possible to pass both the model's data in same create request and save the data ? However right now we are able to save the source and target ids automatically in pivot table by create api but not the additional fields of pivot table

@agnes512
Copy link
Contributor

@savan-thakkar Yes, you should be able to do it if the API takes throughData. For example:

await userRepo
        .users(existedUserId)
        .create(
          {name: 'target model - user'},
          {throughData: {description: 'a through model - UserLink'}},
        );

@savan-thakkar
Copy link

@agnes512 Thank you for the exaplaination and help. It works 💯 Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature parity feature Relations Model relations (has many, etc.)
Projects
None yet
Development

No branches or pull requests

10 participants