search

overlookmotel / sequelize-hierarchy

Nested hierarchies for Sequelize
MIT License
300 stars 92 forks source link

Microsoft SQL Server support #124

Open leroydev opened 6 years ago

leroydev commented 6 years ago

For me personally, it's not really clear if sequelize-hierarchy supports Microsoft SQL Server or not (and if not, why it's not supported).

In the README is the following text:

Works with all dialects of SQL supported by Sequelize (MySQL, Postgres, SQLite) except for Microsoft SQL Server.

But there's also instructions on how to test with MSSQL:

To run tests on a particular database, use npm run test-mysql, npm run test-postgres, npm run test-postgres-native, npm run test-sqlite or npm run test-mssql.

Those two give off a bit of a conflicting message in my opinion.

I did find the commit in which MSSQL support was removed, but it unfortunately doesn't include a reason for the removal.

It might be good to add to the README why MSSQL isn't supported anymore, as I'm now wondering if it's not being tested for compatibility or if it's really not working with MSSQL.

overlookmotel commented 6 years ago

Hi @leroydev. Nice to hear from you.

I can't actually remember why I removed SQL Server support. I think one of the tests failed on MS SQL and I thought "well no-one is using MS SQL anyway, so might as well drop it." Seems I was wrong!

I've just created a branch to run the tests for MS SQL on Travis: https://travis-ci.org/overlookmotel/sequelize-hierarchy/builds/253938155

Let's see what fails...

leroydev commented 6 years ago

@overlookmotel I'm using PostgreSQL at the moment, but having MSSQL open as an option would be nice.

Tests are failing at the moment because of a connection timeout:

SequelizeConnectionError: Failed to connect to mssql.sequelizejs.com:11433 in 15000ms

If there's incompatibilities, I'd be willing to try and fix them. I've got MSSQL running locally anyways. :)

overlookmotel commented 6 years ago

I updated the MS SQL credentials, coping those in Sequelize. But it's still failing (https://travis-ci.org/overlookmotel/sequelize-hierarchy/builds/254123040). There isn't a public instance of MS SQL to run against any more, it seems.

Looks like Sequelize only runs it's MS SQL tests on Appveyor and Appveyor provides the MS SQL instance (see https://github.com/sequelize/sequelize/blob/master/appveyor-setup.ps1). So for sequelize-hierarchy to support MS SQL would require setting up Appveyor, which I have no idea how to do.

Further, I suspect changes would be required to the sequelize-hierarchy codebase to some of the raw SQL queries in e.g. the beforeUpdate hook to make them work on MS SQL. Even if we got that work done, it also adds a maintenance overhead in future.

I suspect that very few people use (or want to use) MS SQL so I'm not sure whether all of the above work is justified. Certainly, for me it's not as I never use MS SQL.

However, if you really want it, and are prepared to do the legwork to make it work, then please be my guest.

I guess the first step would be to run the tests against your local MS SQL instance and see if tests fail. If you do that, please let me know the results.

aguerere commented 6 years ago

@overlookmotel I ran the tests in my MSSQL instance in Azure and got this. Any advise or help would be appreciated. Thanks


> cross-env DIALECT=mssql npm run test-main

> sequelize-hierarchy@1.3.0 test-main /Users/beto/temp/sequelize-hierarchy
> mocha --check-leaks --colors -t 30000 -R spec "test/**/*.test.js"

Sequelize version: 4.4.0
Dialect: mssql

  [MSSQL] Tests
    Hierarchy creation
      ✓ works via isHierarchy()
      ✓ works via define options
