sequelize-typescript icon indicating copy to clipboard operation
sequelize-typescript copied to clipboard

Non-static update method of the Models returns old value

Open BjarkeNL opened this issue 5 years ago • 10 comments

Hi! This library looks like really cool - this is a much better way to declare model-classes for sequelize! Unfortunately i ran into a problem when trying it out:

Versions

I'm submitting a ... [x] bug report [ ] feature request

Actual behavior:

I tried converting my existing sequelize code to sequelize-typescript. It worked for the most part but i encountered a difference in behavior. (Not 100% how to debug this, so i am reporting it as a bug here - could of course be a configuration issue or similar on my end)

The non-static update-function on a Model, normally returns a promise containing a model-object with the updated property values. After changing from plain sequelize to sequelize-typescript, it no longer does this - instead the promise now contains a model-object with the old values from before the update. (The database is updated with the new values, the problem is just that the return value of the update-method contains different values than it normally does)

When debugging, i can see that the dataValues-object of the model-object are updated with the new values, but the properties of the model-object still contain the old values. (They are not updated)

The same problem happens for the with set + save combo.

Expected behavior:

The non-static update method should return a model-object with updated values, like in plain sequelize. (and same for set+save)

Steps to reproduce: See code below

Related code:

The model:

import {Table, Column, Model, HasMany, DataType, BelongsTo, BeforeDestroy, DeletedAt, UpdatedAt, CreatedAt} from 'sequelize-typescript';

@Table({
    tableName: 'users',
    timestamps: true,
    underscored: true,
    paranoid: true
})
export default class User extends Model<User> {

    @Column({type: DataType.INTEGER, field: "id", primaryKey: true, autoIncrement:true})
    id: number = this.id; // <- see: https://github.com/sequelize/sequelize/issues/10579  https://github.com/babel/babel/issues/9105
       
    @Column({type: DataType.STRING(100), field: "first_name", allowNull: false})
    firstName: string = this.firstName;
}

The code to reproduce the problem:

...

const user = await User.findById(userId, { transaction: t });

console.log("Before update: ", user.firstName); // the old value

const updatedUser = await user.update({ firstName: "Something" }, { transaction: t });

console.log("After update: ", updatedUser.firstName); // still the old value

...

Same problem happens with:

...

const user = await User.findById(userId, { transaction: t });

console.log("Before update: ", user.firstName); // the old value

user.set({ firstName: "Something" });

console.log("After set: ", user.firstName); // still the old value

const updatedUser = await user.save({ transaction: t });	

console.log("After save: ", user.firstName); // still the old value
console.log("After save: ", updatedUser.firstName); // still the old value

...

Without sequelize-typescript (i.e. with just plain sequelize without typescript), it works.

It is of course not impossible that it is a babel vs typescript issue. Could it, for example, be because i'm assigning each property to itself in the model? (for the reason for doing this, see: https://github.com/sequelize/sequelize/issues/10579 and https://github.com/babel/babel/issues/9105 ).

Library configurations:

.babelrc:

{
    "presets": [
	["@babel/env", {"targets": {"node": "8.0"}}],
	"@babel/react",
	["@babel/typescript", {"isTSX": true, "allExtensions": true}]
    ],
    "plugins": [
	["@babel/plugin-proposal-decorators", { "legacy": true }],
	["@babel/plugin-proposal-class-properties", { "loose":  true }]
    ]
}

tsconfig.json:

{
    "compilerOptions": {
	"target": "esnext",
	"moduleResolution": "node",
	"allowJs": true,
	"noEmit": true,
	"strict": true,
	"isolatedModules": true,
	"esModuleInterop": true,

	"jsx": "react",

	"experimentalDecorators": true,  // for sequelize-typescript
	"emitDecoratorMetadata": true   // for sequelize-typescript
    },
    "include": [
	"./src/"
    ]
}

Sequelize-typesctipt setup:

import config from ...;
import {Sequelize} from 'sequelize-typescript';

const sequelize = new Sequelize({
    database: config.db.database,
    username: config.db.username,
    password: config.db.password,
    host: config.db.host,
    port: config.db.port,
    dialect: "mysql",
    pool: {
	max: 100,
	min: 0,
	idle: 1000000
    },
    define: {
	charset: 'utf8mb4',
	collate: 'utf8mb4_unicode_ci',
	timestamps: true
    },
    logging: config.sequelize_logging ? (x: any) => console.log("[SEQUELIZE]",x) : () => false,
    typeValidation:false,
    modelPaths: [__dirname + "/../entities"]
});


sequelize
    .authenticate()
    .catch(err => {
	console.error('Unable to connect to the database:', err);
    });

sequelize.sync({
    force: false
});

export default sequelize;
insert short code snippets here

BjarkeNL avatar May 03 '19 16:05 BjarkeNL

