feature: Improved Data Type validation/conversion. #677

arthurschreiber · 2018-01-10T07:37:48Z

This refactors the data type logic in tedious, making it more strict and more accurate.

If the parameter specified by the user can not be converted to the data type of the parameter without a loss of precision, the request will not be executed and an error will be passed to the request's callback instead.

The same parameter value conversion logic is now also used for both regular requests as well as for bulk loads, fixing a long standing issue where storing the same data via these two methods would result either result in a failure or different data being stored in the database.

This is dependent upon #676 being merged first.

arthurschreiber · 2018-01-16T17:49:46Z

@v-suhame @chdh @atsage @Hadis-Fard Would you mind taking a look at this?

Suraiya-Hameed · 2018-01-16T19:43:05Z

src/data-types/timen.js

-      return value;
+      dateValue = value;
+    } else {
+      dateValue = new Date(Date.parse(value));


Need to remove this redundant conversion to Date too 😄

Yeah. I'm just keeping it because the old code allowed passing in strings as values for Time. We can remove this in a breaking change later. 👍

I brought that up because datetime2n and datetimeoffsetn had conversion removed. Shouldn't the same behaviour apply for all 3 datatype?

Yeah, unfortunately I think that there might be users out there that use time and pass in timestrings. I'm not 100% sure, and I'd rather not break this right now, but sometime in the future. 😅

😅 I think I should elaborated a bit more. If users are passing time in timestrings format, then it is likely they will do the same for datetime2 and datetimeoffset, as they all have nano second precision.

Code fragment from timen.js and datetimeoffsetn.js looks like,

validate(value) { ... dateValue = new Date(Date.parse(value)); ... } writeParameterData: function(..){ .. if (parameter.value != null) { const time = new Date(+parameter.value); // datetime2n.js doesn't have this conversion ... }

We should add the conversion(pointed in the code fragment) back into datetime2n.js to allow string passed into datetime2.

Ah, yes, you're totally right! ❤️ Fixed this in 0d4e197.

I think all 3 types now use Date.parse in the validate method to allow conversion from strings, and they no longer perform redundant conversion inside the writeParameterData method.

Suraiya-Hameed · 2018-01-17T00:22:35Z

src/data-types/money.js

-    return value;
+
+    if (!Number.isFinite(numberValue) || (typeof value === 'string' && value !== numberValue.toString()) || numberValue.toString() !== numberValue.toFixed(4)) {
+      return new TypeError(`The given value could not be converted to ${this.name}`);


How about removing or having an alternative for numberValue.toString() !== numberValue.toFixed(4)?

This condition is too strict, if input has scale < 4 it will throw exception, but driver should allow scale <= 4. Same goes for smallmoney.

Can you also add a test for money/smallmoney type with scale < 4? All current tests are for scale 4.

You're right, I missed handling of numbers that have less than 4 numbers after the decimal point. 😓

numberValue.toString().split('.')[1].length > 4

Suraiya-Hameed · 2018-01-17T00:36:04Z

src/data-types/nvarchar.js

-  validate: function(value) {
-    if (value == null) {
+  validate(value, length, precision, scale) {
+    if (value === undefined || value === null) {


Is precision & scale here by mistake?

Yeah, will drop those! 👍

Done in f9b49e1.

Suraiya-Hameed · 2018-01-17T01:30:47Z

src/data-types/real.js

-    return value;
+
+    if (!Number.isFinite(numberValue) || (typeof value === 'string' && value !== numberValue.toString()) || numberValue !== Math.fround(numberValue)) {
+      return new TypeError(`The given value could not be converted to ${this.name}`);


Wouldn't using Math.fround() be a breaking change? 🤔 I'm afraid that most of the data passed would fail at this point, and hinder the usage of real type.

Yeah, this one's a bit harder to decide.

The loss of precision is a huge issue tho, because most users will not know that real is limited and won't save the exact value they're trying to send, and thus the data in the database can no longer be trusted. That's why I think raising an error here is better than before and fixes a bug. What do you think? 😄

I agree to raising error here ✌️ I was just wondering how much of an impact it would have on existing user(i.e. will it be a breaking change), but as you pointed, users must have already run into precision loss, so it better to have this in sooner rather than later.

Suraiya-Hameed · 2018-01-22T21:21:50Z

src/data-types/varchar.js

-        return TypeError('Invalid string.');
-      }
-      value = value.toString();
+


suggestion: remove precision, scale

Good idea! 👍

Suraiya-Hameed · 2018-01-22T21:25:51Z

src/request.js

@@ -175,11 +175,13 @@ module.exports = class Request extends EventEmitter {
  validateParameters() {
    for (let i = 0, len = this.parameters.length; i < len; i++) {
      const parameter = this.parameters[i];
-      const value = parameter.type.validate(parameter.value);
+      const value = parameter.type.validate(parameter.value, parameter.length, parameter.precision, parameter.scale);


Precision and scale are never actually used the validate(), let's remove them.

Yeah, that's a good point. They should be used for numeric and decimal (and maybe for some of the date types?). I'll take another look! 👍

Done in 0968cb4.

This refactors the data type logic in tedious, making it more strict and more accurate. If the parameter specified by the user can not be converted to the data type of the parameter without a loss of precision, the request will not be executed and an error will be passed to the request's callback instead. The same parameter value conversion logic is now also used for both regular requests as well as for bulk loads, fixing a long standing issue where storing the same data via these two methods would result either result in a failure or different data being stored in the database.

arthurschreiber · 2018-05-21T08:28:10Z

Heya @tediousjs/microsoft-contributors 👋

how do you feel about this changeset? Should we declare it as a breaking change and merge and release it together with other breaking changes as v3.0?

Suraiya-Hameed · 2018-05-23T17:26:37Z

+1 to releasing this along with other breaking changes. This PR is good to go once #677 (review) is handled 😄

Suraiya-Hameed · 2018-07-27T00:05:21Z

src/bulk-load.js

        length: c.length,
        scale: c.scale,
        precision: c.precision,
-        value: row[arr ? i : c.objName]
-      }, this.options);
+        value: c.type.validate(row[arr ? i : c.objName], c.length, c.precision, c.scale)


validate() doesn't throw error but it returns error if validation fails. Can you add check to verify type of validate()'s return value before passing it to writeParameterData?

MichaelSun90 · 2020-02-28T19:26:14Z

@arthurschreiber, I think this PR may be outdated since the recent merged pip related changes? If you can confirm that, then I think can close this PR for now.

…ter-type-validation

IanChokS · 2020-03-17T22:57:32Z

src/data-types/float.js

@@ -17,20 +17,28 @@ module.exports = {
  writeParameterData: function(buffer, parameter) {
    if (parameter.value != null) {
      buffer.writeUInt8(8);
-      buffer.writeDoubleLE(parseFloat(parameter.value));
+      buffer.writeDoubleLE(parameter.value);


I'm not sure if this change is necessary?

IanChokS · 2020-03-25T19:03:42Z

src/data-types/tvp.js

+          return {
+            value: column.type.validate(columnValue, column.length, column.scale, column.precision),
+            length: column.length,
+            scale: column.scale,
+            precision: column.precision
+          };


Returning an object causes data-type value inputs to vary: if using TVP, data-type inputs would be this object, whereas anything else would be a single value. To avoid this two variations of data-type inputs, this TVP .validate() method should only return the single value, such as return column.type.validate(columnValue, column.length, column.scale, column.precision)

codecov · 2020-03-25T19:11:41Z

Codecov Report

Merging #677 into master will decrease coverage by 0.92%.
The diff coverage is 81.77%.

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   80.77%   79.85%   -0.93%     
==========================================
  Files          86       86              
  Lines        4384     4427      +43     
  Branches      793      807      +14     
==========================================
- Hits         3541     3535       -6     
- Misses        584      630      +46     
- Partials      259      262       +3

Impacted Files	Coverage Δ
src/data-type.ts	`100.00% <ø> (ø)`
src/data-types/date.ts	`48.00% <0.00%> (-2.00%)`	⬇️
src/data-types/binary.ts	`70.27% <25.00%> (-6.21%)`	⬇️
src/data-types/varbinary.ts	`71.21% <25.00%> (-3.40%)`	⬇️
src/data-types/bigint.ts	`77.27% <40.00%> (-12.21%)`	⬇️
src/data-types/uniqueidentifier.ts	`83.33% <50.00%> (+5.55%)`	⬆️
src/data-types/tvp.ts	`89.65% <63.63%> (+0.76%)`	⬆️
src/bulk-load.ts	`79.37% <66.66%> (-1.04%)`	⬇️
src/data-types/nvarchar.ts	`90.24% <72.72%> (-3.27%)`	⬇️
src/data-types/varchar.ts	`83.33% <72.72%> (-3.24%)`	⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb901a...0ff5ba0. Read the comment docs.

arthurschreiber requested review from chdh and Suraiya-Hameed January 10, 2018 07:37

This was referenced Jan 10, 2018

BulkLoad do not validateParameters() #421

Open

Stricter Data Type checks #674

Closed

Suraiya-Hameed reviewed Jan 16, 2018

View reviewed changes

Suraiya-Hameed reviewed Jan 17, 2018

View reviewed changes

Suraiya-Hameed reviewed Jan 22, 2018

View reviewed changes

arthurschreiber force-pushed the arthur/better-type-validation branch 2 times, most recently from 31d6acd to 9ae5f2f Compare January 30, 2018 17:04

arthurschreiber force-pushed the arthur/better-type-validation branch from 9ae5f2f to 1c4d0a6 Compare May 21, 2018 08:24

Suraiya-Hameed reviewed Jul 27, 2018

View reviewed changes

IanChokS mentioned this pull request Dec 18, 2019

Inserts into varbinary(max) col with FILESTREAM or reads from that column takes 10's of seconds #971

Closed

MichaelSun90 added this to the Preparation for dropping support to NodeJS 6 milestone Feb 28, 2020

Merge branch 'master' of github.com:tediousjs/tedious into arthur/bet…

f4a84e8

…ter-type-validation

IanChokS reviewed Mar 17, 2020

View reviewed changes

chore: saving progress

ff3cebb

IanChokS reviewed Mar 25, 2020

View reviewed changes

refactor: merge master

c5e234b

chore: remove comment

0ff5ba0

IanChokS added the BREAKING CHANGE label Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Improved Data Type validation/conversion. #677

feature: Improved Data Type validation/conversion. #677

arthurschreiber commented Jan 10, 2018

arthurschreiber commented Jan 16, 2018

Suraiya-Hameed Jan 16, 2018

arthurschreiber Jan 16, 2018

Suraiya-Hameed Jan 16, 2018

arthurschreiber Jan 19, 2018

Suraiya-Hameed Jan 19, 2018

arthurschreiber Jan 20, 2018

arthurschreiber Jan 20, 2018 •

edited

Suraiya-Hameed Jan 22, 2018

Suraiya-Hameed Jan 17, 2018 •

edited

Suraiya-Hameed Jan 17, 2018

arthurschreiber Jan 19, 2018

IanChokS Mar 17, 2020 •

edited

Suraiya-Hameed Jan 17, 2018

arthurschreiber Jan 19, 2018

arthurschreiber Jan 20, 2018

Suraiya-Hameed Jan 17, 2018

arthurschreiber Jan 19, 2018 •

edited

Suraiya-Hameed Jan 19, 2018

Suraiya-Hameed Jan 22, 2018 •

edited

arthurschreiber Jan 22, 2018

Suraiya-Hameed Jan 22, 2018

arthurschreiber Jan 22, 2018

arthurschreiber Jan 22, 2018

arthurschreiber commented May 21, 2018

Suraiya-Hameed commented May 23, 2018

Suraiya-Hameed Jul 27, 2018

MichaelSun90 commented Feb 28, 2020

IanChokS Mar 17, 2020

IanChokS Mar 25, 2020

codecov bot commented Mar 25, 2020 •

edited

feature: Improved Data Type validation/conversion. #677

Are you sure you want to change the base?

feature: Improved Data Type validation/conversion. #677

Conversation

arthurschreiber commented Jan 10, 2018

arthurschreiber commented Jan 16, 2018

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

arthurschreiber Jan 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suraiya-Hameed Jan 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanChokS Mar 17, 2020 • edited

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

arthurschreiber Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suraiya-Hameed Jan 22, 2018 • edited

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

arthurschreiber commented May 21, 2018

Suraiya-Hameed commented May 23, 2018

Choose a reason for hiding this comment

MichaelSun90 commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2020 • edited

Codecov Report

arthurschreiber Jan 20, 2018 •

edited

Suraiya-Hameed Jan 17, 2018 •

edited

IanChokS Mar 17, 2020 •

edited

arthurschreiber Jan 19, 2018 •

edited

Suraiya-Hameed Jan 22, 2018 •

edited

codecov bot commented Mar 25, 2020 •

edited