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

Cannot create SQL Server table with a character key #7

Closed
johnsoftek opened this issue Jul 30, 2014 · 11 comments
Closed

Cannot create SQL Server table with a character key #7

johnsoftek opened this issue Jul 30, 2014 · 11 comments
Labels

Comments

@johnsoftek
Copy link

I cannot create a SQL Server table with a character key using automigrate. It works fine with a MySQL datasource. LoopBack 2.0.2, loopback-connector-mssql 1.1.0

 "idInjection": false,
  "properties": {
    "code": {
      "type": "String",
      "id": true,
      "length": 20,
      "nullable": false,
      "generated": false,
      "mysql": {
        "dataType": "varchar",
        "dataLength": 20
      },
      "mssql": {
        "dataType": "varchar",
        "dataLength": 20
      }
    },

I have tried several variations on the above without success.

The automigrate code in mssql.js calls the propertiesSQL function. The column identified as the key is always generated as an int identity column.

MsSQL.prototype.propertiesSQL = function (model) {
  // debugger;
  var self = this;
  var objModel = this._models[model];
  var modelPKID = this.idName(model);

  var sql = ["[" + modelPKID + "] [int] IDENTITY(1,1) NOT NULL"];
  Object.keys(objModel.properties).forEach(function (prop) {
    console.log(prop)
    if (prop === modelPKID) {
      return;
    }
@johnsoftek
Copy link
Author

This mod worked for me:

MsSQL.prototype.propertiesSQL = function (model) {
  // debugger;
  var self = this;
  var objModel = this._models[model];
  var modelPKID = this.idName(model);
  var props = objModel.properties;

  var sql = []
  for (prop in props) {
    if (prop === modelPKID && props[prop].generated) {
      sql.push = "[" + modelPKID + "] [int] IDENTITY(1,1) NOT NULL";
    } else {
      sql.push("[" + prop + "] " + self.propertySettingsSQL(model, prop));
    }
  };

@raymondfeng
Copy link
Contributor

Would you like to submit a patch with a test?

@johnsoftek
Copy link
Author

@raymondfeng

Yes, I would like to work on a test for this problem.

Before making any changes, I ran the current tests and there are a few failures. Are you aware of the errors - or am I doing something wrong, in which case I can supply more details of the error messages.

@raymondfeng
Copy link
Contributor

Most of the tests require a config to our mssql instance.

@johnsoftek
Copy link
Author

I tried the Strongloop server. It errored out.

MS SQL server connector
1) should auto migrate/update tables

discoverModels
Discover models including views
2) should return an array of tables and views
Discover models excluding views
3) should return an array of only tables

Discover models including other users
4) should return an array of all tables and views

Discover model properties
Discover a named model
5) should return an array of columns for product

Discover model primary keys
6) should return an array of primary keys for product
7) should return an array of primary keys for dbo.product

Discover model foreign keys
8) should return an array of foreign keys for inventory
9) should return an array of foreign keys for dbo.inventory

Discover adl schema from a table
10) should return an adl schema for inventory

mssql imported features
datatypes
11) "before all" hook

0 passing (32s)
11 failing

  1. MS SQL server connector should auto migrate/update tables:
    Uncaught RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at Packet.isLast (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\packet.js:103:27)
    at MessageIO.eventData (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js
    :50:18)
    at Socket. (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js:
    3:59)
    at Socket.emit (events.js:95:17)
    at Socket. (stream_readable.js:748:14)
    at Socket.emit (events.js:92:17)
    at emitReadable
    (_stream_readable.js:410:10)
    at emitReadable (_stream_readable.js:406:5)
    at readableAddChunk (_stream_readable.js:168:9)
    at Socket.Readable.push (_stream_readable.js:130:10)
    at TCP.onread (net.js:528:21)

  2. discoverModels Discover models including views should return an array of tables and views:
    Uncaught RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at Packet.isLast (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\packet.js:103:27)
    at MessageIO.eventData (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js
    :50:18)
    at Socket. (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js:
    3:59)
    at Socket.emit (events.js:95:17)
    at Socket. (stream_readable.js:748:14)
    at Socket.emit (events.js:92:17)
    at emitReadable
    (_stream_readable.js:410:10)
    at emitReadable (_stream_readable.js:406:5)
    at readableAddChunk (_stream_readable.js:168:9)
    at Socket.Readable.push (_stream_readable.js:130:10)
    at TCP.onread (net.js:528:21)

  3. discoverModels Discover models excluding views should return an array of only tables:
    Uncaught RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at Packet.isLast (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\packet.js:103:27)
    at MessageIO.eventData (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js
    :50:18)
    at Socket. (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js:
    3:59)
    at Socket.emit (events.js:95:17)
    at Socket. (stream_readable.js:748:14)
    at Socket.emit (events.js:92:17)
    at emitReadable
    (_stream_readable.js:410:10)
    at emitReadable (_stream_readable.js:406:5)
    at readableAddChunk (_stream_readable.js:168:9)
    at Socket.Readable.push (_stream_readable.js:130:10)
    at TCP.onread (net.js:528:21)

  4. Discover models including other users should return an array of all tables and views:
    Uncaught RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at Packet.isLast (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\packet.js:103:27)
    at MessageIO.eventData (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js
    :50:18)
    at Socket. (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js:
    3:59)
    at Socket.emit (events.js:95:17)
    at Socket. (stream_readable.js:748:14)
    at Socket.emit (events.js:92:17)
    at emitReadable
    (_stream_readable.js:410:10)
    at emitReadable (_stream_readable.js:406:5)
    at readableAddChunk (_stream_readable.js:168:9)
    at Socket.Readable.push (_stream_readable.js:130:10)
    at TCP.onread (net.js:528:21)

  5. Discover model properties Discover a named model should return an array of columns for product:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout
    /

  6. Discover model primary keys should return an array of primary keys for product:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout

  7. Discover model primary keys should return an array of primary keys for dbo.product:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout

  8. Discover model foreign keys should return an array of foreign keys for inventory:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout

  9. Discover model foreign keys should return an array of foreign keys for dbo.inventory:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout

  10. Discover adl schema from a table should return an adl schema for inventory:
    Error: timeout of 5000ms exceeded
    at null. (E:\Temp\loopback-connector-mssql\node_modules\mocha\lib\runnable.js:139:19)
    at Timer.listOnTimeout as ontimeout

  11. mssql imported features datatypes "before all" hook:
    Uncaught RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at Packet.isLast (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\packet.js:103:27)
    at MessageIO.eventData (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js
    :50:18)
    at Socket. (E:\temp\loopback-connector-mssql\node_modules\mssql\node_modules\tedious\lib\message-io.js:
    3:59)
    at Socket.emit (events.js:95:17)
    at Socket. (stream_readable.js:748:14)
    at Socket.emit (events.js:92:17)
    at emitReadable
    (_stream_readable.js:410:10)
    at emitReadable (_stream_readable.js:406:5)
    at readableAddChunk (_stream_readable.js:168:9)
    at Socket.Readable.push (_stream_readable.js:130:10)

at TCP.onread (net.js:528:21)

npm ERR! Test failed. See above for more details.
npm ERR! not ok code 0

@aams
Copy link

aams commented Aug 19, 2014

I have checked a little further and the problem is much more serious than I first thought.

The SQL Server connector follows an assumption that every table will have an id column and that column will be an identity column. For example, this is the very simple create function i.e. new record:

MsSQL.prototype.create = function (model, data, callback) {
  //debugger;
  var fieldsAndData = this.buildInsert(model, data);
  var tblName = this.tableEscaped(model);
  var sql = "INSERT INTO " + tblName + " (" + fieldsAndData.fields + ")" + MsSQL.newline;
  sql += "VALUES (" + fieldsAndData.paramPlaceholders + ");" + MsSQL.newline;
  sql += "SELECT IDENT_CURRENT('" + tblName + "') AS insertId;";

  this.query(sql, fieldsAndData.params, function (err, results) {
    if (err) {
      return callback(err);
    }
    //msnodesql will execute the callback for each statement that get's executed, we're only interested in the one that returns with the insertId
    if (results.length > 0 && results[0].insertId) {
      callback(null, results[0].insertId);
    }
  });
};

You can see that the id of the inserted column is found by using by selecting the ident_current property of the table.

One of the consequences of this implementation decision is the effect on the AccessToken table.In this table, the id column is defined as a string to hold a random authorization token that can be exchanged and identify a logged in user. The table is created with an id column int identity(1,1). So each authorization value is 1, 2, 3 etc rather than a typical value in MwSQL of "H2mtuTCnGgexirEEFh3TMxg6VfECCBZaEjzHxNXQE4zULQer2jTtLrPQtsr1myo7". Not very secret!

I tested a workaround for the create table part of mssql.js, but the ramifications are much wider and rather beyond me to fix.

In my opinion the SQL Server connector is quite broken by this problem.

@FoysalOsmany
Copy link
Contributor

@johnsoftek @raymondfeng @aams I am still having similar issues is this fixed?

I am trying to save/update a model with some attribute changes where I do not have any ID property. But when I am debugging the datasource-juggler and getting a SQL like this:

INSERT INTO [dbo].LastUsedID
VALUES (59);
SELECT IDENT_CURRENT('[dbo].[LastUsedID]') AS pkid;

Is it natural I think I should get an update query?

@sashasochka
Copy link
Contributor

The AccessToken table (which was created automatically through this code)

app.dataSources.db.autoupdate(err => {
  if (err) throw err;
  console.log('Models autoupdated');
  cb();
});

has the following schema:


CREATE TABLE [dbo].[AccessToken] (
    [id]      UNIQUEIDENTIFIER NOT NULL,
    [ttl]     INT              NULL,
    [created] DATETIME         NULL,
    [userId]  INT              NULL
);

And this doesn't work with inserts due to the fact that IDs are actually strings as @aams noted! Have I done something wrong or that's the issue everybody had?

@peebles
Copy link

peebles commented Apr 8, 2017

Show stopper for me as well. I think it is not uncommon now-a-days to use UID strings as primary ID keys, so that client side users of the database cannot "predict" other user ids (for example) based on their user id. If I am user ID 4, then there might be a user with an ID of 5 and I can try to probe that. I built my whole application using:

{
   "id": {
      "type": "string",
      "id": true,
      "defaultFn": "shortid"
  }
}

and tested and deployed with MySQL, Postgres and Mongo ... then a client comes along needing MSSQL and it doesn't work.

@stale
Copy link

stale bot commented Aug 22, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this as completed Sep 5, 2017
@stale
Copy link

stale bot commented Sep 5, 2017

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants