Do not apply default values on data from database [3.x] #1705

bajtos · 2019-04-04T08:26:33Z

Before this change, when a property was configured with a default value at LoopBack side and the database was returned a record with a missing value for such property, then we would supply use the configured default.

This behavior is problematic for reasons explained in #1692.

In this patch, we are introducing a new model-level setting called applyDefaultsOnReads, which is enabled by default for backwards compatibility.

When this setting is set to false, operations like find and findOrCreate will NOT apply default property values on data returned by the database (connector).

Please note that most of the other CRUD methods did not apply default values on database data as long as the connector provided native implementation of the operation, that aspect is not changing.

Also note that default values are applied only on properties with undefined values. The value null does not trigger application of default values. This is important because SQL connectors return null for properties with no value set.

Related issues

This is a back-port of #1704. Compared to #1704 (master/4.x), we are preserving backwards compatibility by introducing a new feature flag.

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

bajtos · 2019-04-04T09:21:54Z

[cis-jenkins] downstream: loopback-connector-mongodb@master — Failed! (be1c365)

The failure seems unrelated, see https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-mongodb%7Emaster/35/console

[cis-jenkins] downstream: loopback-connector-mysql@master — Failed! (be1c365)

This is a valid failure. Because the new tests don't run autoupdate/automigrate, the database schema is not matching model schema. I'll investigate.

bajtos · 2019-04-05T12:17:57Z

In 6e7aee4, I added automigrate calls that fixed failing tests for SQL connectors.

However, there are still few failures that look relevant to me.

@jannyHou could you please help me to investigate the Cloudant failure?

Cloudant
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-cloudant%7Emaster/40/console

3) cloudant imported features basic-querying find applies default values by default:
07:50:37      TypeError: Cannot read property 'use' of undefined
07:50:37       at Cloudant.CouchDB.selectModel (node_modules/loopback-connector-couchdb2/lib/couchdb.js:750:29)
07:50:37       at Cloudant.destroyAll (node_modules/loopback-connector-couchdb2/lib/couchdb.js:423:17)

4) cloudant imported features basic-querying find preserves empty values from the database when "applyDefaultsOnReads" is false:
07:50:37      TypeError: Cannot read property 'use' of undefined
07:50:37       at Cloudant.CouchDB.selectModel (node_modules/loopback-connector-couchdb2/lib/couchdb.js:750:29)
07:50:37       at Cloudant.destroyAll (node_modules/loopback-connector-couchdb2/lib/couchdb.js:423:17)
07:50:37       at node_modules/loopback-connector-couchdb2/lib/migrate.js:42:14

The other two test failures reported by Cloudant seem unrelated, they are reported by CouchDB connector too.

@raymondfeng could you please help me with MSSQL? It looks like a bug in the autoupdate implementation. Other SQL connectors are passing these tests.

mssql
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-mssql%7Emaster/39/console

1) mssql imported features
07:58:29        basic-querying
07:58:29          find
07:58:29            applies default values by default:
07:58:29      RequestError: Could not drop object 'dbo.Product' because it is referenced by a FOREIGN KEY constraint

2) mssql imported features
07:58:29        basic-querying
07:58:29          find
07:58:29            preserves empty values from the database when "applyDefaultsOnReads" is false:
07:58:29      RequestError: Could not drop object 'dbo.Product' because it is referenced by a FOREIGN KEY constraint.

3) mssql imported features
07:58:29        manipulation
07:58:29          findOrCreate
07:58:29            applies default values on returned data:
07:58:29      RequestError: Could not drop object 'dbo.Product' because it is referenced by a FOREIGN KEY constraint.

4) mssql imported features
07:58:29        manipulation
07:58:29          findOrCreate
07:58:29            preserves empty values from the database when "applyDefaultsOnReads" is false:
07:58:29      RequestError: Could not drop object 'dbo.Product' because it is referenced by a FOREIGN KEY constraint.

Other test failures seem unrelated:

dashdb
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-dashdb%7Emaster/42/console

Seems unrelated:

07:48:54 > node pretest.js
07:48:54 
07:48:55 > Seeding the database ...
07:49:55 
07:49:55 /home/jenkins/workspace/ds/loopback-connector-dashdb~master/pretest.js:53
07:49:55   if (err) throw err;
07:49:55            ^
07:49:55 Error: [IBM][CLI Driver] SQL30081N  A communication error has been detected. Communication protocol being used: "TCP/IP".  Communication API being used: "SOCKETS".  Location where the error was detected: "9.30.218.42".  Communication function detecting the error: "selectForConnectTimeout".  Protocol specific error code(s): "115", "*", "*".  SQLSTATE=08001
07:49:55 
07:49:55     at Error (native)

Same error for "db2" .

kv-extreme-scale
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-kv-extreme-scale%7Emaster/40/console

