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

Update not using camelCase field names to snake_case for instance.update() call #13897

Open
3 tasks done
th1nkful opened this issue Jan 4, 2022 · 8 comments
Open
3 tasks done
Assignees

Comments

@th1nkful
Copy link

th1nkful commented Jan 4, 2022

Issue Creation Checklist

Bug Description

SSCCE

Here is the link to the SSCCE for this issue: sequelize/sequelize-sscce#213

What do you expect to happen?

I expected the query to work update the row

What is actually happening?

I get an error that datasetId is an unknown column, which is correct because the database is in snake_case

Unknown column 'datasetId' in 'where clause'
sql: UPDATE `MarketDatasets` SET `disabled`=? WHERE `datasetId` = ? AND `marketId` = ? - parameters:[false,3,1]

Additional context

  • Appears to be related to lib/utils.js -> mapWhereFieldNames
  • I believe the offending commit is: 1a16b91
  • Versions including and below 6.6.2 work as expected, after this it stops working
  • Locally, if I undo the change in the commit (by commenting out the cloneDeep) the code works again
  • Other update calls WORK, so this appears to be super specifically related to the conditions of the model/Sequelize configuration

Environment

  • Sequelize version: >= 6.6.4 (6.6.2 is unaffected, I tried most versions of 6.x.x up to latest and it is still an issue)
  • Node.js version: 12.22.7
  • If TypeScript related: n/a

Bug Report Checklist

How does this problem relate to dialects?

  • I don't know, I was using mariadb, with connector library version 2.5.5 and database version 10.2

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time but I don't know how to start, I would need guidance.
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

@github-actions github-actions bot added the stale label Jan 19, 2022
@th1nkful
Copy link
Author

Is there anything I can do here to help? I would be more than happy to help but have no idea where to start. Seems like another part of the sequelize codebase was working based on the fact that before the linked commit, the where attributes were mutated.

@ephys
Copy link
Member

ephys commented Jan 19, 2022

If cloning causes an issue, it must mean some part of the code was relying on a side-effect which is very annoying :/

A good starting point would be to open a PR with a failing test. But the next step is figuring out which part of the code fails, which is going to take some time unfortunately.

Your Model does not have a Primary Key, that's probably relevant to the issue

@ephys ephys removed the stale label Jan 19, 2022
@th1nkful
Copy link
Author

In the application I'm getting the issue, there is a primary key added when the table is created by a migration:

   await queryInterface.createTable('MarketDatasets', {
      disabled: {
        type: DataTypes.BOOLEAN,
        defaultValue: false,
      },
      market_id: {
        type: DataTypes.INTEGER,
        primaryKey: true,
        references: { model: 'Markets', key: 'id' },
      },
      dataset_id: {
        type: DataTypes.INTEGER,
        primaryKey: true,
        references: { model: 'Datasets', key: 'id' },
      },
      created_at: {
        allowNull: false,
        type: DataTypes.DATE,
        defaultValue: DataTypes.literal('CURRENT_TIMESTAMP'),
      },
      updated_at: {
        allowNull: false,
        type: DataTypes.DATE,
        defaultValue: DataTypes.literal('CURRENT_TIMESTAMP'),
      },
    }, {
      charset: 'utf8mb4',
      collate: 'utf8mb4_general_ci',
    });

Just to add to the context, the model looks like this:

module.exports = (sequelize, DataTypes) => {
  class MarketDatasets extends sequelize.Sequelize.Model { }

  MarketDatasets.init(
    {
      disabled: {
        type: DataTypes.BOOLEAN,
        defaultValue: false,
      },
    },
    {
      sequelize,
      timestamps: false,
    },
  );

  MarketDatasets.removeAttribute('id');

  MarketDatasets.associate = ({
    Markets,
    Datasets,
  }) => {
    MarketDatasets.belongsTo(Markets, {
      foreignKey: 'marketId',
      onDelete: 'cascade',
      onUpdate: 'cascade',
    });

    MarketDatasets.belongsTo(Datasets, {
      foreignKey: 'datasetId',
      onDelete: 'cascade',
      onUpdate: 'cascade',
    });
  };

  return MarketDatasets;
};

@th1nkful
Copy link
Author

I can't see anything specific in the documentation, what would the best place for the test be? Happy to write one up.

Would the Foo/Bar representation from the SSCCE be the better use in the test or something closer resembling what I am actually using?

@ephys
Copy link
Member

ephys commented Jan 20, 2022

Currently your SSCCE fails with parameter "id" has invalid "undefined" value because you're removing the primary key of your model in it. If you can update your SSCCE to make it fail with the proper error, I can turn it into a test :)

@th1nkful
Copy link
Author

th1nkful commented Jan 21, 2022

Thanks for getting back to me @ephys, taking a look now (to try fix) I can see error is only for v5, the issue exists in v6 (I hadn't tried v5 as the app was built for v6)

Looking at the SSCCE logs for v6 tests (not TypeScript ones) I am getting the correct error.

From the MySQL 5.8 (v6) results:

original: Error: Unknown column 'fooId' in 'where clause'

Appears to be the same across all dialects in v6 tests.

Did you want an SSCCE that gets the right error in v5 and v6 or is just the v6 one okay?
Not really sure how I'd get it to occur in v5 considering it works in v6

@ephys
Copy link
Member

ephys commented Jan 21, 2022

Oh that's my bad, I didn't realise the SSCCE was still testing v5 too. Definitely something fishy going on in v6 here

@ephys ephys self-assigned this Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants