sequelize / sequelize

Feature-rich ORM for modern Node.js and TypeScript, it supports PostgreSQL (with JSON and JSONB support), MySQL, MariaDB, SQLite, MS SQL Server, Snowflake, Oracle DB (v6), DB2 and DB2 for IBM i.
https://sequelize.org/
MIT License
29.54k stars 4.27k forks source link

Duplicate column names for associations and misleading documentation #9328

Open atomanyih opened 6 years ago

atomanyih commented 6 years ago

Summary

We ran into an issue recently where sequelize generates queries with duplicate column names that cause mysql to errors in subqueries. A similar issue documented here: https://github.com/sequelize/sequelize/issues/3035 allowed us to fix the issue by specifying the foreignKey. However this appears to be in conflict with the documentation which suggests that foreignKey is camelCase by default

It seems like sequelize has mismatched casing assumptions: association definitions assume camelCase while the query builder assumes UpperCamelCase

Longer explanation

We have three models, Basket, Apple, Farmer with associations as defined below:

Basket.belongsTo(models.Farmer)
Basket.hasMany(models.Apple)
Farmer.hasMany(models.Basket)

If we findAll

Basket.findAll()

It generates the following query. (note that farmerId is duplicated)

SELECT `id`,
       `farmerId`,
       `FarmerId`
FROM   `Baskets` AS `Basket`;

If we instead want only 10 Baskets, plus any Apples in each Basket:

Basket.findAll({
  limit: 10,
  include: [
    {
      model: Apple
    }
  ]
})

It generates the following query with a subquery

SELECT `Basket`.*,
       `Apples`.`id`    AS `Apples.id`,
FROM   (SELECT `Basket`.`id`,
               `Basket`.`farmerId`,
               `Basket`.`FarmerId`
        FROM   `Baskets` AS `Basket`
        LIMIT  10) AS `Basket`
       LEFT OUTER JOIN `Apples` AS `Apples`
                    ON `Basket`.`id` =
`Apples`.`BasketId`;

Our duplicated columns are now in a subquery. When this query is run in mysql, it throws an error!

SequelizeDatabaseError: Duplicate column name 'FarmerId'

We can fix this by specifying the foreign key on our relationships:

Basket.belongsTo(models.Farmer, { foreignKey: 'farmerId' })
Basket.hasMany(models.Apple)
Farmer.hasMany(models.Basket, { foreignKey: 'farmerId' })

However, the documentation suggests that foreignKey should already be camelCase by default. It's not super clear to me why the query builder would assume UpperCamelCase. This seems like a bug.

Info

Dialect: mysql, (generates the same queries for sqlite, but doesn't cause an error) Dialect version: default? Database version: mysqld Ver 5.7.21 for osx10.12 on x86_64 (Homebrew) Sequelize version: 4.17.0 and 5.0.0-beta.3

sushantdhiman commented 6 years ago

UpperCamelCase attributes are generated because model name is defined with upper case like

const Basket = sequelize.define('Basket', { .... });

Well its up for discussion, @sequelize/orm what do you suggest here? Enforce proper camel case key even if model name is upper cased?

I think many user prefer PascalCase naming for model, but camel case for attributes name. It make sense to keep attributes camelCased not PascalCased

heisian commented 6 years ago

I think the implication is that the foreign key column should assume that it is in camelCase even if the model is in PascalCase, which I think is a logical assumption.

parkerdan commented 6 years ago

Yeah...not the best bug to try and sort out. I guess I will just be explicit in all model associations

sangaman commented 5 years ago

Same issue here, I was getting the Duplicate column name errors for PascalCase properties after adding several associations to existing models where I didn't explicitly specify foreignKey because it seemed like the default value should have been fine based on documentation. The solution for me as well was to go and explicitly specify each foreignKey using camelCase.

HansHammel commented 5 years ago

still an issue

broox commented 4 years ago

i just ran into this duplicate column bug after updating from sequelize 4.44.4 to 5.21.5. the solution to explicitly define the foreignKey works, but it's kinda gross that you have to do that.

crypto-titan commented 4 years ago

@broox thank you for your help, I just downgraded my Sequelize version to actually fix this issue. @sequelize you should fix this seriously...

saeidfiy commented 4 years ago

thank u , i have this issue , it help me

kingsleyramos commented 4 years ago

I tried to downgrade to 4.44.4, currently on 5.21.3, also tried 6.3.3, I'm still having the same issue for all three versions

afsaneGhafouri commented 4 years ago

I am using version 5.18.4 and I have the same issue. 😞

matiasperz commented 4 years ago

Every time I ran into this kind of problem, either I use one of these 2 options into the query config:

Somehow, most of the time it ends up working.

ruofanwei commented 3 years ago

Every time I ran into this kind of problem, either I use one of these 2 options into the query config:

  • subQuery: false
  • distinct: true

Somehow, most of the time it ends up working.

thanks!!