Cannot connect to the testing database :(

mongodb@3.x
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-mongodb%7E3.x/20/console

Fails in lazyConnect and ping test cases, seems unrelated to me.

Similarly for mongodb@master.

rest
https://cis-jenkins.swg-devops.com/job/ds/job/loopback-connector-rest%7Emaster/21/console

Uncaught AssertionError: You must use an API key to authenticate each request to Google Maps Platform APIs. For additional information, please refer to http://g.co/dev/maps-no-account
07:57:27       + expected - actual
07:57:27 
07:57:27       -REQUEST_DENIED
07:57:27       +OK
07:57:27

jannyHou · 2019-04-05T14:17:48Z

Sure I am looking a the Cloudant failure.

Update:

The first commit in loopbackio/loopback-connector-couchdb2#59 fixes the first test case.

Still investigating the second one.

bajtos · 2019-04-08T08:45:47Z

[cis-jenkins] downstream: loopback-connector-mssql@master — Failed! (6e7aee4)
RequestError: Could not drop object 'dbo.Product' because it is referenced by a FOREIGN KEY constraint.

Apparently, there are other tests (or our setup script) using Product model and setting up a foreign key constraint(s) for it. I am not sure why our mssql connector is not dropping those constraints.

Anyhow, I managed to fix the problem by renaming my test model from Product to Player.

bajtos · 2019-04-08T09:09:41Z

[cis-jenkins] downstream: loopback-connector-cloudant@master — Failed! (6e7aee4)
07:50:37 TypeError: Cannot read property 'use' of undefined

I managed to fix this problem with a small tweak in Cloudant's test suite, see loopbackio/loopback-connector-cloudant#202

bajtos · 2019-04-08T09:10:33Z

I consider this pull request as done, I'll clean up the git history before landing.

@jannyHou @raymondfeng please review

test/basic-querying.test.js

hacksparrow

Overall looks good, except for the choice of the data type to test with.

bajtos · 2019-04-09T06:33:58Z

@slnode test please

hacksparrow

All good now.

bajtos · 2019-04-09T11:02:54Z

I had a chat with @hacksparrow and now I understand the problem - I was using a wrong assertion for the tests checking the legacy behavior where default values are applied. I fixed the problem in f498c26.

Actually, it turns out my original test assertions were correct. When the connector returns property value null, we don't consider it to be undefined and thus the default value is not applied. How confusing! I have reverted f498c26 and updated the comments in the tests to make this more clear.

Before this change, when a property was configured with a default value at LoopBack side and the database was returned a record with a missing value for such property, then we would supply use the configured default. This behavior is problematic for reasons explained in #1692. In this commit, we are introducing a new model-level setting called `applyDefaultsOnReads`, which is enabled by default for backwards compatibility. When this setting is set to `false`, operations like `find` and `findOrCreate` will NOT apply default property values on data returned by the database (connector). Please note that most of the other CRUD methods did not apply default values on database data as long as the connector provided native implementation of the operation, that aspect is not changing. Also note that default values are applied only on properties with `undefined` values. The value `null` does not trigger application of default values. This is important because SQL connectors return `null` for properties with no value set.

hacksparrow

LGTM

jannyHou

🚢 LGTM

bajtos added bug major customer labels Apr 4, 2019

bajtos added this to the April 2019 milestone milestone Apr 4, 2019

bajtos self-assigned this Apr 4, 2019

bajtos requested review from raymondfeng, jannyHou and b-admike April 4, 2019 08:26

bajtos requested review from dhmlau, virkt25 and zbarbuto as code owners April 4, 2019 08:26

bajtos mentioned this pull request Apr 8, 2019

Fix test setup of autoupgrade/autoupdate loopbackio/loopback-connector-cloudant#202

Merged

2 tasks

hacksparrow reviewed Apr 8, 2019

View reviewed changes

test/basic-querying.test.js Show resolved Hide resolved

hacksparrow reviewed Apr 8, 2019

View reviewed changes

hacksparrow approved these changes Apr 9, 2019

View reviewed changes

bajtos force-pushed the fix/default-value-in-response-3x branch 2 times, most recently from f91aa80 to a39a2c7 Compare April 9, 2019 10:46

bajtos force-pushed the fix/default-value-in-response-3x branch from a39a2c7 to fd03302 Compare April 9, 2019 11:03

hacksparrow approved these changes Apr 9, 2019

View reviewed changes

bajtos merged commit b8e08b7 into 3.x Apr 9, 2019

bajtos deleted the fix/default-value-in-response-3x branch April 9, 2019 13:30

This was referenced Apr 9, 2019

Do not apply default values on data from database [2.x] #1706

Merged

Improve tests for empty vs. default values #1707

Merged

sharathmuthu6 mentioned this pull request Apr 9, 2019

DataSource juggler is adding the model property with its default value in response #1692

Closed

3 tasks

jannyHou approved these changes Apr 9, 2019

View reviewed changes

dhmlau mentioned this pull request Apr 9, 2019

Remove kv-extreme-scale connector from downstream test #1710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not apply default values on data from database [3.x] #1705

Do not apply default values on data from database [3.x] #1705

bajtos commented Apr 4, 2019 •

edited

Loading

bajtos commented Apr 4, 2019

bajtos commented Apr 5, 2019

jannyHou commented Apr 5, 2019 •

edited

Loading

bajtos commented Apr 8, 2019

bajtos commented Apr 8, 2019

bajtos commented Apr 8, 2019

hacksparrow left a comment

bajtos commented Apr 9, 2019

hacksparrow left a comment

bajtos commented Apr 9, 2019

hacksparrow left a comment

jannyHou left a comment

Do not apply default values on data from database [3.x] #1705

Do not apply default values on data from database [3.x] #1705

Conversation

bajtos commented Apr 4, 2019 • edited Loading

Related issues

Checklist

bajtos commented Apr 4, 2019

bajtos commented Apr 5, 2019

jannyHou commented Apr 5, 2019 • edited Loading

bajtos commented Apr 8, 2019

bajtos commented Apr 8, 2019

bajtos commented Apr 8, 2019

hacksparrow left a comment

Choose a reason for hiding this comment

bajtos commented Apr 9, 2019

hacksparrow left a comment

Choose a reason for hiding this comment

bajtos commented Apr 9, 2019

hacksparrow left a comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

bajtos commented Apr 4, 2019 •

edited

Loading

jannyHou commented Apr 5, 2019 •

edited

Loading