(sequelize) Warning: MSSQL does not support INTEGER with options. Plain `INTEGER` will be used instead., '
>> Check:', https://msdn.microsoft.com/en-us/library/ms187752%28v=sql.110%29.aspx
      ✓ works via defining in model attribute
      ✓ allows parentId and hierarchyLevel fields to already be defined
      options
        ✓ `through`
        ✓ `throughTable`
        ✓ `throughSchema`
    Methods
      #create
        for root level
          ✓ sets hierarchyLevel (476ms)
          ✓ creates hierarchy table records (295ms)
        for 2nd level
          ✓ sets hierarchyLevel (357ms)
          ✓ creates hierarchy table records (295ms)
        for 3rd level
          ✓ sets hierarchyLevel (318ms)
          ✓ creates hierarchy table records (709ms)
        throws
          ✓ if creating child of itself (362ms)
      #updateAttributes
        for root level
          ✓ sets hierarchyLevel (340ms)
          ✓ updates hierarchy table records (311ms)
        for 2nd level
          ✓ sets hierarchyLevel (418ms)
          ✓ updates hierarchy table records (330ms)
        for 3rd level
          ✓ sets hierarchyLevel (298ms)
          ✓ updates hierarchy table records (324ms)
        descendents
          ✓ sets hierarchyLevel for descendents (502ms)
          ✓ updates hierarchy table records for descendents (1245ms)
        throws
          ✓ if making item child of itself
          ✓ if making item child of one of its own descendents (385ms)
      #destroy
        ✓ removes hierarchy table records (889ms)
        1) throws error if try to destroy a record which has children
      #bulkCreate
        ✓ sets hierarchyLevel for all rows (857ms)
        ✓ creates hierarchy table records for all rows (836ms)
      #bulkUpdate
        ✓ sets hierarchyLevel for all rows (662ms)
        ✓ creates hierarchy table records for all rows (1770ms)
      #bulkDestroy
        ✓ removes hierarchy table records for all rows (1380ms)
        2) throws error if try to destroy a record which has children
      #find
        can retrieve
          ✓ children (343ms)
          ✓ descendents (380ms)
          ✓ parent (345ms)
          ✓ ancestors (561ms)
          ✓ other associations (365ms)
        with hierarchy option
          ✓ findAll gets a structured tree (613ms)
          ✓ find gets a structured tree (649ms)
          ✓ find gets a structured tree when included from another model (391ms)
          ✓ find gets a structured tree when included from another model 2 deep (763ms)
          ✓ works with `raw` option (314ms)
        works with scoped models
          ✓ with main model scoped (446ms)
          ✓ with hierarchy model scoped (337ms)
          ✓ with both models scoped (931ms)
        handles empty result set with
          ✓ #find (438ms)
          ✓ #findAll (397ms)
          ✓ #find with an include (342ms)
          ✓ #find with an empty include (347ms)
          ✓ #find with empty descendents (357ms)
      accessors
        can retrieve
          ✓ children (345ms)
          ✓ descendents (505ms)
          ✓ parent (336ms)
          ✓ ancestors (362ms)
        with hierarchy option
          ✓ getDescendents gets a structured tree (379ms)
      setters
        legal
          ✓ setParent (3901ms)
          ✓ addChild (3241ms)
          createParent
            creates new item
              ✓ with correct parent
              ✓ with correct hierarchyLevel
              ✓ with correct hierarchy table entries (409ms)
            sets current item
              ✓ to correct parent (312ms)
              ✓ with correct hierarchyLevel (344ms)
              ✓ with correct hierarchy table entries (367ms)
          createChild
            ✓ with correct parent
            ✓ with correct hierarchyLevel
            ✓ with correct hierarchy table entries (371ms)
        illegal methods not present
          ✓ setDescendents
          ✓ setAncestors
          ✓ addDescendent
          ✓ addAncestor
          ✓ addDescendents
          ✓ addAncestors
          ✓ createDescendent
          ✓ createAncestor
          ✓ removeDescendent
          ✓ removeAncestor
          ✓ removeDescendents
          ✓ removeAncestors
      #rebuildHierarchy
        ✓ recreates hierarchyLevel for all records (733ms)
        ✓ recreates hierarchy table records (537ms)
    Options
      underscored
        ✓ underscores field names and foreign keys
        ✓ does not override user supplied field names
      underscoredAll
        ✓ underscores through table name
        ✓ does not override user supplied `throughTable` option

  82 passing (23m)
  2 failing

  1) [MSSQL] Tests Methods #destroy throws error if try to destroy a record which has children:

      AssertionError: expected promise to be rejected with 'ForeignKeyConstraintError' but it was rejected with 'SequelizeDatabaseError: The DELETE statement conflicted with the SAME TABLE REFERENCE constraint "FK__folders__parentI__756D6ECB". The conflict occurred in database "sequelize_hierarchy_tests", table "dbo.folders", column \'parentId\'.'
      + expected - actual

      -SequelizeDatabaseError: The DELETE statement conflicted with the SAME TABLE REFERENCE constraint "FK__folders__parentI__756D6ECB". The conflict occurred in database "sequelize_hierarchy_tests", table "dbo.folders", column 'parentId'.
      +ForeignKeyConstraintError

      at getBasePromise.then.reason (node_modules/chai-as-promised/lib/chai-as-promised.js:206:30)
  From previous event:
      at Proxy.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:169:53)
      at Proxy.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:47:29)
      at Proxy.methodWrapper (node_modules/chai/lib/chai/utils/addMethod.js:57:25)
      at Context.<anonymous> (test/all.test.js:389:34)

  2) [MSSQL] Tests Methods #bulkDestroy throws error if try to destroy a record which has children:

      AssertionError: expected promise to be rejected with 'ForeignKeyConstraintError' but it was rejected with 'SequelizeDatabaseError: The DELETE statement conflicted with the SAME TABLE REFERENCE constraint "FK__folders__parentI__345EC57D". The conflict occurred in database "sequelize_hierarchy_tests", table "dbo.folders", column \'parentId\'.'
      + expected - actual

      -SequelizeDatabaseError: The DELETE statement conflicted with the SAME TABLE REFERENCE constraint "FK__folders__parentI__345EC57D". The conflict occurred in database "sequelize_hierarchy_tests", table "dbo.folders", column 'parentId'.
      +ForeignKeyConstraintError

      at getBasePromise.then.reason (node_modules/chai-as-promised/lib/chai-as-promised.js:206:30)
  From previous event:
      at Proxy.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:169:53)
      at Proxy.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:47:29)
      at Proxy.methodWrapper (node_modules/chai/lib/chai/utils/addMethod.js:57:25)
      at Context.<anonymous> (test/all.test.js:487:34)

npm ERR! Darwin 16.6.0
npm ERR! argv "/Users/beto/.nvm/versions/node/v6.11.0/bin/node" "/Users/beto/.nvm/versions/node/v6.11.0/bin/npm" "run" "test-main"
npm ERR! node v6.11.0
npm ERR! npm  v3.10.10
npm ERR! code ELIFECYCLE
npm ERR! sequelize-hierarchy@1.3.0 test-main: `mocha --check-leaks --colors -t 30000 -R spec "test/**/*.test.js"`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the sequelize-hierarchy@1.3.0 test-main script 'mocha --check-leaks --colors -t 30000 -R spec "test/**/*.test.js"'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the sequelize-hierarchy package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     mocha --check-leaks --colors -t 30000 -R spec "test/**/*.test.js"
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs sequelize-hierarchy
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls sequelize-hierarchy
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/beto/temp/sequelize-hierarchy/npm-debug.log

npm ERR! Darwin 16.6.0
npm ERR! argv "/Users/beto/.nvm/versions/node/v6.11.0/bin/node" "/Users/beto/.nvm/versions/node/v6.11.0/bin/npm" "run" "test-mssql"
npm ERR! node v6.11.0
npm ERR! npm  v3.10.10
npm ERR! code ELIFECYCLE
npm ERR! sequelize-hierarchy@1.3.0 test-mssql: `cross-env DIALECT=mssql npm run test-main`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the sequelize-hierarchy@1.3.0 test-mssql script 'cross-env DIALECT=mssql npm run test-main'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the sequelize-hierarchy package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     cross-env DIALECT=mssql npm run test-main
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs sequelize-hierarchy
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls sequelize-hierarchy
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/beto/temp/sequelize-hierarchy/npm-debug.log```
overlookmotel commented 6 years ago

@aguerere Thanks for running the tests. Looks to me like everything is functional. The failing tests appear to be due to Sequelize with MS SQL returning a different error from what you get with e.g. MySQL.

I'd say actually this is a bug in Sequelize. The errors that get returned for a foreign key constraint violation should be consistent regardless of what database you're using.

At a guess the error is not being converted to a ForeignKeyConstraintError because the table relation in question is a self-join i.e. Folder.parentId being linked to another field Folder.id in the same table. This is a little unusual so maybe Sequelize hasn't been taught to recognise that the error message "The DELETE statement conflicted with the SAME TABLE REFERENCE constraint..." is a foreign key violation as most errors of this form won't have the word "SAME" in them.

If you have the appetite to get this fixed, I'd suggest best thing is to raise an issue on Sequelize repo and (even better) see if you guys, since you're using MS SQL, can make a fix. If my guess about what's wrong is correct, it should be pretty trivial to fix.

Then we can run sequelize-hierarchy tests again and make sure everything passes.

But the good news is that it looks like sequelize-hierarchy should work fine on MS SQL.

overlookmotel commented 6 years ago

If either of you are willing to try to fix Sequelize for this and set up / tell me how to set up Appveyor to run the tests on MS SQL, I'd be willing to officially support MS SQL from here on.

Please advise - otherwise I'm afraid I'm not going to have time to help further with this.

nengine commented 5 years ago

Hello, I like to use with MS SQL, please let me know it is working now. Also is this going to work with sequelize v5? Thanks.

overlookmotel commented 5 years ago

@nengine v2.0.0 just released does support Sequelize v5.

I have no plans to support MS SQL. It's not popular, so doesn't seem worth the time to figure out implementation.

If you want to put in the time to do it, I'll be happy to receive a PR, but please be aware that I'm not going to spend time on it myself answering questions etc. You'd have to do the legwork.

nengine commented 5 years ago

Hello, Thank you. I did on pg instead and got the following errors. Any suggestion is appreciated.

folder.js

var { pg, DataTypes, Model } = require('./pg_driver');
class Folder extends Model {}
Folder.init({ 
    name: { type: DataTypes.STRING },
    parentId: {
        type: DataTypes.INTEGER,
        hierarchy: true
      }
 }, {    
    sequelize: pg
    })
module.exports = Folder;

app.js


  await Folder.sync()
  await Folder.isHierarchy()

  var fa = await Folder.create({name: 'a'})
  var fab = await Folder.create({name: 'ab', parentId: 1})
  var fac =  await Folder.create({name: 'ac', parentId: 1})

  const folders = await Folder.findAll({ hierarchy: true });
  console.log(folders)

error

Executing (default): CREATE TABLE IF NOT EXISTS "Folders" ("id"   SERIAL , "name" VARCHAR(255), "parentId" INTEGER REFERENCES "Folders" ("id") ON DELETE RESTRICT ON UPDATE CASCADE, "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL, "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL, "hierarchyLevel" INTEGER, PRIMARY KEY ("id"));
Executing (default): SELECT i.relname AS name, ix.indisprimary AS primary, ix.indisunique AS unique, ix.indkey AS indkey, array_agg(a.attnum) as column_indexes, array_agg(a.attname) AS column_names, pg_get_indexdef(ix.indexrelid) AS definition FROM pg_class t, pg_class i, pg_index ix, pg_attribute a WHERE t.oid = ix.indrelid AND i.oid = ix.indexrelid AND a.attrelid = t.oid AND t.relkind = 'r' and t.relname = 'Folders' GROUP BY i.relname, ix.indexrelid, ix.indisprimary, ix.indisunique, ix.indkey ORDER BY i.relname;
{ SequelizeAssociationError: You have used the alias children in two separate associations. Aliased associations must have unique aliases.
    at new Association (C:\Users\nash\workspace\nodeapps\oma-dpo\node_modules\sequelize\lib\associations\base.js:106:13)
    at new HasMany (C:\Users\nash\workspace\nodeapps\oma-dpo\node_modules\sequelize\lib\associations\has-many.js:19:5)
    at Function.hasMany (C:\Users\nash\workspace\nodeapps\oma-dpo\node_modules\sequelize\lib\associations\mixin.js:34:25)
    at Function.isHierarchy (C:\Users\nash\workspace\nodeapps\oma-dpo\node_modules\sequelize-hierarchy\lib\modelExtends.js:101:9)
    at Object.exports.Tree (C:\Users\nash\workspace\nodeapps\oma-dpo\seed\data.js:48:16) name: 'SequelizeAssociationError' }
overlookmotel commented 5 years ago

@nengine Hi. Can you please open this as a separate issue, as it has nothing to do with Microsoft SQL Server?

Also please specify what version of sequelize-hierarchy and sequelize, and also provide the code from ./pg_driver.