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

fix(mssql) insert record failure because of BOOLEAN column type #12090

Merged
merged 3 commits into from Apr 16, 2020

Conversation

mcgradycchen
Copy link
Contributor

@mcgradycchen mcgradycchen commented Apr 9, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Hi @sushantdhiman

I reopen an PR to merge to master tree simply.
#12024

This is issue is similar with #11121

sequelize@^5.21.5
tedious@8.2.0

The sample code is following

var Company= sequelize.define('tm_company', {
   "COMPANY_GUID": {
       "type": Sequelize.STRING(36)",  "allowNull": false, "primaryKey": true, "defaultValue": Sequelize.UUIDV1},
    "ENCRYPT_ALL": { "type": Sequelize.BOOLEAN, "allowNull": false, "defaultValue": false },
    "MORE_INFO": { "type": Sequelize.TEXT, "allowNull": true}
});

Company.sync().then(() => {
  Company.create({});
})

The db query exception:

SequelizeDatabaseError: Conversion failed when converting the nvarchar value 'false' to data type smallint. at Query.formatError (/opt/o365/node/node_modules/sequelize/lib/dialects/mssql/query.js:314:12) at Request.connection.lib.Request [as userCallback] (/opt/o365/node/node_modules/sequelize/lib/dialects/mssql/query.js:74:23) at Request.callback
.......
sql: 'INSERT INTO [tm_company] ([COMPANY_GUID],[ENCRYPT_ALL],[CREATED_AT],[UPDATED_AT]) OUTPUT INSERTED.* VALUES (@0,@1,@2,@3);', parameters: { '0': 'aba8c810-6d83-11ea-a5bd-b122539542df', '1': false, '2': '2020-03-24 03:58:07.701 +00:00', '3': '2020-03-24 03:58:07.701 +00:00' } }

According to the tedious document, we need to set the boolean type to TYPES.Bit

@codecov
Copy link

@codecov codecov bot commented Apr 9, 2020

Codecov Report

Merging #12090 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12090   +/-   ##
=======================================
  Coverage   96.15%   96.16%           
=======================================
  Files          95       95           
  Lines        9244     9246    +2     
=======================================
+ Hits         8889     8891    +2     
  Misses        355      355           
Impacted Files Coverage Δ
lib/dialects/mssql/query.js 95.67% <100.00%> (+0.04%) ⬆️

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 d16de58...f5c5673. Read the comment docs.

@mcgradycchen
Copy link
Contributor Author

@mcgradycchen mcgradycchen commented Apr 9, 2020

Hi @sushantdhiman

This needs a PR to master first. Also you will need to add a test. I wonder why our tests are not failing, what version of tedious is used here?

Edit: tedious@8.2.0, hmm we need to update our tedious version then

By your last review, I check your mssql regression test cases. It does not have any columns with Sequelize.BOOLEAN type, I think that's why you did not find this issue.
I will add test cases for this issue.

Thanks

Copy link
Contributor Author

@mcgradycchen mcgradycchen left a comment

Add regression test case

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 9, 2020

I can't reproduce this issue, are you sure this test case fails without your fix. Also I found 72 cases where we are using BOOLEAN data types, so tests for this case are already there.

I tried with latest mssql image and tedious@8.2.0

  (async () => {
    const BooleanTable = sequelize.define(
      "BooleanTable",
      {
        status: {
          type: Sequelize.BOOLEAN,
          defaultValue: false,
          allowNull: false,
        },
      },
      {
        freezeTableName: true,
      }
    );
  
    const value = true;
  
    return BooleanTable.sync({ force: true })
      .then(() => {
        return BooleanTable.create({
          status: value,
        });
      })
      .then(() => BooleanTable.findOne())
      .then(r => console.log(r.toJSON()))
  })();
tedious deprecated The default value for `config.options.trustServerCertificate` will change from `true` to `false` in the next major version of `tedious`. Set the value to `true` or `false` explicitly to silence this message. lib/dialects/mssql/connection-manager.js:63:26
Executing (default): IF OBJECT_ID('[BooleanTable]', 'U') IS NOT NULL DROP TABLE [BooleanTable];
Executing (default): IF OBJECT_ID('[BooleanTable]', 'U') IS NULL CREATE TABLE [BooleanTable] ([id] INTEGER NOT NULL IDENTITY(1,1) , [status] BIT NOT NULL DEFAULT 0, [createdAt] DATETIMEOFFSET NOT NULL, [updatedAt] DATETIMEOFFSET NOT NULL, PRIMARY KEY ([id]));
Executing (default): EXEC sys.sp_helpindex @objname = N'[BooleanTable]';
Executing (default): INSERT INTO [BooleanTable] ([status],[createdAt],[updatedAt]) OUTPUT INSERTED.[id],INSERTED.[status],INSERTED.[createdAt],INSERTED.[updatedAt] VALUES (@0,@1,@2);
Executing (default): SELECT [id], [status], [createdAt], [updatedAt] FROM [BooleanTable] AS [BooleanTable] ORDER BY [BooleanTable].[id] OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY;
{
  id: 1,
  status: true,
  createdAt: 2020-04-09T16:42:28.432Z,
  updatedAt: 2020-04-09T16:42:28.432Z
}

@sushantdhiman sushantdhiman added the status: awaiting response label Apr 9, 2020
@SensitiveMix
Copy link

@SensitiveMix SensitiveMix commented Apr 13, 2020

@sushantdhiman

From the above example, it seems that you may understand the error, you can try the following example:

  (async () => {
    const BooleanTable = sequelize.define(
      "BooleanTable",
      {
        status: {
          type: Sequelize.BOOLEAN,
          defaultValue: true,
          allowNull: false,
        },
      },
      {
        freezeTableName: true,
      }
    );
  
    const value = true;
  
    return BooleanTable.sync({ force: true })
      .then(() => {
        return BooleanTable.create({
          // status: value, // See if it will have defaultValue
        });
      })
      .then(() => BooleanTable.findOne())
      .then(r => console.log(r.toJSON()))
  })();

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 13, 2020

Yes, for both cases defaultValue worked properly (MSSQL)

tedious deprecated The default value for `config.options.trustServerCertificate` will change from `true` to `false` in the next major version of `tedious`. Set the value to `true` or `false` explicitly to silence this message. lib/dialects/mssql/connection-manager.js:63:26
{
  id: 1,
  status: false,
  createdAt: 2020-04-13T04:13:04.803Z,
  updatedAt: 2020-04-13T04:13:04.803Z
}

tedious deprecated The default value for `config.options.trustServerCertificate` will change from `true` to `false` in the next major version of `tedious`. Set the value to `true` or `false` explicitly to silence this message. lib/dialects/mssql/connection-manager.js:63:26
{
  id: 1,
  status: true,
  createdAt: 2020-04-13T04:13:39.525Z,
  updatedAt: 2020-04-13T04:13:39.525Z
}

@SensitiveMix Are you suggesting defaultValue is not working for some dialect?

@mcgradycchen
Copy link
Contributor Author

@mcgradycchen mcgradycchen commented Apr 14, 2020

@SensitiveMix Based on my test, it also doesn't matter to default value.

@sushantdhiman Cloud you tell me the MSSQL server version you tested?
I'm using Azure MSSQL Service, the MSSQL version is below.

mssql> select @@version

--------------------------------------------------------------------------------------
Microsoft SQL Azure (RTM) - 12.0.2000.8
        Feb 26 2020 10:26:43
        Copyright (C) 2019 Microsoft Corporation

Anyway, according to the tedious document, I think it is more suitable to specify the boolean type to TYPES.Bit.

Thanks,
McGrady

@sushantdhiman sushantdhiman merged commit b495d5e into sequelize:master Apr 16, 2020
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants