Flag id as updateOnly when forceId is in effect #1453

rashmihunt · 2017-08-04T22:43:38Z

@bajtos Please take a look. I will sync up with you on the test regressions.

Other PRs involved in the solution of issue strongloop/loopback#2924

Changes in this PR

Added updatedOnly in memory flag for generated ID in model-builder.js. I decided to go with property level setting as you suggested rather than top level setting updateOnly= [ ]
Modified an existing test case to cover this.
forceID is set back to settings[] in datasource.js which can now correctly be used in all parts of the code
added new test case to check forceId in settings[]
Implemented getUpdateOnlyProperties() in model.js
Added test case for this
6 existing test cases are failing due to the fact the fact that forceId now being 'true' for generated id cases unless user has passed in forceID = false while creating the model,

In dao.js

  var forceId = this.settings.forceId;
  if (forceId) {
    options = Object.create(options);
    options.validate = !!doValidate;
    if (doValidate) {
      Model.findById(id, options, function(err, model) {
        if (err) return cb(err);
        if (!model) return cb(errorModelNotFound(id));
        model.updateAttributes(data, options, cb);
      });
    } else {
      const model = new Model({id: id}, {persisted: true});
      model.updateAttributes(data, options, cb);
    }
    return cb.promise;
  }

if forceId is true in most failing test cases and doValidate is false, which means it ends up in

else {
      const model = new Model({id: id}, {persisted: true});
      model.updateAttributes(data, options, cb);
    }

which creates a new model and with the new set of attributes passed in. In one case
validateUpsert is calculated and set into 'doValidate' in logic prior to above block of code. Looking through the documentation and closed issues in 3.x , I noticed that in 3.x, validateUpsert is changed to 'true' by default, which means 'undefined' should be considered as 'true' ? Should there be logic just before - if (forceId) {..}

  if (doValidate === undefined) {
    doValidate = true;
  }

Anyway, I will sync up with you on Monday morning on this.

bajtos

Good start! Please take a look at my comments below.

bajtos · 2017-08-07T06:50:47Z

lib/model-builder.js

-    ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true});
+    // updateOnly property is added to indicate that this property will appear in the model for update/updateorcreate
+    // operations but and not for create operation.
+    ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true, updateOnly: true});


I think updateOnly should be set only when forceId is in effect. With forceId:false, clients can provide custom id value even when the id column was injected by the datasource.

I think updateOnly flag should be set in lib/datasource.js:

if (forceId) { modelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'}); // set updateOnly here // something along the following lines, I am not sure if we need to change rawProperties too modelClass.definition.properties[idName].updateOnly = true; }

@bajtos We can't set updateOnly property in the above place, since LoopBack repository's PersistentModel.setupRemoting() --> getUpdateOnlyProperties() gets invoked before above logic in datasource-gluggler's datasource.js code gets executed. Since getUpdateOnlyProperties() gets invoked before setting 'updateOnly' model as you suggested above, getUpdateOnlyProperties() will not return id property even if forceId is true. Any other parts of the logic we can set this property?

@bajtos As alternate approach, I tried this in model builder

// Add the id property if (idInjection) { // Set up the id property // updateOnly property is added to indicate that this property will appear in the model for update related operations // but and not for create operation. if (ModelClass.definition.settings.forceId !== false) { ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true, updateOnly: true}); } else { ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true}); } } }

Unfortunately, this doesn't seem to work either if user explicitly sets forceId = false in model file, for e.g book.json file. ModelClass.definition.settings.forceId logic seems to look at the base model's (Model) setting rather than the book model's setting.

Unfortunately, this doesn't seem to work either if user explicitly sets forceId = false in model file, for e.g book.json file. ModelClass.definition.settings.forceId logic seems to look at the base model's (Model) setting rather than the book model's setting.

I think you should be using ModelClass.settings.forceId here.

There are two things I am concerned about:

We should keep the logic deciding how to treatforceId: undefined in a single place.

When forceId is effectively false, then the id property should not be marked as updateOnly.

I did not realize that setupDataAccess is called too late in our setup process. In that case, I am proposing to move all logic determining forceId from setupDataAccess to this method (ModelBuilder.prototype.define). I am talking about the following lines:

if (idProp.generated && forceId !== false) { forceId = true; } // set the calculated forceId back to settings settings.forceId = forceId;

I believe we should have all required information already available here. The only thing that can change in setupDataAccess is the type of the id property, but that is not affecting forceId (at least I think so).

Thoughts?

Agree ^^

I tried

var forceId = ModelClass.settings.forceId; if (idNames.length > 0) { // id already exists idInjection = false; // Does method `idName` also works for composite Id at this phase? var idName = modelDefinition.idName(); var idProp = ModelClass.definition.rawProperties[idName]; if (idProp.generated && forceId !== false) { forceId = true; } } else { if (forceId !== false) forceId = true; } ModelClass.settings.forceId = forceId;

received same errors as @rashmihunt is fixing in this PR, other tests pass.

@bajtos what you are proposing is to move only the forceId determining logic from setUpDataAccess and leave rest of the logic as is.. for e.g setting idType and calling modelClass.validatesAbsenceOf(..) based on
forceId from the settings.

DataSource.prototype.setupDataAccess = function(modelClass, settings) { if (this.connector) { // Check if the id property should be generated var idName = modelClass.definition.idName(); var idProp = modelClass.definition.rawProperties[idName]; if (idProp && idProp.generated && this.connector.getDefaultIdType) { // Set the default id type from connector's ability var idType = this.connector.getDefaultIdType() || String; idProp.type = idType; modelClass.definition.rawProperties[idName].type = idType; modelClass.definition.properties[idName].type = idType; var forceId = settings.forceId; if (forceId) { modelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'}); } } .... }

@bajtos I did try your proposed solution and it does work.

what you are proposing is to move only the forceId determining logic from setUpDataAccess and leave rest of the logic as is.. for e.g setting idType and calling modelClass.validatesAbsenceOf(..) based on
forceId from the settings.

I did try your proposed solution and it does work.

Cool!

I think it probably makes sense to move modelClass.validatesAbsenceOf over too, while we are at cleaning up this code. Having said that, what you are proposing in your comment above looks reasonable too.

@bajtos I saw lot of tests failed in CI in downstream component 'loopback' if calculated forceId is set back into model class's settings[] in mode-builder.define (..) method instead of datasource.setupDataAccess(). See in this commit
bb9be2d#diff-546e03515f85748fe5b4d0db11cd81b7R355

I am bit puzzled what's the difference between setting it in model-builder.js vs datasource.js.

To avoid the CI failures, I reverted the refactoring of model-builder.js and datasource.js with my latest commit. PTAL.

bajtos · 2017-08-07T06:52:36Z

lib/model.js

+  var property;
+  var updateOnlyProps = [];
+
+  for (var key in props) {


Let's use Array.prototype.filter to simplify this code.

const props = this.definition.properties; return Object.key(props).filter(key => props[key].updateOnly);

bajtos · 2017-08-07T06:56:54Z

test/loopback-dl.test.js


+    // check if method getUpdateOnlyProperties exist in ModelClass and check if the Post has 'id' in updateOnlyProperties list


One assert per test please. There should be two tests: one checking Post.settings.forceId, the other verifying getUpdateOnlyProperties(). We may want to write more tests for getUpdateOnlyProperties in the future, it's probably a good idea to create a new describe block for tests of this method.

@bajtos done

bajtos · 2017-08-07T06:57:45Z

test/loopback-dl.test.js

+    // check if method getUpdateOnlyProperties exist in ModelClass and check if the Post has 'id' in updateOnlyProperties list
+    Post.should.have.property('getUpdateOnlyProperties');
+    var updateOnlyProps = Post.getUpdateOnlyProperties();
+    updateOnlyProps.should.containEql('id');


I think in this test we know exactly what properties should be marked as updateOnly, don't we? In which case I think it's better to assert the exact content of the returned array:

updateOnlyProps.should.eql(['id']);

@bajtos done PTAL

bajtos · 2017-08-07T07:00:51Z

test/default-scope.test.js

@@ -208,7 +208,7 @@ describe('default scope', function() {
      var data = {id: ids.productA, description: 'Anything...', kind: 'ingored'};
      Tool.updateOrCreate(data, function(err, p) {
        should.not.exist(err);
-        p.name.should.equal('Product A');
+        // p.name.should.equal('Product A'); // TODO [rashmi] Sync up with Miroslav about this. propertyly setting the forceId back to settings has this side affect


What are the side effects? Could you post the outcome of the test here please?

Please ignore my comment, I see you have already analyzed the issue and posted learning in the pull request description 👍

bajtos · 2017-08-07T10:49:40Z

@rashmihunt I pushed two commits to fix the test failures you have discovered - see d6716ae and 023439b

bajtos · 2017-08-07T10:51:02Z

I am cross-posting the commit message from d6716ae for posterity:

The contract of updateOrCreate is expecting a full object instance to be passed to the callback.

The current implementation was creating an empty instance and calling updateAttributes under the hood. As a result, the callback was called with the attributes being updated only.

In order to preserve existing behaviour, we have to always build a full initial instance by calling findById.

See the following discussion for more context: #966

jannyHou · 2017-08-07T21:52:07Z

I feel _rev in cloudant connector probably needs this feature too, and I will review it asap, thank you @rashmihunt

bajtos · 2017-08-08T07:22:42Z

@jannyHou

I feel _rev in cloudant connector probably needs this feature too, and I will review it asap, thank you

Yes, I was keeping the use case of _rev property in mind while guiding @rashmihunt's work on these changes.

@rashmihunt would you mind splitting this pull request into two - one fixing updateOrCreate for forceId === undefined, another adding updateOnly? It will make it easier for us in the future to understand the context of the changes being made and distinguish between changes needed for updateOnly from changes needed to fix forceId.

jannyHou

@rashmihunt The changes look reasonable to me, just one same comment as Miroslav posts in https://github.com/strongloop/loopback-datasource-juggler/pull/1453/files#r131837163, I did a quick try, seems like a good solution.

rashmihunt · 2017-08-19T00:44:42Z

@slnode test please

jannyHou · 2017-08-21T20:00:43Z

Cloudant fails with same reason in loopbackio/loopback-connector-cloudant#161 (comment)

There is a big refactor happen in that repo and two PRs block each other, so I have to check in one(with the failure) then the other one(contains the fix), it's not caused by this PR.

The contract of `updateOrCreate` is expecting a full object instance to be passed to the callback. The current implementation was creating an empty instance and calling updateAttributes under the hood. As a result, the callback was called with the attributes being updated only. In order to preserve existing behaviour, we have to always build a full initial instance by calling `findById`. See the following discussion for more context: #966

rashmihunt · 2017-08-22T18:48:44Z

@slnode test please

* Flag id as updateOnly when forceId is in effect (#1453) (Rashmi Hunt) * Add stalebot configuration (Kevin Delisle)

simonbrunel · 2017-10-16T12:25:37Z

lib/model-builder.js

+    }
+
+    if (forceId) {
+      ModelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'});


@rashmihunt @bajtos shouldn't we also reject any requests containing an empty string ID when forceId is true? validatesAbsenceOf considers an empty string value valid, which generates a record with no id (at least with MongoDB) when calling a POST endpoint with an explicit empty string ID even if forceId is true (I'm new to LoopBack, so I might have missed something).

For posterity, I responded here: #1519 (comment)

rashmihunt requested a review from bajtos August 4, 2017 22:43

rashmihunt requested review from b-admike, dhmlau, jannyHou, kjdelisle, loay, ssh24 and virkt25 as code owners August 4, 2017 22:43

rashmihunt added the #review label Aug 4, 2017

0candy added the needs-priority label Aug 4, 2017

rashmihunt removed request for loay, kjdelisle, virkt25, jannyHou, b-admike, ssh24 and dhmlau August 4, 2017 22:44

This was referenced Aug 5, 2017

Support updateOnly feature strongloop/loopback-swagger#92

Merged

Support createOnlyInstance in model strongloop/loopback#3548

Merged

bajtos suggested changes Aug 7, 2017

View reviewed changes

bajtos mentioned this pull request Aug 8, 2017

Fix forceId #1454

Closed

bajtos requested a review from jannyHou August 8, 2017 07:19

jannyHou reviewed Aug 9, 2017

View reviewed changes

jannyHou mentioned this pull request Aug 10, 2017

Replace doesn't work with forceId: true loopbackio/loopback-connector-couchdb2#13

Closed

rashmihunt mentioned this pull request Aug 12, 2017

Test case change for updateOnly property strongloop/loopback-sdk-angular#276

Merged

rashmihunt changed the title ~~WIP: updateOnly, forceId changes for https://github.com/strongloop/loopback/issues/2924~~ updateOnly, forceId changes for https://github.com/strongloop/loopback/issues/2924 Aug 12, 2017

rashmihunt force-pushed the updateonly_feature branch 3 times, most recently from 9b11f61 to 927e209 Compare August 21, 2017 19:03

rashmihunt and others added 14 commits August 22, 2017 09:26

updateOnly, forceId changes

7459aad

support getUpdateOnlyProperties

00b6b7a

fixup! fix tests of upsert validation

afc62a0

move forceId to model-builder

9258f5b

remove TODO comment

6ce125f

revert refactoring and test fixes

2467ba0

Remove duplicate test

f091fab

change testcase names

c8867a8

change to ModeClass.settingse

4e5dd2e

forceId setup from datasource to model-builder

e75887e

fix inheritance of auto-generated forceId

342fb27

Fixed failing tests for auto change

52a1545

fixed a comment

f012c09

rashmihunt force-pushed the updateonly_feature branch from 927e209 to f012c09 Compare August 22, 2017 16:27

rashmihunt merged commit 6c6df15 into master Aug 22, 2017

rashmihunt removed the #review label Aug 22, 2017

0candy removed the needs-priority label Aug 22, 2017

rashmihunt deleted the updateonly_feature branch August 22, 2017 20:09

rashmihunt added a commit that referenced this pull request Aug 22, 2017

3.11.0

1fa89d6

* Flag id as updateOnly when forceId is in effect (#1453) (Rashmi Hunt) * Add stalebot configuration (Kevin Delisle)

rashmihunt mentioned this pull request Aug 23, 2017

Incorrect swagger definition for models with forceId:true strongloop/loopback#2924

Closed

simonbrunel reviewed Oct 16, 2017

View reviewed changes

simonbrunel mentioned this pull request Nov 6, 2017

[BUG] Empty string id is not correctly validated by forceId #1519

Open

bajtos mentioned this pull request Apr 26, 2018

OpenAPI decorator does not properly generate schemas of type Partial loopbackio/loopback-next#1179

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flag id as updateOnly when forceId is in effect #1453

Flag id as updateOnly when forceId is in effect #1453

rashmihunt commented Aug 4, 2017 •

edited

Loading

bajtos left a comment

bajtos Aug 7, 2017

rashmihunt Aug 7, 2017

rashmihunt Aug 8, 2017 •

edited

Loading

bajtos Aug 8, 2017

jannyHou Aug 9, 2017 •

edited

Loading

rashmihunt Aug 9, 2017 •

edited

Loading

rashmihunt Aug 9, 2017 •

edited

Loading

bajtos Aug 10, 2017

rashmihunt Aug 11, 2017 •

edited

Loading

bajtos Aug 7, 2017

bajtos Aug 7, 2017

rashmihunt Aug 11, 2017

bajtos Aug 7, 2017

rashmihunt Aug 11, 2017

bajtos Aug 7, 2017

bajtos Aug 7, 2017

bajtos commented Aug 7, 2017

bajtos commented Aug 7, 2017

jannyHou commented Aug 7, 2017

bajtos commented Aug 8, 2017

jannyHou left a comment

rashmihunt commented Aug 19, 2017

jannyHou commented Aug 21, 2017

rashmihunt commented Aug 22, 2017

simonbrunel Oct 16, 2017

bajtos Feb 9, 2018


		// check if method getUpdateOnlyProperties exist in ModelClass and check if the Post has 'id' in updateOnlyProperties list

Flag id as updateOnly when forceId is in effect #1453

Flag id as updateOnly when forceId is in effect #1453

Conversation

rashmihunt commented Aug 4, 2017 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmihunt Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

rashmihunt Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

rashmihunt Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmihunt Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 7, 2017

bajtos commented Aug 7, 2017

jannyHou commented Aug 7, 2017

bajtos commented Aug 8, 2017

jannyHou left a comment

Choose a reason for hiding this comment

rashmihunt commented Aug 19, 2017

jannyHou commented Aug 21, 2017

rashmihunt commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmihunt commented Aug 4, 2017 •

edited

Loading

rashmihunt Aug 8, 2017 •

edited

Loading

jannyHou Aug 9, 2017 •

edited

Loading

rashmihunt Aug 9, 2017 •

edited

Loading

rashmihunt Aug 9, 2017 •

edited

Loading

rashmihunt Aug 11, 2017 •

edited

Loading