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

[agnes512]

Implemented link and unlink methods.

(continuation of #4438, #5674 and #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 👈

[agnes512]

My take is we should check target instance exists, if not throw error properly.
Not necessarily in the first implementation but make sure we create a story for it.

@jannyHou commented in #5674 (comment). The current design is to take a whole instance ( not just the ID) and link it without checking the existence. I don't have strong opinion on either of them. Would like to also hear from @hacksparrow and @bajtos .

[bajtos]

My take is we should check target instance exists, if not throw error properly.
Not necessarily in the first implementation but make sure we create a story for it.

@jannyHou commented in #5674 (comment). The current design is to take a whole instance ( not just the ID) and link it without checking the existence. I don't have strong opinion on either of them. Would like to also hear from @hacksparrow and @bajtos .

I am glad you are aware of the edge case and started the discussion about enforcing referential integrity! 👍

So far, LoopBack implements what we call "weak relations", where it's up to the database to enforce any referential integrity. Typically, when using SQL, there is a foreign key constraint configured to ensure the "link" rows cannot point to a source/target model that does not exist, and also that it's not possible to delete a source or a target row before all their "link" rows are removed first. (It's also possible to configure cascading delete, but let's not get distracted.) We have been discussing "strong relations" in the past, but they weren't a priority so far. You can learn more in #2331.

IMO, we should keep the current ("weak") design, if only for consistency with other existing relational APIs, and mention in the documentation that the referential integrity must be configured at database level.

See also #1718

[bajtos]

Good progress.

I am a bit confused about the API of helpers for building constraints. It's possible I don't fully understand how they fit into the bigger picture, but this may be also a sign of subtle bugs in the design.

Can you please take a look at my comments and help me better understand the proposed implementation?

[bajtos]

This is actually an interesting discussion point. Should "through" models have their own primary key (id), or should we use a composite primary key composed from source & target keys (e.g. itemId + customerId)?

@raymondfeng What's your opinion?

[raymondfeng]

Imagine we have Appointment as the through model to connect Doctor and Patient, a patient may have multiple appointments with the same doctor. So doctorId + patientId won't be unique in Appointment model, which should have its PK.

For the relation, we need to use two FKs (doctorId and patientId) instead of PK for Appointment.

[agnes512]

Thank @bajtos for the detailed review 🙇‍♀️ ( I accidentally closed the issue)

Since we'd like link and unlink to just take in id values, I modified/added some helpers. I'd like to summarize them there to help review the PR:

To allow these CRUD functions, we need helpers to generate different kind of constraints to meet our requirements. For relationMetadata:

  keyFrom: 'id', // Source model
  keyTo: 'id',  // Target model
  through: {
    model: () => Link, // through model
    keyFrom: 'sourceId',
    keyTo: 'targetId',
  }

• createTargetConstraint creates -> {id: 1} for the Target model
• createFkValues extracts fks -> 1 or [1, 2]
• createThroughConstraint creates -> {sourceId: 5} for the through model
• createThroughFkConstraint creates -> {targetId: 9} for the through model based on the passed in fks

[bajtos]

Thank @bajtos for the detailed review 🙇‍♀️ ( I accidentally closed the issue)

Since we'd like link and unlink to just take in id values, I modified/added some helpers. I'd like to summarize them there to help review the PR:

Thank you for the summary, it's helpful! 👍

To allow these CRUD functions, we need helpers to generate different kind of constraints to meet our requirements. For relationMetadata:

  keyFrom: 'id', // Source model
  keyTo: 'id',  // Target model
  through: {
    model: () => Link, // through model
    keyFrom: 'sourceId',
    keyTo: 'targetId',
  }

• createTargetConstraint creates -> {id: 1} for the Target model
• createFkValues extracts fks -> 1 or [1, 2]
• createThroughConstraint creates -> {sourceId: 5} for the through model
• createThroughFkConstraint creates -> {targetId: 9} for the through model based on the passed in fks

I am little bit confused about the different id values you are showing (1, 2, 5, 9). Here is my understanding, could you please confirm I am getting it right?

Let's say we have "Doctor" has many "Patient" through "Appointment" relation, a doctor with id d, a patient with id p and an appointment with id a that's linking doctor d with patient p. Then:

• createTargetConstraint(relationMeta, {id: 'a', doctorId: 'd', patientId: 'p'}) returns {id: 'p'}
• createFkValues(relationMeta, {id: 'a', doctorId: 'd', patientId: 'p'}) returns p
• createThroughConstraint(relationMeta, 'd') returns {doctorId: 'd'}
• createThroughFkConstraint(relationMeta, ['p']) returns {patientId: 'p'}

I find the function names a bit difficult to map to the actual behavior. It could be caused by my lack of deeper knowledge of other existing relation helpers, but perhaps we can improve the names a bit? In particular, I find the "fk" part ambiguous because there are two foreign keys involved in a has-many-through relation (through.keyFrom and through.keyTo).

An idea to consider:

• Rename createFkValues to getTargetKeyFromThroughModel. We are not creating anything here, just getting values from an object, right? Also the function accepts a single "through" model instance and extracts a single key (id) value, so I think we should not use plural form ("values") in the name.
• Rename createThroughConstraint to make it clear the constraint is being applied on the "from"/"source" side of the link. For example, createSourceThroughConstraint or createThroughConstraintOnSource.
• Similarly createThroughFkConstraint can be renamed to createTargetThroughConstraint or createThroughConstraintOnTarget.

(On the second though, I like createThroughConstraintOnSource and createThroughConstraintOnTarget more, but please pick the names that you prefer and feel free to come up with your own.)

[bajtos]

Is it necessary to support both Trough and Through[] variants? I noticed this design is affecting multiple helpers. Wouldn't it be simpler to support a single value only (Through) and let the caller of this function to deal with the array case by calling instances.map(i => createFkValues(relationMeta, i))?

[agnes512]

Edit: simplified to through[] (I found type conversions are annoying if I use Through), also simplified the returned type to TargetID.

[agnes512]

I am little bit confused about the different id values you are showing (1, 2, 5, 9). Here is my understanding, could you please confirm I am getting it right?
Let's say we have "Doctor" has many "Patient" through "Appointment" relation, a doctor with id d, a patient with id p and an appointment with id a that's linking doctor d with patient p. Then:

• createTargetConstraint(relationMeta, {id: 'a', doctorId: 'd', patientId: 'p'}) returns {id: 'p'}
• createFkValues(relationMeta, {id: 'a', doctorId: 'd', patientId: 'p'}) returns p
• createThroughConstraint(relationMeta, 'd') returns {doctorId: 'd'}
• createThroughFkConstraint(relationMeta, ['p']) returns {patientId: 'p'}

That's correct 👍

Thank @bajtos . It makes more sense with the new names!
I rename them as follows:

Before	After (helper)	After (factory)
createTargetConstraint	createTargetConstraintOnThrough	getTargetConstraintOnThrough
createFkValues	getTargetKeyFromThroughModel	getTargetKey
createThroughConstraint	createThroughConstraintOnSource	getThroughConstraintOnSource
createThroughFkConstraint	createThroughConstraintOnTarget	getThroughConstraintOnTarget

• Improvements of helpers are in a623754
• Improvements of repository/factory are in 8ad58d4

[jannyHou]

@agnes512 I like the new names of functions 👍 left a question for the test case.

[jannyHou]

Is the fkValues an array? if so I think the type should be TargetID[] :p

[agnes512]

I think the any type escaped from the type checking. Converted it to TargetID[] in 975f25e.

[bajtos]

The new versions looks so much better now, I find it much easier to read & understand. Thank you, @agnes512!

I found few more places where we could improve the names for more clarity, but I am not able to tell where it's important and where it's just "icing on the cake". I'll leave it up to you to decide which comments to address and which to ignore.

[bajtos]

Is it safe to combine constraints this way? Should we use WhereBuilder instead to ensure complex syntax like or is handled correctly?

Suggested change

	const constraints = {...targetConstraint, ...throughConstraint};
	const constraints = new WhereBuilder(targetConstraint).and(throughConstraint).build();

@raymondfeng you may have better insight into what's safe and what's not, could you PTAL?

[bajtos]

• Improvements of helpers are in a623754
• Improvements of repository/factory are in 8ad58d4

Thank you for the links to individual commits. I started to post review comments there and then realized they don't show as a regular PR review 😱 I re-posted my suggestions via regular review and deleted the comments attached to individual commits. You may get few weird notifications pointing to that deleted content, sorry for that! 🙈