Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node-mssql v6 incorrectly stores Dates #3202

Closed
pburrows opened this issue Dec 5, 2018 · 27 comments · Fixed by #7264, mattwelke/typeorm-postgres-example#165 or newerton/gobarber-2-backend#17

Comments

@pburrows
Copy link

pburrows commented Dec 5, 2018

Issue type:
[x ] question
[x] bug report

Database system/driver:
[x] mssql

TypeORM version:
[x] latest

Steps to reproduce or a small repository showing the problem:

I am trying to write a date, devoid of timezone information (a UTC date), to my SQL Server database. I created a test project to try it out.

(NOTE: I am in the eastern timezone)

Here are some of the ways I am trying to write the date '12/1/2018' to the database:

The most basic:

const test2: TestTable = new TestTable();
test2.testInfo = 'new Date(2018, 11, 1)';
test2.testDate = new Date(2018, 11, 1);
await TestDb.GETMANAGER().save(test2);

writes the following record to the database:

image

This date is off by 24 hours!!!!

The SQL generated by TypeOrm is this:

INSERT INTO "TestTable"("testInfo", "testDate") OUTPUT INSERTED."tableId" VALUES (@0, @1) -- PARAMETERS: [{"value":"new Date(2018, 11, 1)","type":"nvarchar","params":[]},{"value":"2018-12-01T05:00:00.000Z","type":"datetime2","params":[]}]

Which, if you put the parameters into the query you get this sql statement:

INSERT INTO "TestTable"("testInfo", "testDate") OUTPUT INSERTED."tableId" VALUES ('new Date(2018, 11, 1)', '2018-12-01T05:00:00.000Z') 

If you run THAT SQL Statement in SQL Server Management Studio (by copy and pasting), it writes this record to the database:

image

Which isn't exactly what I am looking for, but is wrong in a way I understand (it added my local timezone to the date string it generated) and that I can account for.

Why is the data written to the database different when the SQL Statement is run manually vs run by TypeORM?

How do I correctly write a UTC date to the database?

@pburrows pburrows changed the title How do you ignore timezone when writing a date to the database? Date is off by 24 hours when written to the database!! Dec 5, 2018
@pburrows
Copy link
Author

pburrows commented Dec 5, 2018

With some experimenting, 1 second after midnight will write a date of November 30th, but 1 MINUTE after midnight, will write the date of December 1st:

test2.testDate = new Date(2018, 11, 1, 0, 1, 0, 0); // midnight + 1 minute

writes:

2018-12-01 00:01:00.0000000

and:

test2.testDate = new Date(2018, 11, 1, 0, 0, 1, 0); // midnight + 1 second

writes:

2018-11-30 00:00:01.0000000

You can see that date jumps from November 30th to December 1st at the 38 seconds after midnight mark:

for (let i = 0; i < 60; i++) {
    const test2: TestTable = new TestTable();
    test2.testInfo = `new Date(2018, 11, 1, 0, 0, ${i}, 0)`;
    test2.testDate = new Date(2018, 11, 1, 0, 0, i, 0);
    await TestDb.GETMANAGER().save(test2);
    
}

image

@pburrows
Copy link
Author

pburrows commented Dec 5, 2018

(sorry for all the comments)

You can download my test project here if you would like to try yourself without setting up a new project.

@amaranth
Copy link
Contributor

amaranth commented Dec 5, 2018

After #3162 this is the only remaining issue blocking unit tests from passing on SQL Server.

@zaksesti-ns8
Copy link

I'm not sure about the exact issue you are running into but are you aware that mysql (driver) will attempt to do timezone conversion automatically. Perhaps this is contributing to the confusion. There is a timezone option which defaults to local. I have no idea in what case someone would find that helpful but that's the default. If you set timezone: 'Z' then it will default to UTC.
https://github.com/typeorm/typeorm/blob/master/docs/connection-options.md#mysql--mariadb-connection-options

@pleerock
Copy link
Member

As I remember you should have your timezone to set to UTC to prevent all different issues. Adding @chriskalmar

@pburrows can you please review #1717 and all related issues?

@pburrows
Copy link
Author

pburrows commented Dec 17, 2018

I looked at those issues and they don't seem to fix the problem.

I can definitely reproduce it every time with the latest code using the project I linked. It happens with datetime2 columns specifically. datetime columns don't exhibit the issue.

I am trying to narrow down where the problem is. I've narrowed it down so far to be somewhere in the SqlServerQueryRunner.ts, in the query() method. I am trying to reproduce the issue using just mssql driver, using the techniques in the query() method to do the query in hopes of pinpointing the issue.

I'll post again when I can narrow it down.

@pleerock
Copy link
Member

@pburrows did you set a local timezone to UTC?

@pburrows
Copy link
Author

