Support updateOnly feature #92

rashmihunt · 2017-08-05T02:00:44Z

This is part of solution forhttps://github.com/strongloop/loopback/issues/2924

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

@bajtos Please take a initial look.

This PR changes include

check for createOnlyInstance in schema-builder.js to use a new model for create only
defining and setting separate model for create operation based on updateOnly props - changes in model-helper.js
tests cases

bajtos

Thank you for the pull request! The changes looks mostly good, I would like you to clean the code a bit.

More importantly, please add a test (or more) to cover this new feature.

bajtos · 2017-08-07T06:39:11Z

lib/specgen/model-helper.js

+
+    // check if ModelClass has getUpdateOnlyProperties() function defined to avoid version issues
+    if (lbdef.modelBuilder && lbdef.modelBuilder.defaultModelBaseClass &&
+        lbdef.modelBuilder.defaultModelBaseClass.getUpdateOnlyProperties) {


This is getting update-only properties from the default base model, which usually don't contain any, if my understanding is correct. I think you should be calling modelCtor.getUpdateOnlyProperties() instead. Thoughts?

hmm I added getUpdateOnlyProperties() at the end of last checkins and tested using local mocha test and above code does work in mocha test.. I re-tried it with end to end test creating app using lb app and using api -explorer and I do see that, I need to use modelCtor.getUpdateOnlyProperties() instead.

bajtos · 2017-08-07T06:39:32Z

lib/specgen/model-helper.js

+      var updateOnlyProps =
+          lbdef.modelBuilder.defaultModelBaseClass.getUpdateOnlyProperties();
+      // if at least one updateOnly property is found, we need to create another model $new_xyz to be used only for
+      // create operation and this model will not have the updateOnly property included in the model. For e.g generated "id" field


Please wrap the lines at 80 chars

bajtos · 2017-08-07T06:41:14Z

lib/specgen/model-helper.js

@@ -50,7 +65,7 @@ var modelHelper = module.exports = {
  },
 };

-var definitionFunction = function(modelCtor, typeRegistry) {
+var definitionFunction = function(modelCtor, typeRegistry, name, updateOnlyProps) {


I am proposing to add a single argument options to keep the number of arguments under control. This new argument can expect two properties for the starter - options.name and options.updateOnlyProps.

bajtos · 2017-08-07T06:44:13Z

lib/specgen/model-helper.js

+    // if model has updateOnly properties, check if current property we are trying to add is updateOnly.
+    // If yes, skip adding to the model since this model is used for create operation where generated/update
+    // only property should not exist
+    if (updateOnlyProps) {


I think the definition function should not need to know that the skipped properties are updateOnly. In the future, we may want to skip other kinds of properties, e.g. createOnly. Let's rename updateOnlyProps to a more generic excludeProps.

I also think this newly added block can be simplified a bit:

if (excludeProps && excludeProps.includes(key)) { return; } swaggerDef.properties[key] = schema;

rashmihunt · 2017-08-16T01:22:38Z

@bajtos PTAL

Incorporated code review comments
Added new tests

bajtos

Looks mostly good, let's clean up the code a bit more please.

bajtos · 2017-08-16T12:16:55Z

lib/specgen/model-helper.js

+    // check if ModelClass has defined getUpdateOnlyProperties() function to
+    // avoid version issues
+    if (modelCtor.getUpdateOnlyProperties) {
+      var excludeProps = modelCtor.getUpdateOnlyProperties();


const excludeProps = ...

bajtos · 2017-08-16T12:17:19Z

lib/specgen/model-helper.js

+      if (excludeProps && excludeProps.length > 0) {
+        const modelName = '$new_' + name;
+        typeRegistry.registerModel(modelName, function() {
+          var options = {


use const

we don't quote property keys unless they contain characters like space or a dot

In ES6+, a property name can be excluded when the value is set to a variable with the same name.

const options = {excludeProps}; return definitionFunction(modelCtor, typeRegistry, options); // or even return definitionFunction(modelCtor, typeRegistry, {excludeProps});

bajtos · 2017-08-16T12:23:19Z

test/specgen/swagger-spec-generator.test.js

+    });
+  });
+
+  function createLoopbackAppWithModelWithForceId(forceId) {


Instead of copy-pasting most of createLoopbackAppWithModel, I think it will be better to add a parameter to createLoopbackAppWithModel.

This is how I envision the new usage:

// replacement for createLoopbackAppWithModel('/custom-api-root'); createLoopbackAppWithModel({apiRoot: '/custom-api-root'}); // in your new tests createLoopbackAppWithModel({modelOptions.forceId: true});

bajtos · 2017-08-16T12:24:34Z

Also please check why the CI builds are failing.

rashmihunt · 2017-08-19T00:38:34Z

@bajtos PTAL.

CI will fail until upstream changes in juggler and loopback are merged. 3 failing tests require these changes since these tests check if there is a new model created or not for create operation.

bajtos

LGTM with two more suggestions. No further review is necessary, at least as far as I am concerned.

bajtos · 2017-08-21T10:06:26Z

test/specgen/swagger-spec-generator.test.js

+          .to.include.members(['Product']);
+    });
+
+    it('should use $new_Product definition for post/create operation', function() {


Please add "when forceId is in effect" and "when forceId is false" to the names of this and the next test. I found it confusing why we say "should use $new_Product" and then "should use Product" without explaining the difference in the tested scenario.

bajtos · 2017-08-21T10:08:05Z

test/specgen/swagger-spec-generator.test.js

+    if (options === undefined || options.forceId === undefined) {
+      modelSettings = {description: ['a-description', 'line2']};
+    } else {
+      modelSettings = {description: ['a-description', 'line2'], forceId: options.forceId};


DRY

const modelSettings = {description: ['a-description', 'line2']}; if (options && options.forceId !== undefined) { modelSettings.forceId = options.forceId; }

bajtos

Oops, I wanted to approve in my comment above.

rashmihunt · 2017-08-23T00:18:18Z

@slnode test please

* Support updateOnly feature (#92) (Rashmi Hunt) * Add stalebot configuration (Kevin Delisle) * Update Issue and PR Templates (#95) (Sakib Hasan)

rashmihunt requested review from bajtos and STRML as code owners August 5, 2017 02:00

rashmihunt added the #review label Aug 5, 2017

rashmihunt removed the request for review from STRML August 5, 2017 02:00

This was referenced Aug 5, 2017

Support createOnlyInstance in model strongloop/loopback#3548

Merged

Flag id as updateOnly when forceId is in effect loopbackio/loopback-datasource-juggler#1453

Merged

bajtos suggested changes Aug 7, 2017

View reviewed changes

bajtos suggested changes Aug 16, 2017

View reviewed changes

rashmihunt closed this Aug 19, 2017

rashmihunt reopened this Aug 19, 2017

rashmihunt added #review and removed #review labels Aug 19, 2017

bajtos suggested changes Aug 21, 2017

View reviewed changes

bajtos approved these changes Aug 21, 2017

View reviewed changes

rashmihunt added 4 commits August 22, 2017 17:34

support updateOnly feature

ce5e61b

code review changes & new tests

e777326

Address review comments

1d989d5

test fix - code review

6c64c6b

rashmihunt force-pushed the updateonly_feature branch from dba4a26 to 6c64c6b Compare August 23, 2017 00:35

use lodash includes for node4

7f738c9

rashmihunt merged commit ae196d6 into master Aug 23, 2017

rashmihunt deleted the updateonly_feature branch August 23, 2017 04:49

rashmihunt removed the #review label Aug 23, 2017

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

4.2.0

58b6edf

* Support updateOnly feature (#92) (Rashmi Hunt) * Add stalebot configuration (Kevin Delisle) * Update Issue and PR Templates (#95) (Sakib Hasan)

rashmihunt mentioned this pull request Aug 23, 2017

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

Closed

rashmihunt mentioned this pull request Sep 5, 2017

Deprecate [email protected] and release 5.0.0 #100

Closed

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

Support updateOnly feature #92

Support updateOnly feature #92

rashmihunt commented Aug 5, 2017 •

edited

Loading

bajtos left a comment

bajtos Aug 7, 2017

rashmihunt Aug 9, 2017

bajtos Aug 7, 2017

bajtos Aug 7, 2017

bajtos Aug 7, 2017

rashmihunt commented Aug 16, 2017

bajtos left a comment

bajtos Aug 16, 2017

bajtos Aug 16, 2017

bajtos Aug 16, 2017

bajtos commented Aug 16, 2017

rashmihunt commented Aug 19, 2017 •

edited

Loading

bajtos left a comment

bajtos Aug 21, 2017

bajtos Aug 21, 2017

bajtos left a comment

rashmihunt commented Aug 23, 2017

Support updateOnly feature #92

Support updateOnly feature #92

Conversation

rashmihunt commented Aug 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmihunt commented Aug 16, 2017

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

Choose a reason for hiding this comment

bajtos commented Aug 16, 2017

rashmihunt commented Aug 19, 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

bajtos left a comment

Choose a reason for hiding this comment

rashmihunt commented Aug 23, 2017

rashmihunt commented Aug 5, 2017 •

edited

Loading

rashmihunt commented Aug 19, 2017 •

edited

Loading