fix: enable explicitly inserting IDENTITY values into mssql #6199

dklischies · 2020-06-04T15:51:50Z

Allow explicit insertion of preset values for mssql IDENTITY columns. These are the equivalent to
auto-incrementing primary keys in MySQL, but require special conditional treatment.

Closes: #2199 for mssql

dklischies · 2020-07-17T10:04:04Z

@pleerock Any opinion on this? Currently the behavior differs between database engines (i.e. Oracle DB, MySQL and Sqllite will update auto increment values, MSSQL will not). This is a step towards homogenizing this behavior.

pleerock · 2020-07-18T00:24:29Z

shouldn't this be set on a connection-level or configuration level? I found it weird having it here.

dklischies · 2020-07-20T09:32:16Z

I agree that this is rather weird, but that weirdness is a result of the way Microsoft implemented enabling manual insertion of values into identity columns. You can only enable INSERT_IDENTITY for one table per connection at the same time (see docs). That means we cannot enable this once when connecting to the database, because that would mean only enabling it for one table. Additionally, enabling this option disables the default value for the identity column. That means that we only want to enable INSERT_IDENTITY if the user supplies a value for the identity column (for specific queries) and not enable it if the user does not supply a value. These two issues lead to the requirement of enabling and disabling the option on a per query basis.

Lastly you are not allowed to set INSERT_IDENTITY if there is no Identity column in a table, hence all these checks in line 376ff of src/query-builder/InsertQueryBuilder.ts

src/query-builder/InsertQueryBuilder.ts

imnotjames · 2020-10-16T15:34:29Z

src/query-builder/InsertQueryBuilder.ts

