adding upsertWithWhere by mountain1234585 · Pull Request #2539 · strongloop/loopback

mountain1234585 · 2016-07-25T04:19:37Z

Requires loopbackio/loopback-datasource-juggler#1001

slnode · 2016-07-25T04:19:38Z

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

slnode · 2016-07-25T04:19:38Z

Can one of the admins verify this patch?

slnode · 2016-07-25T04:19:38Z

Can one of the admins verify this patch?

slnode · 2016-07-25T04:19:38Z

Can one of the admins verify this patch?

slnode · 2016-07-25T04:19:38Z

Can one of the admins verify this patch?

mountain1234585 · 2016-07-25T04:22:11Z

We are still working on adding tests to this PR.

mountain1234585 · 2016-07-25T08:51:32Z

Changes to test files have been made for upsertWithWhere. Pls review.

bajtos · 2016-08-04T13:15:52Z

+          description: 'An object of model property name/value pairs' },
+      ],
+      returns: { arg: 'data', type: typeName, root: true },
+      http: { verb: 'put', path: '/upsertWithWhere' },


I think we usually use POST for non-root endpoints like /upsertWithWhere.

Alternatively, we can use PATCH to be consistent with upsert that's mapped to PATCH / by default since LB 3.0.

@bajtos Could you please confirm me that since this is a new method which is being introduced we don't need to make it configurable like the way we did with replaceOnPUT flag for replaceOrCreate/updateOrCreate and replaceById/patchAttributes, right?

@bajtos ping Please see ^ Also please let @mountain1234585 know whether they should go for POST /updateOrCreate or PATCH /updateOrCreate? I have no strong opinion in this regard.

CC: @raymondfeng

Could you please confirm me that since this is a new method which is being introduced we don't need to make it configurable like the way we did with replaceOnPUT flag for replaceOrCreate/updateOrCreate and replaceById/patchAttributes, right?

Yes, no need to make this configurable.

whether they should go for POST /updateOrCreate or PATCH /updateOrCreate? I have no strong opinion in this regard.

Let's go with POST /upsertWithWhere or even POST /updateOrCreateWithWhere.

@mountain1234585 Lets got for POST /upsertWithWhere and please make sure you add enough test coverage; please look into PR#2316 which gives you an idea what test coverage should be added in this patch.

bajtos · 2016-08-04T13:16:34Z

@slnode ok to test

bajtos · 2016-08-04T13:17:01Z

@mountain1234585 thank you for the pull request, it looks mostly good to me. I'll let @Amir-61 to lead the review and give this the final approval.

Amir-61 · 2016-08-04T18:50:38Z

I guess this patch is missing lots of test cases; at least tests in test/access-control.integration.js, test/model.test.js and test/remoting.integration.js. I recommend if you look into PR#2316, that should help a lot and give you a good direction.

Amir-61 · 2016-08-04T18:52:47Z

I see some commit-linter issues:

**  4 problems found:
**    5574dfe - Merge remote-tracking branch 'strongloop/master' in: First line should be 50 characters or less (saw 69)
**    60463b8 - Merge remote-tracking branch 'strongloop/master' in: First line should be 50 characters or less (saw 69)
**    820f78b - Merge remote-tracking branch 'strongloop/master' in: First line should be 50 characters or less (saw 69)
**    51f2b5d - Merge remote-tracking branch 'strongloop/master' in: First line should be 50 characters or less (saw 69)

Also you PR needs to be rebased on top of master.

Last but not least I see all of tests fail. Please look into failures and make sure they all pass.

Thanks

mountain1234585 · 2016-08-08T11:43:42Z

@Amir-61 While I was running the tests in model.test.js and remoting.integration.js as is without adding any additional tests, there were many failures I could see. I am running mocha test/remoting.integration.js and mocha test/model.test.js in loopback directory. Please let me know , if I am missing out on configuring . I could do a clean run for access-control.integration.js.

mountain1234585 · 2016-08-09T10:29:52Z

Was able to run the tests using npm test.
I have added remaining tests and they are present in test/model.test.js, test/access-control.integration.js, test/remote.integration.js and test/replication.test.js

Amir-61 · 2016-08-09T17:47:26Z

@Amir-61 While I was running the tests in model.test.js and remoting.integration.js as is without adding any additional tests, there were many failures I could see. I am running mocha test/remoting.integration.js and mocha test/model.test.js in loopback directory. Please let me know , if I am missing out on configuring

You need to rebase all your commits on top of master and I believe the failures have been solved in the latest commits

mountain1234585 · 2016-08-10T21:39:46Z

@Amir-61 I have squashed all commits into one and rebased on top of master.

Amir-61 · 2016-08-12T18:19:02Z

@rmg I'm looking into failures; however the logs does not give that much info why tests fail. One of the logs is here. Can you find the reason why tests fail which may help @mountain1234585 to resolve it and get this patch pass our CI?

superkhau · 2016-08-12T19:48:36Z

+   * @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
+   * @param {Object} model Updated model instance.
+   */
+


remove one extra space in front of =

You can also align them together like:

PersistedModel.patchOrCreateWithWhere = PersistedModel.upsertWithWhere = function upsertWithWhere(...) { });

mountain1234585 · 2016-08-19T15:04:20Z

@Amir-61 @superkhau
I have updated PR#1052 with my findings.

Amir-61 · 2016-08-23T22:36:02Z

+        });
+      });
+    });
+    lt.beforeEach.givenModel('accountWithReplaceOnPUTtrue');


accountWithReplaceOnPUTtrue is added specifically for testing replace methods when replaceOnPUT flag is set to true; I don't think you should test with that model; I believe you should test it with account model instead to make the purpose of test clear.

+1, please use account instead.

@bajtos Should I create a new model called account under test/fixtures/common/models .

I see in test/fixtures/common/models, there is already account.json and inside which the "name" is specified as "accountWithReplaceOnPUTtrue" and plural as "accounts-replacing". Please let me know. For now, I have deleted this block, since I have already added tests for "users" model and "banks" model respectively :

lt.it.shouldBeDeniedWhenCalledAnonymously('POST', '/api/users/upsertWithWhere');
lt.it.shouldBeDeniedWhenCalledUnauthenticated('POST', '/api/users/upsertWithWhere');
lt.it.shouldBeDeniedWhenCalledByUser(CURRENT_USER, 'POST', '/api/users/upsertWithWhere');

lt.it.shouldBeDeniedWhenCalledAnonymously('POST', '/api/banks/upsertWithWhere');
lt.it.shouldBeDeniedWhenCalledUnauthenticated('POST', '/api/banks/upsertWithWhere');
lt.it.shouldBeDeniedWhenCalledByUser(CURRENT_USER, 'POST', '/api/banks/upsertWithWhere');

there is already account.json and inside which the "name" is specified as "accountWithReplaceOnPUTtrue" and plural as "accounts-replacing".

Oh right; by default LoopBack 3.0 sets replaceOnPUT to true, whereas in LoopBack 2.x it is set to false by default; so that's why account.json has a name property accountWithReplaceOnPUTtrue. How about using other available models in test/fixtures/access-control/common/models; probably you can use bank model for instance. I do not have a strong opinion I leave it to you and @bajtos.

Amir-61 · 2016-08-23T22:57:20Z

Regarding your question in 2359-comment, it seems in the example code you posted you are using accounts-replacing model which is added specifically for testing replace methods when replaceOnPUT flag is set to true; I don't think you should test with that model; I believe you should test it with regular models instead to make the purpose of test clear. Apart from that, you should add the failing test in the actual code of the PR so then we can comment on that.

I'm busy with landing your 2.x backporting beside other tasks/responsibilities that I have; I sent PR-1061 which implements notify for find method to make your 2.x PR pass. @bajtos PTAL at this PR. The patch almost looks good to me; however my only concern was the missing test cases.

bajtos · 2016-08-24T11:54:10Z

+      'deleteById(id:any):object DELETE /stores/:id',
+      'count(where:object):number GET /stores/count',
+      'prototype.patchAttributes(data:object):store PATCH /stores/:id',
+      'createChangeStream(options:object):ReadableStream POST /stores/change-stream',


Please move this test to describe('Model shared classes', (the block which starts on line 76) and assert only on the methods related to your patch.

it('has upsertWithWhere remote method', function() { var storeClass = findClass('store'); var methods = getFormattedMethodsExcludingRelations(storeClass.methods); var expectedMethods = [ 'upsertWithWhere(where:object,data:object):store POST /stores/upsertWithWhere', ]; expect(methods).to.include.members(expectedMethods); });

done. thanks.

bajtos · 2016-08-24T11:57:03Z

I quickly reviewed the patch, the code looks mostly good. As I see it, there are three line comments above to address, and then the missing test for ACL and lt.it.shouldBeAllowed to implement/figure out.

mountain1234585 · 2016-08-25T09:51:17Z

@bajtos
I have addressed two comments out of 3 above. For using the account model, I have posted my query in the comment.
Also, I have already added tests in test/access-control.integration.js under /users and /banks.

I tried adding lt.it.shouldBeAllowed under the block lt.describe.whenLoggedInAsUser(starting at line 96) in the describe('/users', function() {} block. But it failed with 404.
lt.describe.whenCalledRemotely('POST', '/api/users/upsertWithWhere', function() {
lt.it.shouldBeAllowed();
});
I haven't added this test case , since it dint pass my local setup.
Pls let me know what to do about it.

Amir-61 · 2016-08-25T21:37:58Z

I tried adding lt.it.shouldBeAllowed under the block lt.describe.whenLoggedInAsUser(starting at line 96) in the describe('/users', function() {} block. But it failed with 404.

It gives 401; You have already added:

    {
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW",
      "property": "upsertWithWhere"
    },

TBH I'm not sure why; @bajtos could you please help @mountain1234585 with this?

Thanks!

PS: @mountain1234585 Please when copy&paste code put them in the code-block

bajtos · 2016-08-29T12:53:33Z

@mountain1234585 please commit your code to github, so that I can run your failing unit test locally on my machine. Also please rebase your patch on top of the current master.

mountain1234585 · 2016-08-30T05:28:50Z

@bajtos
I have added the failing test to the recent commit and rebased it on top of current master as per your suggestion. Please take a look.

access control - integration /users POST /api/users/upsertWithWhere should be allowed:
     AssertionError: expected 401 to be within 100..399
    at Context.<anonymous> (test/helpers/loopback-testing-helper.js:230:39)

Also , I noticed in the recent builds one test case ( not relevant to upsertWithWhere feature is failing).

KeyValueModel REST API provides "keys(filter)" at "GET /keys":
     Error: done() invoked with non-Error: {"statusCode":500,"message":"Internal Server Error"}
    at Test.<anonymous> (test/key-value-model.test.js:132:40)

bajtos · 2016-09-02T10:12:36Z

access control - integration /users POST /api/users/upsertWithWhere should be allowed

Oh, I see it now. Upsert does not support $owner role. This is a known limitation of upsert, see e.g. strongloop/loopback-sdk-angular#125. You can see that there is no ACL entry for upsert either.

We should follow upsert behaviour for upsertWithWhere and simply don't allow upsertWithWhere for the User model.

We should eventually find a way how to fix upsert (and upsertWithWhere) for User model, but that's out of scope of this pull request.

I am proposing to revert the changes in common/models/user.json and remove the appropriate tests.

mountain1234585 · 2016-09-02T12:43:34Z

@bajtos I have made the changes as you have suggested and committed the changes. PTAL.

bajtos · 2016-09-02T12:50:03Z

The patch LGTM in general. I'll leave the final approval to @Amir-61, he has better understanding of the overall context of this work.

Amir-61 · 2016-09-02T17:29:07Z

@slnode test please

Amir-61 · 2016-09-02T17:32:59Z

@slnode test please

Amir-61 · 2016-09-02T18:12:52Z

LGTM; however we need to make sure CI tests pass for Loopback and its dependencies

@rmg CI is failing for PhantomJS; any thought about that; please see logs here

Amir-61 · 2016-09-06T17:35:45Z

@slnode test please

rmg · 2016-09-06T17:55:52Z

@Amir-61 the failing build seems to have been cleared already, but since it is PhantomJS + Windows, I imagine it was probably related to running out of handles again. The VMs were recently rebooted, so that will hopefully give us another few days without those errors coming back.

Backport of #2539

Amir-61 · 2016-09-06T20:48:54Z

Landed. Thanks for the contribution!

Backport of #2539

crandmck added the #community contribution label Jul 25, 2016

mountain1234585 mentioned this pull request Jul 25, 2016

Changes to loopback lib/persisted_model.js to expose "upsertWithWhere" as REST API #2496

Closed

bajtos reviewed Aug 4, 2016
View reviewed changes

bajtos assigned Amir-61 Aug 4, 2016

bajtos added feature juggler-DAO and removed #community contribution labels Aug 4, 2016

Amir-61 mentioned this pull request Aug 4, 2016

upsertWithWhere method loopbackio/loopback-datasource-juggler#1001

Merged

mountain1234585 force-pushed the upsertWithWhere branch from f333a37 to 4faf41a Compare August 10, 2016 21:38

superkhau reviewed Aug 12, 2016
View reviewed changes

Amir-61 reviewed Aug 23, 2016
View reviewed changes

Amir-61 assigned bajtos Aug 23, 2016

bajtos reviewed Aug 24, 2016
View reviewed changes

mountain1234585 force-pushed the upsertWithWhere branch from d75f8e9 to fda002c Compare August 25, 2016 09:59

mountain1234585 force-pushed the upsertWithWhere branch from fda002c to 9eed018 Compare August 30, 2016 05:05

Expose upsertWithWhere method

aef6dca

mountain1234585 force-pushed the upsertWithWhere branch from 9eed018 to aef6dca Compare September 2, 2016 12:41

bajtos mentioned this pull request Sep 2, 2016

Fix remoting metadata for "data" arguments [2.x] #2702

Merged

Amir-61 pushed a commit that referenced this pull request Sep 6, 2016

Expose upsertWithWhere

9620ae6

Backport of #2539

Amir-61 mentioned this pull request Sep 6, 2016

Expose upsertWithWhere #2722

Merged

Amir-61 merged commit a6f8ec6 into strongloop:master Sep 6, 2016

Amir-61 removed the #review label Sep 6, 2016

Amir-61 pushed a commit that referenced this pull request Sep 7, 2016

Expose upsertWithWhere

614bf33

Backport of #2539

Amir-61 pushed a commit that referenced this pull request Sep 7, 2016

Expose upsertWithWhere

4c013de

Backport of #2539

Uh oh!

Conversation

mountain1234585 commented Jul 25, 2016 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slnode commented Jul 25, 2016

Uh oh!

slnode commented Jul 25, 2016

Uh oh!

slnode commented Jul 25, 2016

Uh oh!

slnode commented Jul 25, 2016

Uh oh!

slnode commented Jul 25, 2016

Uh oh!

mountain1234585 commented Jul 25, 2016

Uh oh!

mountain1234585 commented Jul 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 4, 2016

Uh oh!

bajtos commented Aug 4, 2016

Uh oh!

Amir-61 commented Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amir-61 commented Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mountain1234585 commented Aug 8, 2016

Uh oh!

mountain1234585 commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amir-61 commented Aug 9, 2016

Uh oh!

mountain1234585 commented Aug 10, 2016

Uh oh!

Amir-61 commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountain1234585 commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 commented Aug 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 24, 2016

Uh oh!

mountain1234585 commented Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

mountain1234585 commented Jul 25, 2016 •

edited by bajtos

Loading

Amir-61 Aug 4, 2016 •

edited

Loading

Amir-61 Aug 9, 2016 •

edited

Loading

Amir-61 commented Aug 4, 2016 •

edited

Loading

Amir-61 commented Aug 4, 2016 •

edited

Loading

mountain1234585 commented Aug 9, 2016 •

edited

Loading

Amir-61 commented Aug 12, 2016 •

edited

Loading

mountain1234585 commented Aug 25, 2016 •

edited

Loading

mountain1234585 commented Aug 30, 2016 •

edited

Loading