overlookmotel / sequelize-hierarchy

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

`underscored` option has wrong behavior in Sequelize v5 #171

Open bravo-kernel opened 5 years ago

bravo-kernel commented 5 years ago

I hope this is not another silly configuration error on my behalf but it looks like the Sequelize 5 underscored mode I am using is not respected for the virtual fields.

image

All pointer appreciated

overlookmotel commented 5 years ago

Hi @bravo-kernel.

I can look into this.

Can you please post your code for:

  1. Sequelize initialization (i.e. sequelize = new Sequelize( ... ))
  2. Model definition
bravo-kernel commented 5 years ago

Ofcourse, highly appreciated.

sequelize.js ```js const Sequelize = require('sequelize'); module.exports = function (app) { const connectionString = app.get('mysql'); const sequelize = new Sequelize(connectionString, { dialect: 'mysql', logging: false, define: { underscored: true, // translates all camelCased model property names to snake_cased database field names (and vice versa) freezeTableName: true } }); const oldSetup = app.setup; app.set('sequelizeClient', sequelize); app.setup = function (...args) { const result = oldSetup.apply(this, args); // Set up data relationships const models = sequelize.models; Object.keys(models).forEach(name => { if ('associate' in models[name]) { models[name].associate(models); } }); // Sync to the database sequelize.sync(); return result; }; }; ```
folder.js ```js // See http://docs.sequelizejs.com/en/latest/docs/models-definition/ // for more of what you can do here. const Sequelize = require('sequelize'); require('sequelize-hierarchy')(Sequelize); const DataTypes = Sequelize.DataTypes; module.exports = function (app) { const sequelizeClient = app.get('sequelizeClient'); const folders = sequelizeClient.define('folders', { id: { type: DataTypes.UUID, defaultValue: DataTypes.UUIDV4, allowNull: false, primaryKey: true }, name: DataTypes.STRING, createdAt: DataTypes.DATE, updatedAt: DataTypes.DATE, }, { hooks: { beforeCount(options) { options.raw = true; } } }); folders.isHierarchy(); // test hierarchy for a given root node async function asyncCall() { let result = await folders.findOne({ where: { name: 'userA' }, include: { model: folders, as: 'descendents', hierarchy: true } }); console.log(JSON.stringify(result, null, 4)); } asyncCall(); // eslint-disable-next-line no-unused-vars folders.associate = function (models) { // Define associations here // See http://docs.sequelizejs.com/en/latest/docs/associations/ }; return folders; }; ```
overlookmotel commented 5 years ago

Ah thanks. So are you saying underscored: true should mean the model attributes should be called parentId and hierarchyLevel, but the fields in database table should be parent_id and hierarchy_level?

The brief mention of underscored option in Sequelize docs here, this seem to suggest that.

In which case, yes, this is a bug.

Would you be willing to submit a PR to fix this? NB this plugin supports Sequelize v2-v5, so a fix would need to selectively change the behavior only for Sequelize v5+, and include tests for all Sequelize versions.

bravo-kernel commented 5 years ago

Exactly, that's how the v5 underscored behavior works (even though I had to re-read it a 1000 times and had to sanity-check using code).

To be honest, a PR is a bit out of my skill league at this point, I just started pounding away at sequelize/js yesterday and don't even have a frontend yet so perhaps better to label this as a bug in case somebody else wants to pick up the challenge. I will keep using the underscored properties for now. When time comes (and this starts blocking me) I might give it a shot.

overlookmotel commented 5 years ago

OK cool. I've labelled it as a bug.

But I don't use underscored myself, so I'm not going to fix it.

Sorry if this seems like rather careless maintenance, but I am busy with other projects and this module has become a bit of a burden. So I'm open to receiving PRs, but I'm not able to put a lot of work in myself.

overlookmotel commented 5 years ago

To get what you need in the meantime, you can always define the parentId and hierarchyLevel attributes yourself in the model definition, and then use the levelFieldName and foreignKey hierarchy options to point sequelize-hierarchy to use them rather than make its own.

bravo-kernel commented 5 years ago

Totally understandable and you are absolutely right. If you ask me... we should all be glad that you are still willing to review and comment. Take it easy.

bravo-kernel commented 5 years ago

I can confirm the attribute mapping works perfectly (as a workaround until the bug gets fixed).

  folders.isHierarchy({
    foreignKey: 'parentId',
    levelFieldName: 'hierarchyLevel'
  });
papb commented 4 years ago

Related: sequelize#11225