rochejul / sequelize-mocking

Sequelize extension to deal with data-mocking for testing
MIT License
63 stars 26 forks source link

Fields UUID not working on creation table #16

Closed SuperToma closed 6 years ago

SuperToma commented 6 years ago

Hello,

In my model definition when I have a field like type: DataTypes.UUID the table creation with the sync() function crashes with : { SequelizeDatabaseError: SQLITE_ERROR: near "BINARY": syntax error

The SQL creation table under sequelize-mocking is : CREATE TABLE IF NOT EXISTSbrands(idCHAR(36) BINARY NOT NULL UNIQUE PRIMARY KEY,

The field id stays as MySQL schema creation (CHAR(36) BINARY) that doesn't seems supported on sqlite (UUID normally).

When I change the field as VARCHAR the table creation is ok.

In the Sequelize documentation : http://docs.sequelizejs.com/manual/tutorial/models-definition.html

Sequelize.UUID // UUID datatype for PostgreSQL and SQLite // CHAR(36) BINARY for MySQL

Any idea ? Regards,

rochejul commented 6 years ago

Hi

Have you got this error when you use basically Sequelize ? If not, could you use Sequelize and Sqlite (instead Mysql) and check if you have the error too ?

If so, that means you have an issue on Sequelize itself which not deal uuid and Sqlite

Have you got some issues onto the Sequelize bug tracker ?

Regards

SuperToma commented 6 years ago

When I use Sequelize with sqlite in my project I don't have this error, the table creation with db.sync() is OK.

The sync function seems KO with the Sequelize mocked object :

SequelizeMocking.createAndLoadFixtureFile() SequelizeMocking.create() ERROR on : return mockedSequelize.sync()

SuperToma commented 6 years ago

I fixed it by forcing the dialect as sqlite during the tests : const db = new Sequelize('sqlite://...

sequelize-mocking seems not override the dialect somewhere.

Thanks !

rochejul commented 6 years ago

@SuperToma Interesting

Could you send me a tiny project to illustrate the issue ? With one test without the dialect forcing and another test with the dialect forcing ?

I think the Sequelize evolution has changed some deep logic and so check too the dialect into the URI (and not only the dialect property)

Many thanks for your feedback

Regards

Embraser01 commented 6 years ago

Hi,

The bug is still here :/

I've been digging a little bit and thats what I found:

The problem is the DataType of a field, it is not reset to sqlite during copy. Except that a DataType is linked to a dialect.

I still don't know why it's not reset, but I think it's because of this function in sequelize:

normalizeDataType(Type) {
    let type = typeof Type === 'function' ? new Type() : Type;
    const dialectTypes = this.dialect.DataTypes || {};

    if (dialectTypes[type.key]) {
      type = dialectTypes[type.key].extend(type);
    }

    if (type instanceof DataTypes.ARRAY && dialectTypes[type.type.key]) {
      type.type = dialectTypes[type.type.key].extend(type.type);
    }
    return type;
  }

I hope it helps !

I'm just starting on sequelize so I don't totally know how it works, maybe it's unrelated :smile: ...

rochejul commented 6 years ago

Hi @Embraser01 That probalby means you should add an issue into the sequelize backlog

I could check this, but if it needs a patch, it means we should fix the sequelize api

Regards

Embraser01 commented 6 years ago

I don't think it's sequelize fault. We just have to create a new DataType depending on SQLite.

I write up an easy fix:

    static copyModel(sequelizeInstance, model) {
        class TempModel extends Sequelize.Model {
        }

        let newModel = TempModel.init(
            _.merge({ }, model.attributes),
            _.merge({ }, model.options, { 'sequelize': sequelizeInstance, 'modelName': model.name })
        );

        // Let's recreate a new instance of the datatype
        for (const att of Object.values(newModel.attributes)) {
            att.type = new Sequelize.DataTypes[att.type.key]();
        }
        return newModel;
    }

    ///////// Or after the mock

   sequelizeInstance.modelManager.all.forEach(model => {
        for (const att of Object.values(model.attributes)) {
            att.type = new DataTypes[att.type.key]();
        }
    });

Regards

rochejul commented 6 years ago

I will have a look on the next days

Many thanks for your feedback

Regards

Loag commented 6 years ago

Just tried @Embraser01 fix and it works/passes tests

rochejul commented 6 years ago

@immexerxez @Embraser01 I will check first if the uuid datatype is basically recognized with sequelize when we start it with a sqlite instance directly.

If so, that probaly means there is a missing call on a setup method, where the datatypes are affected, and maybe something else.

I will take some time in the next days to check if something else is missing and if I found the expected method.

If not, I will apply the below patch.

Many thanks for your feedbacks

Regards

rochejul commented 6 years ago

As the DataTypes are instanciated, using _.merge does not retrieve all what we need :'(

Adapt the fix to works on NodeJs 4.x too (because sequelize should work on NodeJs 4.x):

// Let's recreate a new instance of the datatype
        for (let att of _.values(newModel.attributes)) {
            att.type = new Sequelize.DataTypes[att.type.key]();
        }

I try to deploy a new version asap

Regards