Loopback: Did some 3.0 PUT -> PATCH stuff get into 2.30.0?

[notbrain]

Just ran some travis tests this morning and they were expecting PUT, but got PATCH.

Local run ran fine until I reinstalled with npm install. Our package.json has "loopback": "^2.27.0", which I would think would be backwards compatible.

Or is PUT/PATCH functionality not considered a breaking change? Broke our tests!

[Amir-61]

Just ran some travis tests this morning and they were expecting PUT, but got PATCH.

Could you please elaborate more?

Or is PUT/PATCH functionality not considered a breaking change?

By default update* methods or pre-existing endpoints should not change

In Loopback 2.30.0 we exposed more endpoints; for more information please see our doc

CC: @bajtos

[notbrain]

Ah ok looking into this further, maybe it was a lb-ng generator update since it was some karma angular tests that switched from sending PUT to PATCH:

Tests were passing on local dev env, but with a previous version of loopback (probably a week old npm install; edit: 2.29.1). Then this morning 10am PDT, this result showed up on Travis CI:

1) updates the shipping instructions due date
     Admin booking confirmation unit tests Admin Booking Confirmation Controller
     Error: Unexpected request: PATCH /api/Orders/1
Expected PUT /\/api\/Order.*/ in /Users/notbrain/src/proteus/dist/js/lib.min.js (line 1646)

2) can save and update an order/quote criteria
     Admin editing quote request unit tests Quote Details Admin controller
     Error: Unexpected request: PATCH /api/QuoteCriteria
Expected PUT /\/api\/QuoteCriteria.*/ in /Users/notbrain/src/proteus/dist/js/lib.min.js (line 1646)

3) updates the company name
     Unit tests for Admin Org management AdminDashboardOrgCtrl
     Error: Unexpected request: PATCH /api/Orgs?id=1
Expected PUT /\/api\/Orgs\?.*/ in /Users/notbrain/src/proteus/dist/js/lib.min.js (line 1646)

When I switched the tests to expect PATCH instead of PUT, they broke locally but passed on Travis. The only difference being we rebuild the full suite on Travis for each build. Was just alarming that those angular resources would be changed and still be considered 2.* compatible.

We'll just update to 2.30 and continue on, and curious that this sort of thing was included in a semantic version compatible version.

[notbrain]

UPDATE Looks like I was premature in saying things didn't break elsewhere--seeing a change with Promise.join() where the .join method is no longer found (we are overriding Promise with bluebird at a global boot-script level, so scratching our heads). We have now moved to an explicit "loopback":"2.29.0" in our package.json to make sure nothing in 2.30.0 gets in to break stuff.

[Amir-61]

we are overriding Promise with bluebird at a global boot-script level, so scratching our heads

Sorry to hear that! So if I understood this is not a LoopBack issue by it self; if that's right let's please close this ticket.

Thanks!

[Amir-61]

If ^ is not correct please provide more info about the bug (if there is any).

[notbrain]

@Amir-61 This is still a breaking change from 2.29.0 with regard to PUT and PATCH lb-ng differences that break spec. Other stuff is broken as well, so I can't in good conscience close this. Was just commenting that other instability resulted from blindly upgrading to 2.30.0 from 2.29.1.

[notbrain]

These look like the culprits (comparing 2.29.1 to 2.30.0):

diff --git a/lib/persisted-model.js b/lib/persisted-model.js
index 7349927..d3e9a6f 100644
--- a/lib/persisted-model.js
+++ b/lib/persisted-model.js
@@ -565,14 +622,35 @@ module.exports = function(registry) {
       http: {verb: 'post', path: '/'}
     });

-    setRemoting(PersistedModel, 'upsert', {
-      aliases: ['updateOrCreate'],
-      description: 'Update an existing model instance or insert a new one into the data source.',
+    var upsertOptions = {
+      aliases: ['patchOrCreate', 'updateOrCreate'],
+      description: 'Patch an existing model instance or insert a new one into the data source.',
       accessType: 'WRITE',
-      accepts: {arg: 'data', type: 'object', description: 'Model instance data', http: {source: 'body'}},
-      returns: {arg: 'data', type: typeName, root: true},
-      http: {verb: 'put', path: '/'}
-    });
+      accepts: { arg: 'data', type: 'object', http: { source: 'body' }, description:
+        'Model instance data' },
+      returns: { arg: 'data', type: typeName, root: true },
+      http: [{ verb: 'patch', path: '/' }],
+    };

@@ -687,13 +784,21 @@ module.exports = function(registry) {
       http: {verb: 'get', path: '/count'}
     });

-    setRemoting(PersistedModel.prototype, 'updateAttributes', {
-      description: 'Update attributes for a model instance and persist it into the data source.',
+    var updateAttributesOptions = {
+      aliases: ['patchAttributes'],
+      description: 'Patch attributes for a model instance and persist it into the data source.',
       accessType: 'WRITE',
-      accepts: {arg: 'data', type: 'object', http: {source: 'body'}, description: 'An object of model property name/value pairs'},
-      returns: {arg: 'data', type: typeName, root: true},
-      http: {verb: 'put', path: '/'}
-    });
+
+      accepts: { arg: 'data', type: 'object', http: { source: 'body' }, description: 'An object of model property name/value pairs' },
+      returns: { arg: 'data', type: typeName, root: true },
+      http: [{ verb: 'patch', path: '/' }],
+    };
+
+    setRemoting(PersistedModel.prototype, 'updateAttributes', updateAttributesOptions);
+
+    if (!options.replaceOnPUT) {
+      updateAttributesOptions.http.push({ verb: 'put', path: '/' });
+    }

[Amir-61]

@notbrain

@Amir-61 This is still a breaking change from 2.29.0 with regard to PUT and PATCH lb-ng differences that break spec.

TBH I'm not clear what issue you are facing in LoopBack by itself, @bajtos @davidcheung you are more familiar with our angular sdk; could you please chime in?

Please note that there is no breaking change in terms of endpoints in LoopBack 2.x;

Regarding #2640-comment please aslo see the rest of changes that updateOrCreate is exposed not only on PATCH verb , but also PUT if options.replaceOnPUT is false which is the case for LoopBack 2.x by default. So here is the bigger picture of changes of the one you mentioned in the previous post:

    var upsertOptions = {
      aliases: ['patchOrCreate', 'updateOrCreate'],
      description: 'Patch an existing model instance or insert a new one into the data source.',
      accessType: 'WRITE',
      accepts: { arg: 'data', type: 'object', http: { source: 'body' }, description:
        'Model instance data' },
      returns: { arg: 'data', type: typeName, root: true },
      http: [{ verb: 'patch', path: '/' }],
    };

    if (!options.replaceOnPUT) {
      upsertOptions.http.push({ verb: 'put', path: '/' });
    }
    setRemoting(PersistedModel, 'upsert', upsertOptions);

All of these changes should be documented in our doc

[notbrain]

There is still a breaking change in the way that the angular sdk generates resource endpoints to communicate with loopback _based on changes in loopback itself_. Just saying that these probably should have waited for 3.0 to be released to be included. Otherwise what's the point of semantic versioning if we can't update minor versions when the REST verbs change?

[bajtos]

Hi @notbrain, thank you for reporting this issue. First of all, we try very hard to preserve backwards compatibility, you can look up the discussions in pull requests. It was definitely not our intention to break any applications using LoopBack 2.x

This is still a breaking change from 2.29.0 with regard to PUT and PATCH lb-ng differences that break spec. Other stuff is broken as well, so I can't in good conscience close this. Was just commenting that other instability resulted from blindly upgrading to 2.30.0 from 2.29.1.

Here is my understanding of this problem:

1. #2435 added a new endpoint PATCH /my-model/:id for updateAttributes method by prepending it to the list of existing endpoints (PUT /my-model/:id).
2. loopback-sdk-angular uses the first endpoint when generating the client method.
3. As a result, generated clients are using PATCH instead of PUT now.

In that light, I am proposing to fix loopback 2.x to make the PUT endpoint the first one in the list of endpoints. Same change should be applied to upsert endpoints for consistence.

@Amir-61 thoughts?

[Amir-61]

@bajtos

Here is my understanding of this problem:

1.#2435 added a new endpoint PATCH /my-model/:id for updateAttributes method by prepending it to the list of existing endpoints (PUT /my-model/:id).
2.loopback-sdk-angular uses the first endpoint when generating the client method.
3.As a result, generated clients are using PATCH instead of PUT now.

Now I have better understanding of what is happening in our oopback-sdk-angular. Thanks for clarification!

In that light, I am proposing to fix loopback 2.x to make the PUT endpoint the first one in the list of endpoints. Same change should be applied to upsert endpoints for consistence.

@Amir-61 thoughts?

I agree!

[notbrain]

@bajtos @Amir-61 Thanks! I stopped short of trying to understand _why_ the PATCH showed up instead of the PUT, but that would make sense. Only showed up because of the way our karma tests are set up with $httpBackend.expectPUT vs $httpBackend.expectPATCH.

[Amir-61]

Fixed in PR: https://github.com/strongloop/loopback/pull/2670 and https://github.com/strongloop/loopback/pull/2682
It will be available in the next NPM release.