tedious icon indicating copy to clipboard operation
tedious copied to clipboard

validate bulk load data

Open mottykohn opened this issue 7 years ago • 5 comments

This will add flexibility. e.g. Dates can be strings... At a speed cost...

Before submitting a PR :

  1. Ensure your fork is created from master branch of the repository.
  2. Run npm install in the root folder.
  3. After bug fix/code change, ensure all the existing tests and new tests (if any) pass (npm run-script test-all). During development, to run individual test use node_modules/nodeunit test/<test_file.js> -t <test_name>.
  4. Build the driver (npm run build).
  5. Run eslint and flow typechecker (npm run lint).
  6. Run commitlint (node_modules/.bin/commitlint --from origin/master --to HEAD). Refer commit conventions and commit rules.

Thank you for Contributing!

mottykohn avatar Jun 20 '18 20:06 mottykohn

@mottykohn Thanks for submitting the PR.

I get error if I try to send date as string, both with and without your fix 😅

  bulkLoad.addColumn('_date', TYPES.Date, { nullable: true });
  bulkLoad.addRow({ _date: '1990-05-05' });

And its the same error in both the cases,

src\data-types\date.js:28
        var dstDiff = -(parameter.value.getTimezoneOffset() - YEAR_ONE.getTimezoneOffset()) * 60 * 1000;
                                        ^
TypeError: parameter.value.getTimezoneOffset is not a function

It is because string value becomes number at the below condition, and writeParameterData() tries getTime() or getTimezoneOffset() on that number.

https://github.com/tediousjs/tedious/blob/c22c894d887797fc0062a07b68ffaad9b3a06e72/src/data-types/date.js#L33-L39

Best way to send string to any SQL Server datatype, would be to set the TYPE as VarChar or NVarChar and let Server handle the conversion.

  bulkLoad.addColumn('_date', TYPES.VarChar, { nullable: true });

Suraiya-Hameed avatar Jun 29 '18 19:06 Suraiya-Hameed

Whatever check+conversion validate() does is flaky, that causes error even with your fix, sorry about that 😓

Suraiya-Hameed avatar Jun 29 '18 19:06 Suraiya-Hameed

Didn't see that because I'm on option.useUTC=true. Should I fix by

    if (!(value instanceof Date)) {
      value = Date.parse(value);
      if (!options.useUTC && !isNaN(value))
        value = new Date(value + new Date().getTimezoneOffset() * 60 * 1000)
    }

I am using this library dynamically where I don't know column types in advance. But I see that I'm running into the old timezone problem.

mottykohn avatar Jun 29 '18 20:06 mottykohn

Unfortunately we will run into this issue for the other temporal data-types (date, datetime, datetime2, time) too. Wouldn't setting the type as VarChar or NVarChar work for your case?

Suraiya-Hameed avatar Jun 29 '18 20:06 Suraiya-Hameed

I'm using mssql.recordset.toTable() function - I guess I could try to loop over columns and switch all date* columns to Varchars, Just not sure how easy that is to reassign a column type in that library.

mottykohn avatar Jun 29 '18 20:06 mottykohn