Update maps returned instance to the wrong model

[a88zach]

Issue Description

When multiple models are wired up to the same table. The model's static update method can incorrectly map the returned instance to the wrong model

What are you doing?

class Foo extends Model {}
Foo.init({ /* bla */ }, {
  tableName: 'table_with_discriminator',
  sequelize
})

class Bar extends Model {}
Bar.init({ /* bla */ }, {
  tableName: 'table_with_discriminator',
  sequelize
})


const updates = await Bar.update(
      { prop: 'value' },
      {
        where: { id: 'xxx },
        returning: true
      }
    );
const updatedInstance = updates[1][0]; // this is an instance of Foo

What do you expect to happen?

The updated instance should be an instance of Bar

What is actually happening?

The updated instance is an instance of Foo

Additional context

The root cause if this line:
https://github.com/sequelize/sequelize/blob/master/lib/query-interface.js#L1040
Which is called from the update method here:
https://github.com/sequelize/sequelize/blob/master/lib/model.js#L3219

The returned instances are being mapped to a model based on the underlying table name. The instance should be mapped to the same model used to call the update method

Environment

• Sequelize version: 5.16.0
• Node.js version: 10.15.3
• Operating System: Mac

Issue Template Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s):
• I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

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

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[papb]

How is it possible to have two tables with the same name?

[a88zach]

It's not that we have 2 tables with the same name. We have 2 models that are each bound to the same table. This is a very common scenario when you have a common table in the db with a discriminator column

[papb]

It's not that we have 2 tables with the same name. We have 2 models that are each bound to the same table. This is a very common scenario when you have a common table in the db with a discriminator column

Hmmm, interesting, I never though about that, can you provide a more detailed example on how you use this kind of idea in a real scenario? (Give an example of discriminator, etc) Thanks

[a88zach]

Think about a read only application based around restaurants where all of the basic restaurant data is the same, but you are storing different types of restaurants all in the same table. You may classify the restaurants as FAST_FOOD, CASUAL, and FINE_DINING and you add a field called restaurant_type to the restaurant table.

Now you could certainly create a Restaurant model in sequelize and apply a filter every time you query the table so that you only get one type of restaurant in the result set.

A better way, IMO, would be to create 3 sequelize models and use a default scope where the default scope would always apply a filter on the restaurant_type column.

By using the restaurant_type column as the discriminator column in the table and using sequelize scopes, you now can deal with each restaurant type as a discrete entity collection even though the underlying data is all populated from the same table.

[papb]

@a88zach I guess it might make sense... But have you considered just using multiple names scopes instead?

const FastFoodRestaurant = Restaurant.scope("FAST_FOOD");
await FastFoodRestaurant.findAll();
// And same idea defining other scopes for the other types

[sushantdhiman]

Interesting usecase, possible fix will be to remove

sequelize/lib/query-interface.js

Line 1040 in 398ae59

const model = _.find(this.sequelize.modelManager.models, { tableName: table.tableName });

altogether and have Model class set options.model whenever using queryInterface.bulkUpdate

[a88zach]

@papb, that, indeed, would solve the issue for the example I gave above. However, that is not my real use case as it's a bit more complicated.

We publish a shared npm package that exposes our data repositories and domain models that are derived from the sequelize models. This package is then consumed in our api and client projects so that the domain models can be used consistently throughout our code.

However, we don't want to expose the sequelize methods to consumers as we only allow certain operations and consumers must go through our repository methods to interact with our database. Those repositories essentially wrap the sequelize methods we want to expose.

For the domain models, we use a custom typing to strip away the sequelize stuff before we export so that we only expose the pojo. This is a Typescript project by the way.

export type RawModel<T extends Model<T>> = Pick<
  T,
  Exclude<keyof T, keyof Model<T>>
> & { id?: string };

export type RecursiveRawModel<T extends Model<T>> = {
  [K in keyof RawModel<T>]: T[K] extends
    | Date
    | number
    | string
    | boolean
    | undefined
    ? T[K]
    : T[K] extends (infer X)[]
    ? X extends Model<X>
      ? RecursiveRawModel<X>[]
      : T[K]
    : T[K] extends (infer Y)[] | undefined
    ? Y extends Model<Y>
      ? RecursiveRawModel<Y>[] | undefined
      : T[K]
    : T[K] extends infer U
    ? U extends Model<U>
      ? RecursiveRawModel<U>
      : T[K]
    : T[K] extends infer V | undefined
    ? V extends Model<V>
      ? RecursiveRawModel<V> | undefined
      : T[K]
    : never
};

We then export our raw domain models like so:
export type AppSetting = RecursiveRawModel<models.AppSetting>;

This means that consumers can only build domain objects using the pojo and do not have access to all the sequelize goodies. This enforces that all consumers go through our data repositories

Our models that are bound to the same database table are also made up of different attributes in some situations, so we cannot just use the scopes or the exposed models would contain properties that do not belong to specific domain models.

However, regardless of the approach, I still feel like this is a bug with the update method that should be fixed. This specific bug only affects the update command and not others like create

[papb]

@a88zach Thanks for the detailed response. Looks complicated indeed. Well, I agree with you, if update is the only one acting differently regarding this, it should be changed.

Can you try sushantdhiman's suggestion above and tell us if it works?

[a88zach]

@papb, the suggestion above is to make a change in the sequelize source code. I'm not set up right now, nor have the bandwidth to make/test changes to the source code.

[papb]

@a88zach I understand, if later you have the time for it let us know.

If someone else is willing to check it as well, it would be great.

[github-actions]

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