hasMany relation CRUD methods

[b-admike]

Description / Steps to reproduce / Feature proposal

Follow up to #1372. Create the rest of the allowed CRUD methods on the target repository for a hasMany relation between two models to make sure the full set of allowed methods is exposed.

Acceptance Criteria:

Implement the following public CRUD methods APIs for hasMany relations:

• update
• delete
• save
• createAll
• deleteAll
• updateAll

Use https://github.com/strongloop/loopback-next/blob/8acb28ca32433e6047d162d2e26097b7cefb6f51/packages/repository/src/repositories/relation.repository.ts, and the implemented find and create methods from #1342 once it lands for inspiration.

See Reporting Issues for more tips on writing good issues

[jannyHou]

cc @b-admike @bajtos based on our discussion in PR #1403, I would suggest we implement replace insteadof save in this story.

So I am going to add

• delete
• replace
• patch

And we can discuss the other 3 batch operations

• createAll
• patchAll
• deleteAll

I temporarily add them in my PR, if we decide to drop them, I will delete them.
If there is no objection I am going to change the list in story description.

[dhmlau]

Close as #1403 has merged.

[shimks]

I picked up introducing replace for the relation's CRUD methods, but it ended up asking serious design choice on how the target model constructor is going to be passed onto the repository.

I picked up the task thinking that replace was in the scope of DP3, but is it? Can you confirm @jannyHou, @bajtos?

[bajtos]

I picked up the task thinking that replace was in the scope of DP3, but is it? Can you confirm @jannyHou, @bajtos?

I think the scope of DP3 is limited to methods that are reasonably easy to implement. If replace requires too much work then we can leave it out.

I picked up introducing replace for the relation's CRUD methods, but it ended up asking serious design choice on how the target model constructor is going to be passed onto the repository.

Why do you need to access the target model constructor?

Anyhow, inside DefaultHasManyEntityCrudRepository, I think you should be able to access the model (LB4 Entity) constructor via this.targetRepository.entityClass (provided by DefaultCrudRepository) if we modify EntityCrudRepository interface to expose entityClass property and thus force all implementation classes to do the same.

I was reading through the discussion in #1403 and could not find why replace was left out. I vaguely remember there were some issues with how to correctly implement it using the current juggler API which may be not flexible enough.

When you say replace, do you mean "replace all records matching a filter" or "replace a single record with the given id"? The first one is not really possible because "replace" operation replaces the entire instance (all properties including id), but each instance must have different id value. The second is not very practical, because once you know the id of the target model, you can update it using regular endpoint (e.g. PUT /orders/{order-id} instead of PUT /customers/{customer-id}/orders/{order-id}.

[shimks]

We need access to the target model constructor to determine the id property of the model (for replaceById).

I think replace was left out precisely because of the fact that we have no way to get the id key from the model with the way EntityCrudRepository is defined.

That being said, I did it find it odd that we need to implement replace in the first place, especially after considering your points. I think the intention was to have the general CRUD operations (create, read, update/replace, delete) available to our 'default' relation repository. @jannyHou Thoughts on how to go forward this issue?

[jannyHou]

@shimks @bajtos Sorry for late reply, I see all your confusions here, let me elaborate it.

First, please note the only replace method we have in the legacy juggler bridge file is replaceById. It is the only replace method we could leverage in the relation crud repositories.

Then let's discuss the implementation limit/blocker for each signature: replaceWithNewData({idProp: idValue, prop1: 'newVal1', prop2: ''newVal2}), replaceAll, replaceById(idValue, obj):

customer.orders.replaceAll

NOT POSSIBLE. Obviously, replaceById expects the Id value and could only replace one instance at one time, as @bajtos explained above.

customer.orders.replaceById(idValue, obj)

We could do it...but I agree with @bajtos 's concern:

The second is not very practical, because once you know the id of the target model, you can update it using regular endpoint (e.g. PUT /orders/{order-id} instead of PUT /customers/{customer-id}/orders/{order-id}

And I would rather hold on having a replace method in DP3.

customer.orders.replaceWithNewData({idProp: idValue, prop1: 'newVal1', prop2: ''newVal2})

This is the one that requires access to the target model's id property name in the relation repository. We need to know which property in the new data is id, then call replaceById under the hood.

So my opinion is

• signature 1 is not feasible(even for GA)
• signature 2 is useless, so why add it :(
• if signature 3 takes time to build, then leave replace out of the DP3 scope.