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

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

Conversation

@Americas
Copy link
Contributor

@Americas Americas commented Oct 9, 2019

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: ['*']
@Americas Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch from 9dae6b1 to fd956cd Oct 9, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 9, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@Americas Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch 3 times, most recently from d45e8cc to c3a8341 Oct 9, 2019
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 9, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 14, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 14, 2019

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
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

@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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

@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
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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: #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

image

@Americas
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

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

@Americas
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

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
Copy link
Contributor Author

@Americas Americas commented Oct 15, 2019

@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
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 15, 2019

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

@TheOptimisticFactory
Copy link

@TheOptimisticFactory TheOptimisticFactory commented Oct 16, 2019

@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
Copy link
Contributor Author

@Americas Americas commented Oct 16, 2019

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

@Americas Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch 2 times, most recently from 42b37b8 to e92ac61 Oct 16, 2019
Americas added 2 commits Oct 16, 2019
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 Americas force-pushed the feature/exclude-non-model-attributes-on-returning branch from e92ac61 to 498a1f9 Oct 16, 2019
@sushantdhiman sushantdhiman merged commit 26ea410 into sequelize:master Oct 19, 2019
3 of 4 checks passed
@Americas Americas deleted the feature/exclude-non-model-attributes-on-returning branch Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants