[WIP]Expose replace methods for related models

[ivanschwarz]

Description

This PR aims to implement the missing Full replace vs. partial update v3 change for related models. This is a breaking change, but I would argue, that it was already announced in the original v3 release notes, just not implemented. I've tried to be as much consistent as possible with the original implementation (#2316). I've implemented the changes just for the hasMany relation. As soon as I'm assured, that I'm on the right path, I can add it for other relations too.

This PR depends on loopbackio/loopback-datasource-juggler#1616.

@bajtos : Can you review?

Related issues

• Reported in PATCH verb for model relation methods #3139 .

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[bajtos]

@ivanschwarz thank you for the pull request and sorry for the long delay on our side.

I took a quick glance at the proposed changes and they look pretty good to me 👍

This is a breaking change, but I would argue, that it was already announced in the original v3 release notes, just not implemented.

I am afraid I disagree here. Here is a scenario to consider: a LoopBack user is running an application in production, their package.json is saying "loopback": "^3.20.2". Their clients are using PUT relation API. If we proceed with your change and release it as semver-minor, the application outlined above will break after npm install. That's an outcome we want to avoid.

Your patch already contain the core of the solution to this problem - the feature flag replaceOnPUT that can be set on model level.

To move this patch forward, we need that feature flag to be disabled by default for relational methods, in order to preserve backwards compatibility. I do realize replaceOnPUT is enabled by default for regular methods. If we think this will be too confusing, then we need to introduce a different flag for relational methods to avoid confusion. Personally, I am fine to have the same flag replaceOnPUT for both regular and relational methods.

What we can (and probably should) do is to modify the template used by lb model command to explicitly set replaceOnPUT to true and thus enable the desired behavior in all new models and applications. See how validateUpsert is always enabled in the template here.

Merge branch 'master' into feat/expose-replace-methods-for-related-mo…

When bringing in latest changes from master, please use git rebase && git push -f instead of git merge. We are keeping linear history and cleaning up merge commits can be tedious.

[ivanschwarz]

@bajtos : Thank you for the feedback.

Let's say we keep the replaceOnPUT flag for all types of methods. We set the default of replaceOnPUT to true for regular methods and to false for relational methods, like you proposed.

1. User doesn't have the replaceOnPUT flag set. We fallback to true for regular methods, to false for relational methods. Backward compatibility is preserved.
2. User has the replaceOnPUT flag set to false. Backward compatibility is preserved.
3. User has the replaceOnPUT flag set to true. All PUT endpoints for related models will suffer from a breaking change, because they were acting as PATCH endpoints until now.

It's also possible that I didn't understand your suggestions completely and you meant to solve it in a different way. I'm pretty new to LoopBack codebase, so I might be missing something.

I can definitely make the update to the template used by lb model when this PR is merged eventually. I will also update docs.

My apologies for the merging from master. I will make sure to rebase properly to keep it clean here.

[bajtos]

3. User has the replaceOnPUT flag set to true. All PUT endpoints for related models will suffer from a breaking change, because they were acting as PATCH endpoints until now.

Ouch! I did not consider this scenario. You are right about the breaking change my suggestion would introduce.

In that case, I am afraid we have to introduce a new feature flag. I have troubles coming up with a good name though. What do you think about changing the type of replaceOnPUT from true | false into "all" | true | false, where the string all enables the new behavior, and true/false behaves the same way as before? (We can use a different string value than all if we can find one that better describes the intent.)

[bajtos]

@ivanschwarz After LoopBack 4.0 GA was announced last week (the announcement), this repository has moved to Active LTS mode and no longer accepts new features. As a result, we can no longer accept this contribution 😞 Thank you for the understanding.

[ivanschwarz]

@bajtos : Damn, I should've kept track of LoopBack 4 better. Well, all information was available, so it's my fault, that I was not able to finish in time. I was struggling to finish implementation of some of the relationships in loopbackio/loopback-datasource-juggler#1616, because they were implemented in another way, which delayed the whole process. I guess we can close that PR too.

[bajtos]

We are still using loopback-datasource-juggler in LoopBack 4. There are only two breaking changes between juggler@3 used by LB3 and juggler@4 used by LB4: Node.js 8.x or newer is required and we use native Promises instead of Bluebird. The goal was to allow us to start using async functions and await statements.

I think it still makes sense to continue working on loopbackio/loopback-datasource-juggler#1616 and get it landed. The new API will be available to LB4 users. We are not going to back-port your pull request to v3 version of juggler, but if you are ok with running on Node.js 8+ and don't require Bluebird APIs, then you can fork v3 of loopback, update it to use v4 of juggler and land this pull request to your fork.

Obviously, you will have to maintain your fork of loopback until you upgrade to LB4. I think it should be reasonably easy to keep the fork up to date with the upstream repository, because there are not that many changes happening in loopback module and with the change to LTS I expect there will be even less changes.