+                .filter((column) => this.expressionMap.insertColumns.length > 0 ? this.expressionMap.insertColumns.indexOf(column.propertyPath) !== -1 : column.isInsert)
+                .some((column) => this.isOverridingAutoIncrementBehavior(column))
+        ) {
+            query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF`;


Are you able to escape the path for the table name here? (this.escapePath)

Have a look at line 304. Either the table name is already escaped, or there is an issue with injection already.

imnotjames · 2020-10-16T15:36:37Z

src/query-builder/InsertQueryBuilder.ts

+                && column.generationStrategy === "increment"
+                && this.getValueSets().some((valueSet) => 
+                    column.getEntityValue(valueSet) !== undefined 
+                    && column.getEntityValue(valueSet) !== null


If it is null isn't that technically a valid value that we'd want to count as a "supplied" value

In general, null is not a valid value for a primary key (because that is a clustered index that determines on-disk ordering of entries). Most databases (including mssql) prevent you from doing this.

Now, one could argue that this should fail instead of falling back to auto increment. However, that would be inconsistent with common SQL behavior, as databases cannot distinguish between undefined and null, and as such would insert a auto generated primary key if you supply null in a query.

What if it is '0'. I want to use the 'save()' method which can update and insert and when I want to insert then I set the data of identity column is 0. And it really insert 0 into the identity column. So I can not insert more data because it only update this row.

imnotjames · 2020-10-16T17:09:08Z

I'm making a change now to loosen up that insert test.

imnotjames · 2020-10-16T17:25:52Z

ACTUALLY, I'm confused - why is it running it for that code?

The DDL for the table that's in the test that failed is:

create table test
(
	id int not null
		constraint PK_5417af0062cf987495b611b59c7
			primary key,
	description nvarchar(255) not null
)

It's not an IDENTITY type.

But it should be - 🤔 This isn't a problem with your changes.

imnotjames · 2020-10-16T17:32:39Z

src/query-builder/InsertQueryBuilder.ts

+                .filter((column) => this.expressionMap.insertColumns.length > 0 ? this.expressionMap.insertColumns.indexOf(column.propertyPath) !== -1 : column.isInsert)
+                .some((column) => this.isOverridingAutoIncrementBehavior(column))
+        ) {
+            query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF`;


Also, Instead of doing this as part of building the query itself, how would it look as part of the execute function?

That way it's not muddying up the SQL we've generated - and we already do something similar to that for @OutputTable. That way it's more for the session than the query itself we're generating.

I thought about that. However, i think its more appropriate to have it here. Otherwise, the query generation method will generate a query which cannot be run, unless you execute it with the execute function. In other words, this method would build an invalid query, without additional context provided by execute. There is also a lot of driver dependent code in this method anyways.

The OutputTable handling in the execute method on the other hand has nothing to do with the insert query, but the way we retrieve results. In other words, without the OutputTable handling, the query would still work, it just could not retrieve the results correctly (which is a concern with regard to the return value of the execute function).

dklischies · 2020-10-16T18:34:44Z

ACTUALLY, I'm confused - why is it running it for that code?

The DDL for the table that's in the test that failed is:
create table test
(
	id int not null
		constraint PK_5417af0062cf987495b611b59c7
			primary key,
	description nvarchar(255) not null
)
It's not an IDENTITY type.

But it should be - 🤔 This isn't a problem with your changes.

It is an entity type. Look at the related entity/test.ts in that folder

@Entity()
export class Test {

    @PrimaryGeneratedColumn()
    id: number;

}

imnotjames · 2020-10-16T18:49:21Z

The DDL it generates is not an identity, though. It should be. Not sure why - I think there's a bug elsewhere in typeorm. Unrelated to this PR.

dklischies · 2020-10-16T18:54:23Z

The DDL it generates is not an identity, though. It should be. Not sure why - I think there's a bug elsewhere in typeorm. Unrelated to this PR.

I'm pretty sure you are looking at the wrong DDL. Where does that description field come from?

imnotjames · 2020-10-16T19:29:16Z

The DDL it generates is not an identity, though. It should be. Not sure why - I think there's a bug elsewhere in typeorm. Unrelated to this PR.

I'm pretty sure you are looking at the wrong DDL. Where does that description field come from?

From sql server studio - I also confirmed by inspecting the table manually. But not related to this PR so we're good :)

Allow explicit insertion of preset values for mssql IDENTITY columns. These are the equivalent to auto-incrementing primary keys in MySQL, but require special conditional treatment. Closes: typeorm#2199 for mssql

dklischies · 2020-11-05T14:42:04Z

Is there anything required from my side to get this merged? I'll happily discuss and make changes.
If you feel that you don't want to merge this at all please let me know, so I can stop rebasing/resolving conflicts.

FabianScheidt · 2020-12-14T16:34:04Z

I am having the exact issue that seems to be solved with this PR. Is there a plan to merge this in the near future?

pleerock · 2021-02-08T15:36:23Z

I usually prefer not to have these kind of changes (because of additional complexities it brings, and many "why"-s on many "if"-s. But you did a great job on investigation on how it works in sql server and I can't simply ignore these amount of efforts. Thank you for contribution!

geersch · 2021-02-10T13:27:59Z

Debatable if this should be considered as a breaking change since the behaviour is changed and can suddenly cause issues. At the very least it is a new feature and I would like to see this reflected in the version number. Only the patch portion was updated giving me no indication of new features, let alone breaking changes. Should only be incremented for backwards compatible fixes. Of course, this is due to the 0 major version which is still being used.

Updated to this version recently and a lot of tests started failing on the CI server. Turned out we had some code which was generating a value of 0 for the primary, generated columns. Granted this is a bug on our part. TypeOrm of course now thought this value should be inserted into the IDENTITY column.

Perhaps it is time to drop the "anything can change at any time" leading 0 major version? 0.3.0 would now be considered a major update, so the only option you have now is to update the patch version for backwards compatible fixes and features.

EatingJoey · 2021-07-02T03:16:29Z

Debatable if this should be considered as a breaking change since the behaviour is changed and can suddenly cause issues. At the very least it is a new feature and I would like to see this reflected in the version number. Only the patch portion was updated giving me no indication of new features, let alone breaking changes. Should only be incremented for backwards compatible fixes. Of course, this is due to the 0 major version which is still being used.

Updated to this version recently and a lot of tests started failing on the CI server. Turned out we had some code which was generating a value of 0 for the primary, generated columns. Granted this is a bug on our part. TypeOrm of course now thought this value should be inserted into the IDENTITY column.

Perhaps it is time to drop the "anything can change at any time" leading 0 major version? 0.3.0 would now be considered a major update, so the only option you have now is to update the patch version for backwards compatible fixes and features.

So how you reslove this issue？ I face the same issue with you and I did not find the early version which don`t have this function.

Thanks

geersch · 2021-07-02T06:27:40Z

@EatingJoey Browsing through the GIT history I see version 0.2.30 is the last version to not contain this change.

IIRC we fixed it by making sure we do not specify an explicit value for the identity based column. That was a bug on our part.

EatingJoey · 2021-07-02T06:38:55Z

@EatingJoey Browsing through the GIT history I see version 0.2.30 is the last version to not contain this change.

IIRC we fixed it by making sure we do not specify an explicit value for the identity based column. That was a bug on our part.

@geersch Thank you for your information.

wy193777 · 2022-04-26T00:12:04Z

Is there a way to disable SET IDENTITY_INSERT ON when using query builder or getRepository().save()? This feature makes trigger after insert and update unusable.

dklischies · 2022-04-26T08:41:29Z

SET IDENTITY_INSERT ON is only set if you supply a value for a primary key during insertion. Do not supply a value, and it will not be set.

Do you have a test case for the broken behavior you are seeing?

wy193777 · 2022-04-26T23:02:32Z

Seems only first call of getRepository().save() doesn't have SET IDENTITY_INSERT ON, all following calls will have it and id will be automatically provided by typeorm. Let me try create a test for it.

wy193777 · 2022-04-26T23:50:40Z

I found the problem. getRepository().save(data) will give data's object id if they are just inserted. So after first call, all following calls will have SET IDENTITY_INSERT ON. Solved by using JSON.parse(JSON.stringify(data)) to copy objects.

imnotjames reviewed Oct 14, 2020

View reviewed changes

src/query-builder/InsertQueryBuilder.ts Show resolved Hide resolved

src/query-builder/InsertQueryBuilder.ts Outdated Show resolved Hide resolved

src/query-builder/InsertQueryBuilder.ts Outdated Show resolved Hide resolved

dklischies force-pushed the mssql-2199-support-pk-insert branch from a14b9d9 to 972092b Compare October 16, 2020 14:59

imnotjames reviewed Oct 16, 2020

View reviewed changes

imnotjames mentioned this pull request Oct 16, 2020

Database identity insert with @PrimaryGeneratedColumn and MS SQL Server #6918

Closed

imnotjames reviewed Oct 16, 2020

View reviewed changes

fix: enable explicitly inserting IDENTITY values into mssql

30c43ad

Allow explicit insertion of preset values for mssql IDENTITY columns. These are the equivalent to auto-incrementing primary keys in MySQL, but require special conditional treatment. Closes: typeorm#2199 for mssql

dklischies force-pushed the mssql-2199-support-pk-insert branch from 081091c to 30c43ad Compare October 19, 2020 08:20

dklischies requested a review from imnotjames October 22, 2020 08:23

pleerock merged commit 4abbd46 into typeorm:master Feb 8, 2021

dklischies deleted the mssql-2199-support-pk-insert branch February 8, 2021 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enable explicitly inserting IDENTITY values into mssql #6199

fix: enable explicitly inserting IDENTITY values into mssql #6199

dklischies commented Jun 4, 2020

dklischies commented Jul 17, 2020

pleerock commented Jul 18, 2020

dklischies commented Jul 20, 2020

imnotjames Oct 16, 2020 •

edited

Loading

dklischies Oct 16, 2020

imnotjames Oct 16, 2020

dklischies Oct 16, 2020

EatingJoey Jul 2, 2021

imnotjames commented Oct 16, 2020

imnotjames commented Oct 16, 2020 •

edited

Loading

imnotjames Oct 16, 2020

dklischies Oct 16, 2020

dklischies commented Oct 16, 2020

imnotjames commented Oct 16, 2020

dklischies commented Oct 16, 2020

imnotjames commented Oct 16, 2020 •

edited

Loading

dklischies commented Nov 5, 2020

FabianScheidt commented Dec 14, 2020

pleerock commented Feb 8, 2021

geersch commented Feb 10, 2021 •

edited

Loading

EatingJoey commented Jul 2, 2021 •

edited

Loading

geersch commented Jul 2, 2021

EatingJoey commented Jul 2, 2021

wy193777 commented Apr 26, 2022

dklischies commented Apr 26, 2022

wy193777 commented Apr 26, 2022

wy193777 commented Apr 26, 2022 •

edited

Loading

fix: enable explicitly inserting IDENTITY values into mssql #6199

fix: enable explicitly inserting IDENTITY values into mssql #6199

Conversation

dklischies commented Jun 4, 2020

dklischies commented Jul 17, 2020

pleerock commented Jul 18, 2020

dklischies commented Jul 20, 2020

imnotjames Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

dklischies Oct 16, 2020

Choose a reason for hiding this comment

imnotjames Oct 16, 2020

Choose a reason for hiding this comment

dklischies Oct 16, 2020

Choose a reason for hiding this comment

EatingJoey Jul 2, 2021

Choose a reason for hiding this comment

imnotjames commented Oct 16, 2020

imnotjames commented Oct 16, 2020 • edited Loading

imnotjames Oct 16, 2020

Choose a reason for hiding this comment

dklischies Oct 16, 2020

Choose a reason for hiding this comment

dklischies commented Oct 16, 2020

imnotjames commented Oct 16, 2020

dklischies commented Oct 16, 2020

imnotjames commented Oct 16, 2020 • edited Loading

dklischies commented Nov 5, 2020

FabianScheidt commented Dec 14, 2020

pleerock commented Feb 8, 2021

geersch commented Feb 10, 2021 • edited Loading

EatingJoey commented Jul 2, 2021 • edited Loading

geersch commented Jul 2, 2021

EatingJoey commented Jul 2, 2021

wy193777 commented Apr 26, 2022

dklischies commented Apr 26, 2022

wy193777 commented Apr 26, 2022

wy193777 commented Apr 26, 2022 • edited Loading

imnotjames Oct 16, 2020 •

edited

Loading

imnotjames commented Oct 16, 2020 •

edited

Loading

imnotjames commented Oct 16, 2020 •

edited

Loading

geersch commented Feb 10, 2021 •

edited

Loading

EatingJoey commented Jul 2, 2021 •

edited

Loading

wy193777 commented Apr 26, 2022 •

edited

Loading