feat(postgres): change returning option to only return model attributes #11526

Americas · 2019-10-09T13:13:29Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you update the typescript typings accordingly (if applicable)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

If an underlying table has columns that don't exist in the model, when using returning: true the instance will be created with these extra attributes. This change alters this behavior to only return the fields defined in the model, provided a model definition is passed.

Note: this is a breaking change since the option returning: true will no longer create
attributes on the instance that are not defined in the model.

The old default behavior can be used by:

- returning: true
+ returning: ['*']

codecov · 2019-10-09T13:51:12Z

Codecov Report

Merging #11526 into master will increase coverage by 0.39%.
The diff coverage is 94.91%.

@@            Coverage Diff             @@
##           master   #11526      +/-   ##
==========================================
+ Coverage   95.86%   96.26%   +0.39%     
==========================================
  Files          91       94       +3     
  Lines        8982     9186     +204     
==========================================
+ Hits         8611     8843     +232     
+ Misses        371      343      -28

Impacted Files	Coverage Δ
lib/dialects/mssql/query-generator.js	`95.91% <100%> (+0.01%)`	⬆️
lib/model.js	`96.53% <100%> (+0.06%)`	⬆️
lib/dialects/postgres/query-generator.js	`94.35% <100%> (ø)`	⬆️
lib/dialects/abstract/query-generator.js	`97.01% <94.44%> (-0.18%)`	⬇️
lib/dialects/mariadb/index.js	`100% <0%> (ø)`
lib/dialects/mariadb/connection-manager.js	`100% <0%> (ø)`
lib/dialects/mariadb/query.js	`87.96% <0%> (ø)`
lib/sequelize.js	`95.95% <0%> (+0.62%)`	⬆️
lib/query-interface.js	`92.19% <0%> (+1.46%)`	⬆️
lib/dialects/mariadb/data-types.js	`100% <0%> (+49.01%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2472bb...498a1f9. Read the comment docs.

sushantdhiman · 2019-10-09T17:19:42Z

LGTM, This is a breaking change.

We have been planing to start work related to next major release. I think I'll have some time during weekend of 19-20th Oct. I'll start next beta cycle then and will include this change. Thanks

TheOptimisticFactory · 2019-10-14T16:53:39Z

01 - Issue

This appears to unfortunately break upsert feature of postgres.
In my case it fails with SequelizeDatabaseError: invalid input syntax for integer: "2019-10-14 16:23:45.797+00"

const [ baseline ] = await db.session_users.findOrCreate({
    defaults: {
      start_at: moment.utc().toISOString(),
    },
    logging: console.log,
    where: {
      id_session: session.id_session,
      id_user: user.id_user,
    },
  });

//=========================
// OUTPUT FROM SEQUELIZE   
//=========================
 
Executing (95687979-1cf9-4c19-8d46-abe0ab0e706e): START TRANSACTION;

Executing (95687979-1cf9-4c19-8d46-abe0ab0e706e): SELECT 
  "id_session_user", "id_session", "id_user", "device_type", "start_at", "end_at", "state", "token", "active_time", "accept_recording", "created_at", "updated_at" 
  FROM "session_users" AS "session_users" 
  WHERE "session_users"."id_session" = 1 AND "session_users"."id_user" = 1 LIMIT 1;

Executing (95687979-1cf9-4c19-8d46-abe0ab0e706e): CREATE OR REPLACE FUNCTION pg_temp.testfunc(OUT response "session_users", OUT sequelize_caught_exception text) 
  RETURNS RECORD AS $func_238212f3bd9f4e0c8b34e77774f9fd37$ 
  BEGIN INSERT INTO "session_users" ("id_session_user","id_session","id_user","start_at","state","active_time","accept_recording","created_at","updated_at") 
  VALUES (DEFAULT,1,1,'2019-10-14 16:23:45.794 +00:00','pending',NULL,NULL,'2019-10-14 16:23:45.797 +00:00','2019-10-14 16:23:45.797 +00:00')
  RETURNING "id_session_user","id_session","id_user","device_type","start_at","end_at","state","token","active_time","accept_recording","created_at","updated_at" INTO response; 
  EXCEPTION WHEN unique_violation THEN GET STACKED DIAGNOSTICS sequelize_caught_exception = PG_EXCEPTION_DETAIL; END $func_238212f3bd9f4e0c8b34e77774f9fd37$ LANGUAGE plpgsql; 
  SELECT (testfunc.response).*, testfunc.sequelize_caught_exception FROM pg_temp.testfunc(); DROP FUNCTION IF EXISTS pg_temp.testfunc();

Executing (95687979-1cf9-4c19-8d46-abe0ab0e706e): COMMIT;

//=========================
// OUTPUT FROM POSTGRES   
//=========================

2019-10-14 16:46:02.141 UTC [2900] ERROR:  invalid input syntax for integer: "2019-10-14 16:46:02.139+00"
2019-10-14 16:46:02.141 UTC [2900] CONTEXT:  PL/pgSQL function pg_temp_6.testfunc() line 1 at SQL statement

02 - Troubleshooting / fix

I haven't completely narrowed down why it does not work.

BUT I've noticed that enforcing the return fragment to equal * solves the problem 😃

To do so, I've added returnValues.query = 'RETURNING *'; here => https://github.com/sequelize/sequelize/pull/11526/files#diff-def52474024b12c7d133bb504cdf57d1R138

//============================================
// OUTPUT FROM SEQUELIZE (with "RETURNING *")
//============================================

Executing (37209d5c-b8e2-42a1-af05-b718cde87431): START TRANSACTION;

Executing (37209d5c-b8e2-42a1-af05-b718cde87431): SELECT 
  "id_session_user", "id_session", "id_user", "device_type", "start_at", "end_at", "state", "token", "active_time", "accept_recording", "created_at", "updated_at" 
  FROM "session_users" AS "session_users" 
  WHERE "session_users"."id_session" = 1 AND "session_users"."id_user" = 1 LIMIT 1;

Executing (37209d5c-b8e2-42a1-af05-b718cde87431): CREATE OR REPLACE FUNCTION pg_temp.testfunc(OUT response "session_users", OUT sequelize_caught_exception text) 
  RETURNS RECORD AS $func_ab6518d65b79441791ef8ccdbd4ec59d$ 
  BEGIN INSERT INTO "session_users" ("id_session_user","id_session","id_user","start_at","state","active_time","accept_recording","created_at","updated_at") 
  VALUES (DEFAULT,1,1,'2019-10-14 16:49:29.275 +00:00','pending',NULL,NULL,'2019-10-14 16:49:29.277 +00:00','2019-10-14 16:49:29.277 +00:00')
  RETURNING * INTO response; 
  EXCEPTION WHEN unique_violation THEN GET STACKED DIAGNOSTICS sequelize_caught_exception = PG_EXCEPTION_DETAIL; END $func_ab6518d65b79441791ef8ccdbd4ec59d$ LANGUAGE plpgsql; 
  SELECT (testfunc.response).*, testfunc.sequelize_caught_exception FROM pg_temp.testfunc(); DROP FUNCTION IF EXISTS pg_temp.testfunc();

Executing (37209d5c-b8e2-42a1-af05-b718cde87431): COMMIT;

TheOptimisticFactory · 2019-10-14T17:19:46Z

Here is the model for full disclosure

const SessionUsers = sequelize.define('session_users', {
    id_session_user: {
      type: DataTypes.INTEGER,
      autoIncrement: true,
      primaryKey: true,
    },
    id_session: {
      type: DataTypes.INTEGER,
      allowNull: false,
    },
    id_user: {
      type: DataTypes.INTEGER,
      allowNull: false,
    },
    device_type: DataTypes.ENUM('web', 'ios'),
    start_at: DataTypes.DATE,
    end_at: DataTypes.DATE,
    state: {
      type: DataTypes.ENUM('declined', 'cancelled', 'joined', 'left', 'pending'),
      defaultValue: 'pending',
    },
    token: {
      type: DataTypes.TEXT,
      allowNull: true,
    },
    active_time: {
      allowNull: true,
      defaultValue: null,
      type: DataTypes.INTEGER,
    },
    accept_recording: {
      allowNull: true,
      defaultValue: null,
      type: DataTypes.BOOLEAN,
    },
    created_at: DataTypes.DATE,
    updated_at: DataTypes.DATE,
  }, {
    underscored: true,

    // WORKAROUND until https://github.com/sequelize/sequelize/issues/11225 gets fixed
    createdAt: 'created_at',
    updatedAt: 'updated_at',
  });

Americas · 2019-10-15T08:34:11Z

@TheOptimisticFactory I'm going to need more information, because that query works for me. Postgres output shows no error, and the query returns the created or selected row as expected...

Executing (40e92b5f-83d6-49c6-b43e-4833f68578f3): START TRANSACTION;

Executing (40e92b5f-83d6-49c6-b43e-4833f68578f3): SELECT 
  "id_session_user", "id_session", "id_user", "device_type", "start_at", "end_at", "state", "token", "active_time", "accept_recording", "created_at", "updated_at" 
  FROM "session_users" AS "session_users" 
  WHERE "session_users"."id_session" = 1 AND "session_users"."id_user" = 1 
  LIMIT 1;

Executing (40e92b5f-83d6-49c6-b43e-4833f68578f3): CREATE OR REPLACE FUNCTION pg_temp.testfunc(OUT response "session_users", OUT sequelize_caught_exception text) 
  RETURNS RECORD AS $func_4cec91f65fc34cef9f32ca4f80638b76$ 
  BEGIN INSERT INTO "session_users" ("id_session_user","id_session","id_user","start_at","state","active_time","accept_recording","created_at","updated_at") 
  VALUES (DEFAULT,1,1,'2019-10-15 08:27:29.247 +00:00','pending',NULL,NULL,'2019-10-15 08:27:29.268 +00:00','2019-10-15 08:27:29.268 +00:00') 
  RETURNING "id_session_user","id_session","id_user","device_type","start_at","end_at","state","token","active_time","accept_recording","created_at","updated_at" INTO response;
  EXCEPTION WHEN unique_violation THEN GET STACKED DIAGNOSTICS sequelize_caught_exception = PG_EXCEPTION_DETAIL; END $func_4cec91f65fc34cef9f32ca4f80638b76$ LANGUAGE plpgsql; 
  SELECT (testfunc.response).*, testfunc.sequelize_caught_exception FROM pg_temp.testfunc(); DROP FUNCTION IF EXISTS pg_temp.testfunc();

Executing (40e92b5f-83d6-49c6-b43e-4833f68578f3): COMMIT;

Instance created:

session_users {
  dataValues:
   { state: 'pending',
     active_time: null,
     accept_recording: null,
     id_session_user: 1,
     start_at: 2019-10-15T08:27:29.247Z,
     id_session: 1,
     id_user: 1,
     updated_at: 2019-10-15T08:27:29.268Z,
     created_at: 2019-10-15T08:27:29.268Z,
     device_type: null,
     end_at: null,
     token: null },
  _previousDataValues:
   { start_at: 2019-10-15T08:27:29.247Z,
     id_session: 1,
     id_user: 1,
     id_session_user: 1,
     device_type: null,
     end_at: null,
     state: 'pending',
     token: null,
     active_time: null,
     accept_recording: null,
     created_at: 2019-10-15T08:27:29.268Z,
     updated_at: 2019-10-15T08:27:29.268Z },
  _changed:
   { start_at: false,
     id_session: false,
     id_user: false,
     id_session_user: false,
     device_type: false,
     end_at: false,
     state: false,
     token: false,
     active_time: false,
     accept_recording: false,
     created_at: false,
     updated_at: false },
  _modelOptions:
   { timestamps: true,
     validate: {},
     freezeTableName: false,
     underscored: true,
     paranoid: false,
     rejectOnEmpty: false,
     whereCollection: { id_session: 1, id_user: 1 },
     schema: null,
     schemaDelimiter: '',
     defaultScope: {},
     scopes: {},
     indexes: [],
     name: { plural: 'session_users', singular: 'session_user' },
     omitNull: false,
     createdAt: 'created_at',
     updatedAt: 'updated_at',
     sequelize:
      Sequelize {
        options: [Object],
        config: [Object],
        dialect: [PostgresDialect],
        queryInterface: [QueryInterface],
        models: [Object],
        modelManager: [ModelManager],
        connectionManager: [ConnectionManager],
        importCache: {} },
     hooks: {} },
  _options:
   { isNewRecord: true,
     _schema: null,
     _schemaDelimiter: '',
     attributes: undefined,
     include: undefined,
     raw: undefined,
     silent: undefined },
  isNewRecord: false }

TheOptimisticFactory · 2019-10-15T09:16:50Z

@TheOptimisticFactory I'm going to need more information, because that query works for me. Postgres output shows no error, and the query returns the created or selected row as expected...

Sure thing. I would be happy to help.
What additional information would you want to have?

Americas · 2019-10-15T10:06:07Z

Postgres version, is it running native or not? Do you have a migration for the table? Or did you create it using sequelize.sync()? Or better yet, can you show me the table schema?

TheOptimisticFactory · 2019-10-15T11:08:23Z

Postgres version, is it running native or not? Do you have a migration for the table? Or did you create it using sequelize.sync()? Or better yet, can you show me the table schema?

Environment

Node: v12.11.1, NPM: v6.11.3
Using postgres v10 (docker image pulled from library/postgres:10-alpine)
Using the dependencies sequelize v5.19.0 modified with the diffs of this PR, along with pg v7.12.1

Context

Models are deleted in cascade and migrations rerun from scratch prior to running my tests (snippet n°1)
I cant disclose all migrations (97 files) but ultimately the model is exactly as shown in my previous comment: feat(postgres): change returning option to only return model attributes #11526 (comment)
You'll be able to find the table properties extracted from DBeaver in snippet n°2

Snippet n°1

'use strict';

const path = require('path');
const Umzug = require('umzug');

const { database: db } = require('~API_SERVICES');

async function runMigrations() {

  await db.sequelize.drop({
    cascade: true,
  });

  const umzug = new Umzug({
    migrations: {
      params: [
        db.sequelize.getQueryInterface(),
        db.Sequelize,
      ],
      path: path.join(process.cwd(), './services/database/migrations/scripts'),
    },
    storage: 'none',
  });

  await umzug.up();
}

module.exports = runMigrations;

Snippet n°2

Americas · 2019-10-15T11:57:56Z

Still nothing...

Judging by the error context pg_temp_6.testfunc() do you have any triggers running on this table? or any sequelize hooks?

TheOptimisticFactory · 2019-10-15T12:18:14Z

Still nothing...

Judging by the error context pg_temp_6.testfunc() do you have any triggers running on this table? or any sequelize hooks?

No trigger nor any sequelize hook.

Discard the following (wrong assessment wtith stashed diffs)

~~I have just made another discovery~~

~~The diffs of this PR applied on top of sequelize v5.19.0 throws the aforementioned error~~
~~The diffs of this PR applied on top of sequelize v5.19.6 works as expected~~ 🤔

Americas · 2019-10-15T12:21:23Z

I checked out to v5.19.0 and cherry-picked this commit on top of it. It had no issues running the findOrCreate.

git log:

* 2d74388a - (HEAD) feat(postgres): change returning option to only return model attributes (48 minutes ago) <Ricardo Proença>
* 8b53f721 - (tag: v5.19.0) fix(showTablesQuery): ignore views for mssql/mysql (#11439) (4 weeks ago) <João Eiras>

TheOptimisticFactory · 2019-10-15T12:34:10Z

Nevermind, I am dumb. I am facing the issue using both v5.19.0 and v5.19.6 as baselines.
It is unrelated to the version

-- My stashed diffs included the workaround returnValues.query = 'RETURNING *';.
-- I have edited my previous comment to avoid any confusion

TheOptimisticFactory · 2019-10-15T13:34:09Z

Removing created_at and updated_at from the RETURNING-fragment makes the query work

The thing is another model with a similar findOrCreate call works without any RETURNING-fragment workaround 🤔

TheOptimisticFactory · 2019-10-15T14:46:56Z

Enforcing await db.sequelize.sync({ force: true }); prior to running the tests also makes the query work.

In the snippet n°1 of this comment: #11526 (comment), I was using db.sequelize.drop({ cascade: true }) to "reset" the database

Is there a difference I am missing between this operation and db.sequelize.sync({ force: true })?

EDIT

The only diff I can see between the tables before/after running db.sequelize.sync({ force: true }) is the ordinal position of columns being different

=> as shown here: https://www.diffchecker.com/0e8MaKWO

Here is my updated migration runner snippet:

'use strict';

const path = require('path');
const Umzug = require('umzug');

const { database: db } = require('~API_SERVICES');

async function runMigrations() {
  await db.sequelize.drop({
    cascade: true,
  });

  const umzug = new Umzug({
    migrations: {
      params: [
        db.sequelize.getQueryInterface(),
        db.Sequelize,
      ],
      path: path.join(process.cwd(), './services/database/migrations/scripts'),
    },
    storage: 'none',
  });

  await umzug.up();

  const debugQuery = `SELECT
    "column_name",
    "ordinal_position",
    "udt_name",
    "udt_schema",
    "data_type",
    "is_nullable",
    "column_default"
    from INFORMATION_SCHEMA.COLUMNS where table_name = 'session_users'
    ORDER BY ordinal_position`;

  const [ before ] = await db.sequelize.query(debugQuery);

  console.log(JSON.stringify(before, null, 2));

  await db.sequelize.sync({
    force: true,
  });

  const [ after ] = await db.sequelize.query(debugQuery);

  console.log(JSON.stringify(after, null, 2));
}

module.exports = runMigrations;

TheOptimisticFactory · 2019-10-15T16:36:45Z

Adding an additional field after the table creation through a migration, seems to be the root cause of the error.

I suspect a mismatch between the order of returned columns and their respective value somehow

Americas · 2019-10-15T16:56:19Z

Yes, if I order the table as the diff, I get that error, hurray 😄

Americas · 2019-10-15T17:11:19Z

This is a bigger bug related to the exception option, trying to run this with returning: false will throw in master. But it seems the fix is not too complex 😄

Americas · 2019-10-15T17:30:02Z

@TheOptimisticFactory I fixed the issue with the insert while using option exception: true which is what happens on the findOrCreate method.

The only issue now is that if that option is true, it will return all the table's columns, so I'll have to fix that as well now 😆

Can you confirm that this fixes your use case? It works for me.

TheOptimisticFactory · 2019-10-15T20:18:12Z

@Americas I am out of the office ATM. I will try your hotfix tomorrow first thing (CEST timezone) 😃

TheOptimisticFactory · 2019-10-16T08:29:34Z

@TheOptimisticFactory I fixed the issue with the insert while using option exception: true which is what happens on the findOrCreate method.

The only issue now is that if that option is true, it will return all the table's columns, so I'll have to fix that as well now

Can you confirm that this fixes your use case? It works for me.

@Americas I can confirm your hotfix 44c2603 indeed solved the issue I was encountering! 🎉

Americas · 2019-10-16T10:56:05Z

Awesome! I'll update tests then, and fix this to return only model attributes as well.

If an underlying table has columns that don't exist in the model, when using `returning: true` the instance will be created with these extra attributes. This change alters this behavior to only return the fields defined in the model, provided a model definition is passed. BREAKING CHANGE: setting `returning: true` will no longer create attributes on the instance that are not defined in the model. The old default behavior, if desired, can be used by changing the: `returning: true` to `returning: ['*']`

Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch from 9dae6b1 to fd956cd Compare October 9, 2019 13:51

Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch 3 times, most recently from d45e8cc to c3a8341 Compare October 9, 2019 16:40

sushantdhiman approved these changes Oct 9, 2019

View reviewed changes

sushantdhiman added the breaking change For issues and PRs. Changes that break compatibility and require a major version increment. label Oct 9, 2019

papb added the status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue. label Oct 15, 2019

Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch 2 times, most recently from 42b37b8 to e92ac61 Compare October 16, 2019 12:50

Americas added 2 commits October 16, 2019 15:02

fix(findOrCreate): ensure option returning is true on insert

6f4ebbd

Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch from e92ac61 to 498a1f9 Compare October 16, 2019 14:07

sushantdhiman merged commit 26ea410 into sequelize:master Oct 19, 2019

Americas deleted the feature/exclude-non-model-attributes-on-returning branch October 19, 2019 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgres): change returning option to only return model attributes #11526

feat(postgres): change returning option to only return model attributes #11526

Americas commented Oct 9, 2019 •

edited

codecov bot commented Oct 9, 2019 •

edited

sushantdhiman commented Oct 9, 2019

TheOptimisticFactory commented Oct 14, 2019 •

edited

TheOptimisticFactory commented Oct 14, 2019

Americas commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 •

edited

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019 •

edited

Americas commented Oct 15, 2019

Americas commented Oct 15, 2019

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019

TheOptimisticFactory commented Oct 16, 2019 •

edited

Americas commented Oct 16, 2019

feat(postgres): change returning option to only return model attributes #11526

feat(postgres): change returning option to only return model attributes #11526

Conversation

Americas commented Oct 9, 2019 • edited

Pull Request check-list

Description of change

codecov bot commented Oct 9, 2019 • edited

Codecov Report

sushantdhiman commented Oct 9, 2019

TheOptimisticFactory commented Oct 14, 2019 • edited

01 - Issue

02 - Troubleshooting / fix

TheOptimisticFactory commented Oct 14, 2019

Americas commented Oct 15, 2019 • edited

TheOptimisticFactory commented Oct 15, 2019

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019

Environment

Context

Snippet n°1

Snippet n°2

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 • edited

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 • edited

TheOptimisticFactory commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019 • edited

EDIT

TheOptimisticFactory commented Oct 15, 2019 • edited

Americas commented Oct 15, 2019

Americas commented Oct 15, 2019

Americas commented Oct 15, 2019

TheOptimisticFactory commented Oct 15, 2019

TheOptimisticFactory commented Oct 16, 2019 • edited

Americas commented Oct 16, 2019

Americas commented Oct 9, 2019 •

edited

codecov bot commented Oct 9, 2019 •

edited

TheOptimisticFactory commented Oct 14, 2019 •

edited

Americas commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 15, 2019 •

edited

TheOptimisticFactory commented Oct 16, 2019 •

edited