escape columns in InsertQueryBuilder.orUpdate #6316

Vrtak-CZ · 2020-06-26T13:46:51Z

Fixies this use-case

await repository
        .createQueryBuilder()
        .insert()
        .values({
            name,
            code,
            createdAt: new Date(),
            updatedAt: new Date(),
        })
        .orUpdate({
            conflict_target: ['name'],
            overwrite: ['code', 'updatedAt'],
        })
        .execute();

because it generates:

INSERT INTO "my_table"("id", "name", "code", "createdAt", "updatedAt") VALUES (DEFAULT, $1, $2, $3, $4) ON CONFLICT  ( name )  DO UPDATE SET code = EXCLUDED.code, updatedAt = EXCLUDED.updatedAt

and it causes:

ERROR:  column excluded.updatedat does not exist
LINE 1: ...  DO UPDATE SET code = EXCLUDED.code, updatedAt = EXCLUDED.u...
                                                             ^
HINT:  Perhaps you meant to reference the column "excluded.updatedAt".

instead of:

INSERT INTO "my_table"("id", "name", "code", "createdAt", "updatedAt") VALUES (DEFAULT, $1, $2, $3, $4) ON CONFLICT  ( "name" )  DO UPDATE SET "code" = EXCLUDED."code", "updatedAt" = EXCLUDED."updatedAt"

pleerock · 2020-07-06T23:48:29Z

src/query-builder/InsertQueryBuilder.ts

      if (statement && Array.isArray(statement.columns))
-          this.expressionMap.onUpdate.columns = statement.columns.map(column => `${column} = :${column}`).join(", ");
+          this.expressionMap.onUpdate.columns = statement.columns.map(column => `${this.escape(column)} = :${column}`).join(", ");
      if (statement && Array.isArray(statement.overwrite)) {
        if (this.connection.driver instanceof MysqlDriver || this.connection.driver instanceof AuroraDataApiDriver) {
          this.expressionMap.onUpdate.overwrite = statement.overwrite.map(column => `${column} = VALUES(${column})`).join(", ");


why did you ignore a mysql?

i do not work with MySQL so i don't know if it's needed

MySQL might not need this as it preserves case sensitivity for column names if no quotes. https://seeq.atlassian.net/wiki/spaces/KB/pages/443088907/SQL+Column+Names+and+Case+Sensitivity

Can we escape MySQL anyways as part of this to improve that side of things as well?

In code for columns pleerock removed extra escaping from columns some two years ago :) . So probably more stable decision needed.

so what is the decision? should i add escaping for MySQL or not? I didn't work with MySQL for few years now so i do not know if it's needed or if it breaks something when it's escaped when you don't need to escape.

@gintsgints do you have any insights on why we removed extra escaping?

@Vrtak-CZ it will be consistent if we add for MySQL as well. Case sensitivity preservation is a configuration option that is different by default in different databases.

pleerock · 2020-07-06T23:50:31Z

You are sure conflict_target and overwrite only accept a column names as their values? Can you please add tests, so we can be sure functionality is properly working for all platforms and won't break in future

Vrtak-CZ · 2020-08-06T11:26:43Z

You are sure conflict_target and overwrite only accept a column names as their values?

I'm not.

Can you please add tests, so we can be sure functionality is properly working for all platforms and won't break in future.

Few tests added

imnotjames · 2020-10-10T01:53:51Z

Can you rebase against master so the tests kick off fully?

Vrtak-CZ · 2020-10-14T20:39:37Z

@imnotjames done

imnotjames · 2020-10-14T22:30:09Z

Unfortunately, some test failures.

query builder > insertion > on conflict 1) "before all" hook for "should perform insertion correctly" 2) "after all" hook for "should perform insertion correctly" 371 passing (2m) 9 pending 2 failing 1) query builder > insertion > on conflict "before all" hook for "should perform insertion correctly": DataTypeNotSupportedError: Data type "timestamp with time zone" in "Post.date" is not supported by "better-sqlite3" database. at new DataTypeNotSupportedError (build/compiled/src/error/DataTypeNotSupportedError.js:2:568) at /typeorm/build/compiled/src/metadata-builder/EntityMetadataValidator.js:29:2316 at Array.forEach (<anonymous>) at EntityMetadataValidator.validate (build/compiled/src/metadata-builder/EntityMetadataValidator.js:29:2045) at /typeorm/src/metadata-builder/EntityMetadataValidator.ts:27:100 at Array.forEach (<anonymous>) at EntityMetadataValidator.validateMany (src/metadata-builder/EntityMetadataValidator.ts:27:100) at Connection.buildMetadatas (src/connection/Connection.ts:177:29) at Connection.<anonymous> (build/compiled/src/connection/Connection.js:39:31) at step (node_modules/tslib/tslib.js:141:27) at Object.next (node_modules/tslib/tslib.js:122:57) at fulfilled (node_modules/tslib/tslib.js:112:62) 2) query builder > insertion > on conflict "after all" hook for "should perform insertion correctly": TypeError: Cannot read property 'map' of undefined at Object.closeTestingConnections (test/utils/test-utils.ts:289:36) at Context.<anonymous> (test/functional/query-builder/insert-on-conflict/query-builder-insert-on-conflict.ts:14:17) at processImmediate

imnotjames

Looks like a great change to make. Test cases can be improved, maybe.

test/functional/query-builder/insert-on-conflict/query-builder-insert-on-conflict.ts

imnotjames · 2020-10-19T05:13:25Z

src/query-builder/InsertQueryBuilder.ts

      if (statement && Array.isArray(statement.columns))
-          this.expressionMap.onUpdate.columns = statement.columns.map(column => `${column} = :${column}`).join(", ");
+          this.expressionMap.onUpdate.columns = statement.columns.map(column => `${this.escape(column)} = :${column}`).join(", ");
      if (statement && Array.isArray(statement.overwrite)) {
        if (this.connection.driver instanceof MysqlDriver || this.connection.driver instanceof AuroraDataApiDriver) {
          this.expressionMap.onUpdate.overwrite = statement.overwrite.map(column => `${column} = VALUES(${column})`).join(", ");


Can we escape MySQL anyways as part of this to improve that side of things as well?

test/functional/query-builder/insert-on-conflict/entity/Post.ts

Vrtak-CZ · 2020-10-26T14:18:47Z

is there anything more I can do?

pleerock · 2021-02-08T09:31:24Z

Sorry for delayed merge. Thank you for contribution!

Vrtak-CZ changed the title ~~esacpe columns in InsertQueryBuilder.orUpdate~~ escape columns in InsertQueryBuilder.orUpdate Jun 29, 2020

pleerock reviewed Jul 6, 2020

View reviewed changes

Vrtak-CZ force-pushed the escape-columns branch from 7220090 to 0a65444 Compare August 6, 2020 11:16

Vrtak-CZ mentioned this pull request Aug 6, 2020

Query builder .orUpdate() doesn't add double-quotes around column names for Postgres #4017

Closed

Vrtak-CZ force-pushed the escape-columns branch from 0a65444 to c977ae3 Compare October 14, 2020 20:39

imnotjames self-requested a review October 14, 2020 22:29

Vrtak-CZ force-pushed the escape-columns branch 2 times, most recently from 1f73bd6 to 86198a6 Compare October 18, 2020 12:15

imnotjames reviewed Oct 19, 2020

View reviewed changes

Vrtak-CZ force-pushed the escape-columns branch 2 times, most recently from 50d9fb6 to 717ccc1 Compare October 20, 2020 16:45

esacpe columns in InsertQueryBuilder.orUpdate

1238d63

Vrtak-CZ force-pushed the escape-columns branch from 717ccc1 to 1238d63 Compare October 20, 2020 16:46

pleerock merged commit ab56e07 into typeorm:master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

escape columns in InsertQueryBuilder.orUpdate #6316

escape columns in InsertQueryBuilder.orUpdate #6316

Vrtak-CZ commented Jun 26, 2020 •

edited

Loading

pleerock Jul 6, 2020

Vrtak-CZ Aug 6, 2020

gintsgints Oct 17, 2020

imnotjames Oct 19, 2020

gintsgints Oct 20, 2020 •

edited

Loading

Vrtak-CZ Oct 20, 2020

pleerock Nov 2, 2020

pleerock Nov 2, 2020

pleerock commented Jul 6, 2020

Vrtak-CZ commented Aug 6, 2020

imnotjames commented Oct 10, 2020

Vrtak-CZ commented Oct 14, 2020

imnotjames commented Oct 14, 2020

imnotjames left a comment

imnotjames Oct 19, 2020

Vrtak-CZ commented Oct 26, 2020

pleerock commented Feb 8, 2021

escape columns in InsertQueryBuilder.orUpdate #6316

escape columns in InsertQueryBuilder.orUpdate #6316

Conversation

Vrtak-CZ commented Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gintsgints Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock commented Jul 6, 2020

Vrtak-CZ commented Aug 6, 2020

imnotjames commented Oct 10, 2020

Vrtak-CZ commented Oct 14, 2020

imnotjames commented Oct 14, 2020

imnotjames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrtak-CZ commented Oct 26, 2020

pleerock commented Feb 8, 2021

Vrtak-CZ commented Jun 26, 2020 •

edited

Loading

gintsgints Oct 20, 2020 •

edited

Loading