Hey @BjarkeNL, I think the issue is indeed caused by this behaviour of babel. I've got the impression that _defineProperty overrides the previously defined getters and setters created by sequelize. As you already mentioned the dataValues are updated. But when calling user.firstName it uses the getter which is defined by the babel plugin within the constructor of User and in turn doesn't get through the dateValues. So to me it definitely looks like an issue with babel-plugin-proposal-class-properties. Can you omit this plugin? If not, I'm trying to think about a workaround.

Hope this helps!

RobinBuschmann avatar May 03 '19 20:05 RobinBuschmann

@RobinBuschmann Hey! I have this exact problem. Did you manage to find a solution?

Telokis avatar May 11 '19 17:05 Telokis

This is a current difference in behavior between Typescript and ECMAScript class fields. Babel's TypeScript transformation parses TS, but the runtime behavior for ECMA features follows the ECMA spec, not TS.

https://github.com/microsoft/TypeScript/issues/27644

loganfsmyth avatar May 12 '19 19:05 loganfsmyth

@loganfsmyth thanks for giving more insights. So babel-plugin-proposal-class-properties is doing it the right way 🤔

@Telokis @BjarkeNL For now I can only provide a workaround (Overriding the defined properties again):

function Fix(target): void {
  return class extends target {
    constructor(...args: any[]) {
      super(...args);
      Object.keys(new.target.rawAttributes).forEach(propertyKey => {
        Object.defineProperty(this, propertyKey, {
          get() {
            return this.getDataValue(propertyKey);
          },
          set(value) {
            this.setDataValue(propertyKey, value);
          }
        });
      });
    }
  } as any;
}

@Table
@Fix
export class Actor extends Model<Actor> { }

RobinBuschmann avatar May 13 '19 16:05 RobinBuschmann

@RobinBuschmann, this solution doesn't work with associations.

@Table
@Fix
export default class Example extends Model<Example> {
  @AllowNull(false)
  @ForeignKey(() => SomeModel)
  @Column(DataType.INTEGER)
  someModelId!: number;

  @BelongsTo(() => SomeModel)
  someModel!: SomeModel;
}

example.someModel is undefined where example is instance of Example.

DalerAkhmetov avatar Feb 07 '20 14:02 DalerAkhmetov

@DalerAkhmetov Good point. One need to add the association keys as well like so:

[
  ...Object.keys(new.target.rawAttributes), 
  ...Object.keys(new.target.associations),
].forEach(propertyKey => {
  // ..
});

(Untested, but) Hope this helps for now!

RobinBuschmann avatar Feb 08 '20 11:02 RobinBuschmann

@DalerAkhmetov Good point. One need to add the association keys as well like so:

[
  ...Object.keys(new.target.rawAttributes), 
  ...Object.keys(new.target.associations),
].forEach(propertyKey => {
  // ..
});

(Untested, but) Hope this helps for now!

thanks, it works

DalerAkhmetov avatar Feb 09 '20 15:02 DalerAkhmetov

Hi @RobinBuschmann, I found a bug. Solution https://github.com/RobinBuschmann/sequelize-typescript/issues/612#issuecomment-491890977 doesn't work if I use bulkCreate and pass records as array of Model instance For example:

let invoices = [];
invoices.push(Invoice.build({amount: 100}));
invoices.push(Invoice.build({amount: 120}));
invoices.push(Invoice.build({amount: 150}));

await Invoice.bulkCreate(invoices); // UNEXPECTED BEHAVIOUR - bulkCreate will try to insert records without fields (in our case it's amount);
// but the following line works fine
await Invoice.bulkCreate(invoices.map(e => e.get({plain: true})));

I don't know how to fix it. Do you have any ideas?

DalerAkhmetov avatar Jul 17 '20 09:07 DalerAkhmetov

@DalerAkhmetov Good point. One need to add the association keys as well like so:

[
  ...Object.keys(new.target.rawAttributes), 
  ...Object.keys(new.target.associations),
].forEach(propertyKey => {
  // ..
});

This is awfully close but it does seem to result in any modified associations being included in the models internal changed() calculation. for example if you have a BelongsTo association setup for collection, and then do something like:

const item = new Item();
item.collection = new Collection();
item.save()

The query that's generated is invalid because it tries to save the collection field. I don't yet have a solution, suspect I need to understand sequelize internals much more fully.


Edit: This solution also overrides any existing custom getter/setters 😢


Edit 2: You can find a working implementation of the Fix decorator here: https://github.com/outline/outline/blob/0de6650aa5e97612d46b459de719a000da6349be/server/models/decorators/Fix.ts#L9

tommoor avatar Dec 28 '21 23:12 tommoor

I'm getting the same error and I've not fixed it yet. I tried all the above solutions and it does not work for me :'(

EdisonJpp avatar Oct 18 '23 18:10 EdisonJpp