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

Azure MS SQL gets completely different decimals #1012

Closed
obermobber opened this issue Dec 12, 2019 · 21 comments
Closed

Azure MS SQL gets completely different decimals #1012

obermobber opened this issue Dec 12, 2019 · 21 comments
Labels
Response needed Response by tedious member is needed

Comments

@obermobber
Copy link

obermobber commented Dec 12, 2019

Hello together,

I had a Range Error Issue which forced me to update tedious to version 6.5.0 (I tried to update to 6.6.1 but that didn't work out, please see issue #1008 )

Now, when I try to save a high decimal value, it turns into a different, much smaller number.

18728.4 turns into 281.655926290
21345.4 turns into 2898.655926290
625348.4 turns into 16605.845567585
&
34348.42 turns into 15901.675926290

It's suspicious how often they end with 5926290.
When the execution is posted to the console, I actually see the correct value. So it changes on the way after the query and into the db.

POSTing 12304.53 actually works and will be written into the DB just like that.

I use it in combination with
sequelize 5.15.1
typescript: 3.3.4000
Node: 10.15.3
an Azure MS SQL database

and the field is defined as

numResult: {
		type: DECIMAL(18, 9),
	},

This is also what the ms sql database says itself about that field.

Let me know if you need additional information

@arthurschreiber
Copy link
Collaborator

So if you check the value via a different tool in the database, the value that was written is incorrect? I'll try to write a test case for this.

@obermobber
Copy link
Author

Well I use my nodeJs sequelize server to write the value and I also see the query, where the value is correct.

Then, using DBeaver, I see the incorrect value in the DB.
GETting it with sequelize of course returns the wrong value as well.

@obermobber
Copy link
Author

@arthurschreiber fyi I updated to tedious v6.6.1 after fixing the problem described in #1008 . Didn't make a difference for this issue though

@obermobber
Copy link
Author

Does anyone have an indication why this might happen and/or what a workaround could look like?

@arthurschreiber
Copy link
Collaborator

@obermobber I'll take a look as soon as I can.

@obermobber
Copy link
Author

Okay, thanks @arthurschreiber ! It's just really driving me crazy haha

@arthurschreiber
Copy link
Collaborator

@obermobber I tried reproducing this, but I'm failing to do so.

I added new test cases to the decimal tests in test/integration/parameterised-statements-test.js, with the values you posted in this issue, but they get send to the SQLServer correctly.

Can you distill the issue you're seeing into a simple test case? 🙇

@arthurschreiber arthurschreiber added the Response needed Response by tedious member is needed label Dec 14, 2019
@obermobber
Copy link
Author

@arthurschreiber shame on me, but I've never written a single test in my short developer life (it's bad, i know..). But I'd be more than happy to provide all the information necessary :(

@arthurschreiber
Copy link
Collaborator

It doesn't have to be a test case written in the framework we use for for testing tedious.

I just need the minimum amount of code that would allow me to reproduce this issue. 😄

@obermobber
Copy link
Author

obermobber commented Dec 14, 2019

So, I hope I got that right.

All I'm executing is this:

@Post('/test')
	private async postDecimalTest(
		@Body() body: testCaseBody
	) {
		try {
			const transaction = await sequelize.transaction();
			
			await DB.TestExecutionValues.create({
				testId: body.testId,
				numResult: body.numResult,
				strResult: body.strResult,
				enumResultSid: body.enumResult,
				unitId: body.unitId,
				ynPassed: body.ynPassed,
				comment: body.comment,
			}, { transaction });

			await transaction.commit();
			return { message: 'success posting test' };
		} catch (e) {
			console.log(e);
			throw e;
		}
	}

I am sending this body:

{
    "testId": "LEVEL",
    "numResult": 465324.75,
    "strResult": "testcase",
    "enumResult": "1",
    "unitId": "m³",
    "ynPassed": 1,
    "comment": ""
}

this is what's logged:

Executing (036ca06b3c1869b4851b): BEGIN TRANSACTION;
Executing (036ca06b3c1869b4851b): INSERT INTO [TestExecutionValues] ([testId],[numResult],[strResult],[enumResultSid],[unitId],[ynPassed],[comment]) OUTPUT INSERTED.* VALUES (@0,@1,@2,@3,@4,@5,@6);
Executing (036ca06b3c1869b4851b): COMMIT TRANSACTION;

and this is what's written in the DB:

{
                "testId": {
                    "id": "LEVEL",
                    "description": null
                },
                "numResult": 4156.148157261,
                "strResult": "testcase",
                "enumResult": {
                    "id": "#N/A",
                    "description": null
                },
                "unit": {
                    "id": "m³",
                    "description": null
                },
                "ynPassed": true,
                "comment": ""
}

so 465324.75 turns into 4156.148157261

I use routing controllers 0.7.7 for the requests, if that's important to know.
Need any other information? (or was that whole thing right in the first place?)

+++EDIT+++

Thought you might need the table definition:

const TestExecutionValues = sequelize.define('TestExecutionValues', {
	sid: {
		type: INTEGER,
		autoIncrement: true,
		primaryKey: true,
		allowNull: false,
	},
	testId: {
		type: STRING,
		references: {
			model: Tests,
			key: 'testId',
		},
	},
	numResult: {
		type: DECIMAL(18, 9),
	},
	strResult: {
		type: STRING,
	},
	enumResultSid: {
		type: INTEGER,
		references: {
			model: EnumResults,
			key: 'sid',
		},
	},
	unitId: {
		type: STRING,
		references: {
			model: Units,
			key: 'unitId',
		},
	},
	ynPassed: {
		type: BOOLEAN,
	},
	comment: {
		type: TEXT,
	},
}, {
	timestamps: false,
});

@arthurschreiber
Copy link
Collaborator

It looks like you're using sequelize on top of tedious. This often makes issues harder to track down, because there's more moving parts where things could be going wrong. 😞

How is DB.TestExecutionValues implemented? 🤔

@arthurschreiber
Copy link
Collaborator

Ah, you just posted the definition. I'll see what I can do.

@obermobber
Copy link
Author

Yes, I use tedious as DB driver for sequelize..
If it's of any help, this is a DBeaver screenshot of the field:

Bildschirmfoto 2019-12-14 um 12 20 05

@arthurschreiber
Copy link
Collaborator

Here is an example I just whipped up:

const { Connection, Request, TYPES: { Decimal } } = require('tedious');

const connection = new Connection({
  // ...
});

connection.on('connect', () => {
  const request = new Request('CREATE TABLE #decimals (value decimal(18, 9))', (err) => {
    if (err) {
      throw err;
    }

    const request = new Request('INSERT INTO #decimals (value) VALUES (@value)', (err) => {
      if (err) {
        throw err;
      }

      const request = new Request('SELECT * FROM #decimals', (err) => {
        if (err) {
          throw err;
        }

        connection.close();
      });

      request.on('row', (columns) => {
        console.log(columns);
      });

      connection.execSql(request);
    });

    request.addParameter('value', Decimal, 18728.4, { precision: 18, scale: 9 });

    connection.execSql(request);
  });

  connection.execSqlBatch(request);
});

and this is the output I get back:

[ { value: 18728.4,
    metadata:
     { userType: 0,
       flags: 9,
       type: [Object],
       collation: undefined,
       precision: 18,
       scale: 9,
       udtInfo: undefined,
       dataLength: 17,
       schema: undefined,
       colName: 'value',
       tableName: undefined } } ]

I really don't think this is an issue with tedious. 🤔 You can maybe try running this same code against your database and see if you get back different results?

Which version of tedious are you using via sequelize?

@obermobber
Copy link
Author

obermobber commented Dec 14, 2019

hmm... I'll try that, thanks!

Currently stuck on
RequestError: Requests can only be made in the LoggedIn state, not the SentPrelogin state

But while I try to fix that, here is a snapshot of my package-lock (tedious is not listed in the dependencies of sequelize, this is the only occurrence of tedious in my package-lock):

"tedious": {
      "version": "6.6.2",
      "resolved": "https://registry.npmjs.org/tedious/-/tedious-6.6.2.tgz",
      "integrity": "sha512-0Yziuys2h66dVlqMPJpNFciQ/N2VrgwY8o8TXyj4OZBaxrvqRPeMuTKZZVBFTGOjt/J15fR0fX0HBnCHjm7QWA==",
      "requires": {
        "@azure/ms-rest-nodeauth": "2.0.2",
        "@types/node": "^12.7.11",
        "@types/readable-stream": "^2.3.5",
        "bl": "^3.0.0",
        "depd": "^2.0.0",
        "iconv-lite": "^0.5.0",
        "jsbi": "^3.1.1",
        "native-duplexpair": "^1.0.0",
        "punycode": "^2.1.0",
        "readable-stream": "^3.4.0",
        "sprintf-js": "^1.1.2"
      },

@arthurschreiber
Copy link
Collaborator

arthurschreiber commented Dec 14, 2019

Oh, if you change the code to this:

const { Connection, Request, TYPES: { Decimal } } = require('tedious');

const connection = new Connection({
  // ...
});

connection.on('connect', (err) => {
  if (err) {
    throw err;
  }

  const request = new Request('CREATE TABLE #decimals (value decimal(18, 9))', (err) => {
    if (err) {
      throw err;
    }

    const request = new Request('INSERT INTO #decimals (value) VALUES (@value)', (err) => {
      if (err) {
        throw err;
      }

      const request = new Request('SELECT * FROM #decimals', (err) => {
        if (err) {
          throw err;
        }

        connection.close();
      });

      request.on('row', (columns) => {
        console.log(columns);
      });

      connection.execSql(request);
    });

    request.addParameter('value', Decimal, 18728.4, { precision: 18, scale: 9 });

    connection.execSql(request);
  });

  connection.execSqlBatch(request);
});

You should see a better error message if connecting fails.

@obermobber
Copy link
Author

That actually helped a lot, I didn't have the encryption property.

The following should be correct right?

const connection = new Connection({
    server: 'server',
    authentication: {
        type: 'default',
        options: {
            userName: 'user',
            password: 'password',
        }
    },
    options: {
        encrypt: true,
        database: 'dbname'
    }
});

I used the same credentials with DBeaver and din't have any problems, but I always get ConnectionError: Login failed for user 'user'.
The only difference in my sequelize connection setup is the dialect option, but I didn't find that here: http://tediousjs.github.io/tedious/api-connection.html

Sorry for all the trouble!

@arthurschreiber
Copy link
Collaborator

That should be right! Is the user you're connecting with a SQLServer user or a Azure Active Directory account user? (There can be both/either on Azure databases). Try using azure-active-directory-password as the authentication type instead of default.

@obermobber
Copy link
Author

This brought me a step closer. Now I get (node:67210) UnhandledPromiseRejectionWarning: ConnectionError: Security token could not be authenticated or authorized.
But this seems to be more of a problem on the azure configuration side. (at least that's where google's directing me haha)

Regarding this issue, everything seems pointing towards an error somewhere else than tedious, so we could close this on my part and I'd probably raise an issue with sequelize.

I really really appreciate the time you took @arthurschreiber !!

@arthurschreiber
Copy link
Collaborator

Hm. I'm still wondering why you can connect using sequelize, but not using tedious. 😬

Feel free to close this issue and reopen it once you can confirm the problem is coming from tedious. 🙇

@obermobber
Copy link
Author

I wouldn't say no to solving this mystery with you haha! But I didn't want to take any more of your time 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Response needed Response by tedious member is needed
Projects
None yet
Development

No branches or pull requests

2 participants