(oops! didn't mean to hit close!)

@pburrows pburrows reopened this Dec 18, 2018
@pburrows
Copy link
Author

I tried setting process.env.TZ, but it didn't really make a difference.

I did not try changing the timezone on my OS, cause that does not seem like a reasonable solution. Heh.

@pleerock
Copy link
Member

just try to, just to see if it works =)

@pburrows
Copy link
Author

Heading for vacation in a bit, but I'll give it a shot when I get back.

My guess is that I will have the opposite problem of trying to make a non-UTC date to reproduce the issue with (because dates, by default, use the host's timezone, so they will be UTC, so nothing will need to be translated).

So it would be hard to reproduce this bug (or even notice it!) if your OS date happens to be UTC.

@pburrows
Copy link
Author

I ran the test with my local operating system timezone set to UTC and it worked as expected:

image

Setting my local operating system to any other time zone (I tried -1 (Azores), -2 (UCT-2), -4 (Salvador) and it... started working right!

Wait, what?

I was only testing with a specific date, December 1st, midnight + 28 seconds:

image

But why did that start working? I ran my full test again (which writes December 1st midnight +0 through 59 seconds) and the results have changed:

(compare this screenshot with the one I took when I first reported the issue)
image

Notice now only midnight and midnight + 1 second are off by 24 hours. All the other dates are correct.

It is still wrong, of course. I can run SQL Server Profiler and see that the query is being sent to SQL Server incorrectly.

Here is the query being sent to SQL Server for midnight +1 second:

exec sp_executesql @statement=N'INSERT INTO "TestTable"("testInfo", "testDate") OUTPUT INSERTED."tableId" VALUES (@0, @1)',@params=N'@0 nvarchar(33), @1 datetime2(7)',@0=N'new Date(2018, 11, 1, 0, 0, 1, 0)',@1='2018-11-30 00:00:01'

Notice the incorrect November 30th date being passed.

Here is the query being sent for midnight +2 seconds:

exec sp_executesql @statement=N'INSERT INTO "TestTable"("testInfo", "testDate") OUTPUT INSERTED."tableId" VALUES (@0, @1)',@params=N'@0 nvarchar(33), @1 datetime2(7)',@0=N'new Date(2018, 11, 1, 0, 0, 2, 0)',@1='2018-12-01 00:00:02'

Notice that the date being passed is now correctly December 1st.

I have had no luck figuring out where in TypeOrm it is going wrong. I've gone through the code in SqlServerQueryRunner, logged the values of the query() method at every step, and nothing seems wrong.

I tried to duplicate what SqlServerQueryRunner.query() is doing using the straight-up mssql driver, and have not been able to reproduce the issue (it writes the correct value at every step).

Now, that the point at which the date gets messed up has changed (after changing my local timezone), I am even more confused. I'll try digging through the connection settings next, maybe. Though this issue is definitely not something obvious.

@pburrows
Copy link
Author

Also worth mentioning, this is only happening with DateTime2 columns in Sql Server. It is not happening with DateTime columns.

@v1d3rm3
Copy link
Contributor

v1d3rm3 commented Jan 15, 2019

Also worth mentioning, this is only happening with DateTime2 columns in Sql Server. It is not happening with DateTime columns.

Here it's happening with datetime too. Have you tried options.useUTC = true?

@ghost
Copy link

ghost commented Sep 17, 2019

Also worth mentioning, this is only happening with DateTime2 columns in Sql Server. It is not happening with DateTime columns.

same here. Change TypeORM entity definition for date column to "DateTime" works. Table definition in database can still be "datetime2(7)". Also options.useUTC = true is set by default

@niccolofanton
Copy link

Also worth mentioning, this is only happening with DateTime2 columns in Sql Server. It is not happening with DateTime columns.

I'm also having this issue:
2020-02-09T23:00:00.000Z saved as 2020-02-10 00:00:00.0000000 but
2020-02-10T22:59:59.999Z saved as 2020-02-11 23:59:59.9990000

I solved by changing the entity column form datetime2 to datetime but I don't understand why input date is converted again if options.useUTC = true by default and the given date is already in UTC format. Mssql locale is UTC

@imnotjames
Copy link
Contributor

Tests for mssql are now passing in CI. Any time we use the older mssql module it acts up like this. Might be a good place to look for debugging.

We've confirmed it as working in our CI environment and I'll be closing this out.

@pburrows
Copy link
Author

pburrows commented Oct 11, 2020

Can you clarify "older mssql module?"
I just used the sample project I wrote (linked above), upgraded to the latest TypeOrm and latest MSSQL and latest TypeScript and the issue still fails.
If there is a different MSSQL module to use, can you specify what that is?

Also, I do not see a specific unit test for this issue (#3202) in the issue specific list of typeorm unit tests. If this is covered by a different specific issue, let me know.

@imnotjames
Copy link
Contributor

imnotjames commented Oct 11, 2020

I was mistaken and the fix in v6 was about datetime, not datetime2. I think this is an upstream issue, though, not TypeORM.

A commenter in another issue had mentioned that v5 was having this issue but v6 was not. Digging into this a bit, though, to confirm the issue, it seems that the comment was in reference to datetime, not the type you're using, datetime2.

Reopening and doing a bit of research :)

@imnotjames imnotjames reopened this Oct 11, 2020
@imnotjames
Copy link
Contributor

imnotjames commented Oct 11, 2020

We have installed the versions for testing:

  • MSSQL 6.2.3 (latest on npm is 6.2.3)
  • Tedious 6.7.0 (latest on npm is 9.2.1 🤔 Looks like that's only used by the alpha release of mssql 7.0?)

We do have tests that verify the various column types work for an identity check. Howeer, looks like some of them had been commented out - didn't see that before.

Bummer, because that's what I was referring to for tests.


I created a test specifically for this issue. datetime works as expected for me - the value is sent over correctly & everything works as expected. datetime2 doesn't for the first 3 seconds of the day for me.

Ok - so the issue is in datetime2.

Let's look at the client: It looks like we're binding the right date to the MSSQL client & doing everything as expected for the query but it fails when the server constructs it all somehow, and passes over the wire.

If I change the type we're binding it as - manually forcing all datetime2 values to be parameterized as datetime it all starts working as expected. Further pointing things at datetime2 being a problem.

To take TypeORM out of the equation, I've set up a minimal reproduction

const mssql = require('mssql');

const connection = new mssql.ConnectionPool({
    server: 'localhost',
    database: 'tempdb',
    options: {
      useUTC: false,
    },
    authentication: {
        type: 'default',
        options: {
            userName: 'sa',
            password: 'Admin12345'
        }
    }
});

connection.connect(() => {
    const request = new mssql.Request(connection);
    const testDatetime = new Date(2018, 11, 1, 0, 0, 0, 0);
    request.input(0, mssql.Int, Math.floor(Math.random() * 9999999));
    request.input(1, mssql.DateTime, testDatetime);
    request.input(2, mssql.DateTime2(), testDatetime);
    request.query("INSERT INTO \"test\" (id, testDatetime, testDatetime2) VALUES (@0, @1, @2);", console.log);
});

This ALSO exhibits the issue which means that this is not a bug in TypeORM but in node-mssql or tedious

Digging further, the datetime2 code in tedious does the following:

        const dstDiff = -(parameter.value.getTimezoneOffset() - YEAR_ONE.getTimezoneOffset()) * 60 * 1000;
        buffer.writeUInt24LE(Math.floor((+parameter.value - +YEAR_ONE + dstDiff) / 86400000));

If you replace parameter.value with our Dates:

const YEAR_ONE = new Date(2000, 0, -730118);

for (let i = 0; i < 5; i++) {
  const d = new Date(2018, 11, 1, 0, 0, i, 0);
  const dstDiff = -(d.getTimezoneOffset() - YEAR_ONE.getTimezoneOffset()) * 60 * 1000;
  console.log(Math.floor((+d - +YEAR_ONE + dstDiff) / 86400000));
}

You get the following outputs:

737027
737027
737028
737028
737028

This is apparently where this bug is - this is how we end up with the wrong date.

The rounding causes the date to be off by one. Thing is.. this code was changed by the latest release of tedious. Experimenting with the next release means updating to mssql 7.0.0-alpha.1 - which I did for the test. A few small changes needed but otherwise easy to get working.

And in testing - this fixes the problem. I think that can confirm that this is a bug in Tedious. The PR that fixed it in tedious is tediousjs/tedious#1023

Problem is, though - we cannot install that alpha release & use it because it's not compatible with the version we're using now. Until it's out of alpha I'm not comfortable coding against it, either.


@pburrows Does that help at all?

@imnotjames imnotjames changed the title Date is off by 24 hours when written to the database!! node-mssql v6 incorrectly stores Dates Oct 11, 2020
@pburrows
Copy link
Author

Thanks, so much, Jim! Great analysis. I am very happy you discovered the issue. That's awesome. I guess now it is just a matter of waiting until everything gets released.

@cvicenteust
Copy link

cvicenteust commented Nov 26, 2020

Hi! I'm trying to test this solution because we have a similar issue with smalldatetime. Problem is I cannot link typeorm 0.2.29 with mssql 7.0.0-alpha.1:

const serverSql: SqlServerConnectionOptions = {
        name: process.env.DB_SQL_DEFAULT_NAME,
        entities: [`${__dirname}/../../schemas/**/*.js`],
        type: 'mssql',
        host: process.env.DB_SQL_DEFAULT_HOST,
        port: Number(process.env.DB_SQL_DEFAULT_PORT),
        username: process.env.DB_SQL_DEFAULT_USER,
        password: process.env.DB_SQL_DEFAULT_PASSWORD,
        requestTimeout: 30000,
        options: {
          connectionIsolationLevel: 'READ_UNCOMMITTED',
          isolation: 'READ_UNCOMMITTED',
          enableArithAbort: false
        },
        pool: {
          acquireTimeoutMillis: 30000
        },
        logging: 'all'
      };
      await createConnections([serverSql])

Throws this error: Connection error: The "config.options.connectionIsolationLevel" property must be of type number. Received type string (READ_UNCOMMITTED)

Any ideas?

@nebkat
Copy link
Contributor

nebkat commented Nov 26, 2020

This suggests you have to use an enum or something tediousjs/node-mssql#482

@cvicenteust
Copy link

I've tried using ISOLATION_LEVEL enum, but typeorm SqlConnectionOptions only validates "strings"

@nebkat
Copy link
Contributor

nebkat commented Nov 26, 2020

You can use the enum with as any and it will pass, or if you want a permanent fix you could open a pull request that makes it also accept a number.

@cvicenteust
Copy link

cvicenteust commented Nov 27, 2020

Previous error dissapear, but now it shows:

TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be of type string or an instance of Buffer or ArrayBuffer. Received type number (0)

when I try to do a DB operation (the login SELECT in this case)

@matispf84
Copy link

Previous error dissapear, but now it shows:

TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be of type string or an instance of Buffer or ArrayBuffer. Received type number (0)

when I try to do a DB operation (the login SELECT in this case)

I have the same problem with datetime type. When I save for example 2020-11-24T00: 00: 00.000Z in the database it stores 2020-11-23T00: 00: 00.000Z (one day less).
I see that in the node_modules\tedious\lib\data-types\datetime.js file in mssql version 7.0 alpha it is fixed but this version does not work with Typeorm

AlexMesser pushed a commit that referenced this issue Feb 6, 2021
This upgrades the `mssql` driver to version 7 to resolve #3202. This
issue was created in the bulk insert test adding a datetime2 field
with a time of midnight.

v7 no longer allows numeric parameter names and was throwing an error
inside tedious but you can see that `node-mssql` is expecting a string.
https://github.com/tediousjs/node-mssql/blob/76585f973dd8cad48836fe302f99f67d47ff6129/lib/base/request.js#L101
Another user saw this issue in the comments of issue #3202.
#3202 (comment)

Stringifying the numeric parameter names before it enters the driver
resolves this issue.

Now all mssql tests pass locally once you provide additional config in
`ormconfig.json` locally required for a locally running docker instance
of mssql which uses a self signed certificate.
```json
"extra": {
  "trustServerCertificate": true
}
```
v1d3rm3 pushed a commit to v1d3rm3/typeorm that referenced this issue Jun 30, 2021
This upgrades the `mssql` driver to version 7 to resolve typeorm#3202. This
issue was created in the bulk insert test adding a datetime2 field
with a time of midnight.

v7 no longer allows numeric parameter names and was throwing an error
inside tedious but you can see that `node-mssql` is expecting a string.
https://github.com/tediousjs/node-mssql/blob/76585f973dd8cad48836fe302f99f67d47ff6129/lib/base/request.js#L101
Another user saw this issue in the comments of issue typeorm#3202.
typeorm#3202 (comment)

Stringifying the numeric parameter names before it enters the driver
resolves this issue.

Now all mssql tests pass locally once you provide additional config in
`ormconfig.json` locally required for a locally running docker instance
of mssql which uses a self signed certificate.
```json
"extra": {
  "trustServerCertificate": true
}
```

(cherry picked from commit 4711a71)
zshipleyTAG pushed a commit to Amherst-Development/typeorm that referenced this issue Oct 17, 2022
This upgrades the `mssql` driver to version 7 to resolve typeorm#3202. This
issue was created in the bulk insert test adding a datetime2 field
with a time of midnight.

v7 no longer allows numeric parameter names and was throwing an error
inside tedious but you can see that `node-mssql` is expecting a string.
https://github.com/tediousjs/node-mssql/blob/76585f973dd8cad48836fe302f99f67d47ff6129/lib/base/request.js#L101
Another user saw this issue in the comments of issue typeorm#3202.
typeorm#3202 (comment)

Stringifying the numeric parameter names before it enters the driver
resolves this issue.

Now all mssql tests pass locally once you provide additional config in
`ormconfig.json` locally required for a locally running docker instance
of mssql which uses a self signed certificate.
```json
"extra": {
  "trustServerCertificate": true
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment