upsertWithWhere method by mountain1234585 · Pull Request #1001 · loopbackio/loopback-datasource-juggler

mountain1234585 · 2016-07-13T18:09:55Z

Added a new method for performing upsertWithWhere in dao.js

mountain1234585 · 2016-07-13T18:15:07Z

@raymondfeng @Amir-61 : Please consider this pull request. I believe following things are pending from the merge : !) Test case addition 2) All tests passing Please let me know if anything else is pending for the merge to happen.

I have already started to write the tests. Should be able to commit by tomorrow.

Also, I have already signed the CLA and when I click on details, it says you have signed the CLA. I was wondering why is it saying not all contributors have signed the CLA. This is a fresh fork with a fresh commit and a new pull request.

Reference : I have incorporated the following review comments from @raymondfeng and @Amir-61 :
@raymondfeng

why we don't update multiple instances rather return error if multiple instances are found.
This was discussed as part of the requirement we had and we plan to restrict this only for single instance update. I had a look at how upsert with foreign key is performed in some of the popularly used endpoints and they follow the similar approach.
why the key value pair mentioned in the "where" is merged into data
In case of create, we want the field to be updated when a new record is created. And similar is seen in upsert operation with Salesforce as well.
method signature is wrong in memory.js
I have now removed the method in memory.js since the entire logic is handled in dao.js
too many empty lines in dao.js
I have removed them now.
5)wrapped the callback with process.nextTick
Made the changes with the latest commit now

@Amir-61
resolved the eslint errors , Signed the CLA

I wanted to get these changes merged soon since we have a blocking , high priority requirement. Request your support and guidance in doing the same.

mountain1234585 · 2016-07-13T18:29:25Z

Follow up from : #995

Amir-61 · 2016-07-13T18:38:39Z

@mountain1234585

I'm confused why creating another PR, while you had another open PR#995.

So IIUC you are making the scope of this work smaller by proposing to skip the atomic implementation for connectors including memory connector as well as skipping operation hooks? This may lead to inconsistency between our approach for other methods; with that being said, I leave the decision to @raymondfeng @bajtos.

mountain1234585 · 2016-07-13T19:00:29Z

@Amir-61
I had got too many conflicts in that PR, while trying to do a rebase and sync. Hence created a new fork and PR, not to mess it up anymore. However, I have retained the comments I had received after you and Raymond had reviewed in the comment above so that it comes handy.
This implementation is as per the requirement we have which I have discussed with Raymond Feng earlier . I have tested these changes for memory connector, salesforce and sapodata (our inhouse connector) and it works perfectly fine. Also, I have ensured, for connectors who want to use their native endpoint APIs , can provide their own implementation in the connector runtime code and those who want to fallback to the default implementation can do so. As you suggested yesterday, I have started writing the tests. Please let me know if you have any more questions.

bajtos · 2016-07-14T14:09:19Z

method signature is wrong in memory.js
I have now removed the method in memory.js since the entire logic is handled in dao.js

It's important to provide an atomic implementation in the memory connector. The current implementation, which executes multiple database commands, is prone to race conditions. (Consider what happens when two requests to upsert the same record are handled at the same time in parallel.)

skipping operation hooks

The promise of operation hooks is that regardless of the operation invoked, the hook observers will be always executed. I.e. if a user installs "before save" observer, they can be confident their handler will be always invoked before any change is being persisted.

I am not comfortable breaking that promise. I would much rather see operation hooks implemented in as part of this patch.

It's ok to keep this work incremental though. You can continue adding unit-tests for the functionality already there, and leave implementation of operation hooks only after the other known issues are resolved.

bajtos · 2016-07-14T14:10:43Z

+ . This is a PUT operation and based on
+ * non-primary key. The filter takes one key value pair and updates single matching instance.*
+ */
+DataAccessObject.upsertWithWhere = function(where, data, options, cb) {


In longer term, we want to promote patchOrCreate instead of upsert/updateOrCreate. Can you please add an alias patchOrCreateWithWhere = upsertWithWhere? See how other aliases in this file are set up.

mountain1234585 · 2016-07-14T14:39:21Z

@bajtos
I will look into the operation hooks as per your suggestion and will revert if I need help. Thanks for the review.

bajtos · 2016-07-14T14:46:10Z

@slnode ok to test

mountain1234585 · 2016-07-14T15:06:21Z

@bajtos .. one clarification.. so will it be ok to merge after resolving the comments you have suggested to the current implementation and along with unit tests passing. And then incrementally add on these operation hooks soon after the first merge. Or, do we need to have everything together for the first merge.

bajtos · 2016-07-14T15:29:45Z

so will it be ok to merge after resolving the comments you have suggested to the current implementation and along with unit tests passing. And then incrementally add on these operation hooks soon after the first merge. Or, do we need to have everything together for the first merge.

I would prefer to have everything together for the first merge.

mountain1234585 · 2016-07-14T17:50:08Z

@bajtos
I have now implemented the operation hooks and did a quick test with two scenarios :

Connector has its own implementation
2)Connector want to use the default implementation.
It worked as expected. I will go ahead and make other modifications as per your comments and commit the changes to PR for your review.

mountain1234585 · 2016-07-15T11:33:38Z

@bajtos
I have addressed all your comments except having the implementation in memory.js, which I am working on currently. Please review the other changes I have done and let me know your comments. Thanks.

bajtos · 2016-07-15T12:56:51Z

-
  var pk = idName(Model);
  if (keys[0] !== pk) return false;
-


unrelated change, please remove. same applies for the removed line above.

mountain1234585 · 2016-08-10T20:36:48Z

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

Amir-61 · 2016-08-12T17:24:47Z

+            hookState: ctx.hookState,
+            options: options,
+          };
+          Model.notifyObserversOf('loaded', contxt, function(err) {


ummm not sure why you could not use context as opposed to contxt; probably it conflicts the name. How about using ctx instead of contxt which is missing e

we are making use of ctx.hookstate , which is defined before we define the context properties for 'loaded'. Using ctx instead will make ctx.hookState undefined. Hence, I have left the variable name as contxt .

Amir-61 · 2016-08-12T18:09:56Z

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

Thanks! I left a few minor comments which should be pretty easy to address; the code looks almost good to me. I leave the final approval to @bajtos.

@superkhau also you may look into this patch as well before @bajtos goes for final approval.

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

@raymondfeng @bajtos I've looked at this PR enough and I am about as confident as I can be. Gonna need your opinions here for final LGTM.

Amir-61 · 2016-08-12T19:50:26Z

LGTM from my side after addressing my minor comments. Final LGTM needs to be either from @bajtos or @raymondfeng

bajtos · 2016-08-15T14:42:50Z

I took a quick look and did not see any obvious problems. Let's get this landed, we can always fix bugs later when they are discovered by a real user.

Amir-61 · 2016-08-16T15:51:16Z

@mountain1234585 Thanks for the contribution! I landed it! 🚢

Amir-61 · 2016-08-16T15:55:01Z

I will back-port it 2.x as well

mountain1234585 · 2016-08-16T16:04:37Z

Thankyou @Amir-61 @bajtos @superkhau @rmg @raymondfeng : For your valuable feedback and the reviews on this PR. I had a great learning !

Backport from #1001

Amir-61 · 2016-08-16T16:14:28Z

Backport PR #1052

Amir-61 · 2016-08-16T16:15:13Z

Thankyou @Amir-61 @bajtos @superkhau @rmg @raymondfeng : For your valuable feedback and the reviews on this PR. I had a great learning !

No problem. My pleasure!

Backport from #1001

crandmck added the #community contribution label Jul 13, 2016

mountain1234585 mentioned this pull request Jul 13, 2016

changes to dao.js and memory.js to add a new method on Upsert based on non-primary key #995

Closed

Amir-61 assigned Amir-61 and unassigned Amir-61 Jul 13, 2016

bajtos reviewed Jul 14, 2016
View reviewed changes

bajtos reviewed Jul 15, 2016
View reviewed changes

mountain1234585 force-pushed the upsertNPK branch from 3ec1f5e to 1cfddb6 Compare August 10, 2016 20:32

Amir-61 reviewed Aug 12, 2016
View reviewed changes

Amir-61 assigned bajtos Aug 12, 2016

upsertWithWhere feature support in juggler DAO

37541dd

mountain1234585 force-pushed the upsertNPK branch from 1cfddb6 to 37541dd Compare August 16, 2016 12:37

Amir-61 merged commit 56aeeeb into loopbackio:master Aug 16, 2016

Amir-61 removed the #review label Aug 16, 2016

Amir-61 mentioned this pull request Aug 16, 2016

upsertWithWhere feature support in juggler DAO [2.x] #1052

Merged

Amir-61 pushed a commit that referenced this pull request Aug 16, 2016

upsertWithWhere feature support in juggler DAO

87dab69

Backport from #1001

Amir-61 pushed a commit that referenced this pull request Aug 23, 2016

upsertWithWhere feature support in juggler DAO

0d36eb5

Backport from #1001

Amir-61 pushed a commit that referenced this pull request Aug 29, 2016

upsertWithWhere feature support in juggler DAO

27b27b1

Backport from #1001

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

upsertWithWhere feature support in juggler DAO

44e1d29

Backport from #1001

Uh oh!

Conversation

mountain1234585 commented Jul 13, 2016 • edited by Amir-61 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mountain1234585 commented Jul 13, 2016

Uh oh!

Amir-61 commented Jul 13, 2016

Uh oh!

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

Uh oh!

Uh oh!

bajtos commented Jul 14, 2016

Uh oh!

bajtos Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

mountain1234585 Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

mountain1234585 commented Jul 14, 2016

Uh oh!

bajtos commented Jul 14, 2016

Uh oh!

mountain1234585 commented Jul 14, 2016

Uh oh!

bajtos commented Jul 14, 2016

Uh oh!

mountain1234585 commented Jul 14, 2016

Uh oh!

mountain1234585 commented Jul 15, 2016

Uh oh!

bajtos Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Amir-61 Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

mountain1234585 Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

Amir-61 commented Aug 12, 2016

Uh oh!

superkhau commented Aug 12, 2016

Uh oh!

Amir-61 commented Aug 12, 2016

Uh oh!

bajtos commented Aug 15, 2016

Uh oh!

Amir-61 commented Aug 16, 2016

Uh oh!

Amir-61 commented Aug 16, 2016

Uh oh!

mountain1234585 commented Aug 16, 2016

Uh oh!

Amir-61 commented Aug 16, 2016

Uh oh!

Amir-61 commented Aug 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mountain1234585 commented Jul 13, 2016 •

edited by Amir-61

Loading

mountain1234585 commented Jul 13, 2016 •

edited

Loading

mountain1234585 commented Jul 13, 2016 •

edited

Loading

mountain1234585 commented Aug 10, 2016 •

edited

Loading