Bug 579409: Add support for Statement.getGeneratedKeys for Identity

[dazey3]

Linking to BZ #579409 Linking to BZ #288015

When persisting entity values, that use IDENTITY generation, EclipseLink may obtain an incorrect IDENTITY on SQLServer. If the SQLServer database has added TRIGGERs that generate IDENTITY values before EclipseLink is able to query for the correct value, the wrong value is obtained.

When persisting an entity, that uses IDENTITY generation, EclipseLink performs an INSERT followed by a separate query to obtain the ID value:

    [EL Fine]: sql: Connection(1940526571)--Thread(Thread[main,5,main])--INSERT INTO COFFEE(NAME, PRICE) VALUES (?, ?)
	bind => [Capa, 4.2]
...
    [EL Finest]: query: Thread(Thread[main,5,main])--Execute query ValueReadQuery(name="SEQ_GEN_IDENTITY" sql="SELECT @@IDENTITY")
    [EL Fine]: sql: 2022-03-24 11:30:15.807--ClientSession(1647497427)--Connection(1940526571)--Thread(Thread[main,5,main])--SELECT @@IDENTITY

Notice that in this trace from SQLServer, EclipseLink executes SELECT @@IDENTITY to obtain the ID value. The problem is that this value returned from SELECT @@IDENTITY may not be correct. If another process generates an IDENTITY value BEFORE EclipseLink can execute this separate query, then the value returned will not be the one matching the actual entity inserted into the table.

---

Table:

    CREATE TABLE COFFEE (ID NUMERIC(19) IDENTITY NOT NULL, NAME VARCHAR(255) NULL, PRICE FLOAT(32) NULL, PRIMARY KEY (ID))

For instance, consider the following example. If the SQLServer database contains the following additional, non-JPA table that also uses IDENTITY generation:

    CREATE TABLE COFFEE_AUDIT (AUDIT_ID NUMERIC(19) IDENTITY NOT NULL, COFFEE_ID NUMERIC(19) NOT NULL, NAME VARCHAR(255) NULL, PRICE FLOAT(32) NULL, PRIMARY KEY (AUDIT_ID))

and a TRIGGER to insert into that table:

    CREATE TRIGGER coffeetrigger on COFFEE AFTER INSERT as BEGIN SET NOCOUNT ON; INSERT INTO COFFEE_AUDIT( COFFEE_ID, NAME, PRICE ) SELECT i.ID, i.NAME, i.PRICE FROM inserted i END

What ends up occurring is that EclipseLink will persist and INSERT into the Entity table:

    INSERT INTO COFFFEE (NAME, PRICE) VALUES (?, ?)
	bind => [Capa, 4.2]

This will cause an IDENTITY generation for COFFFEE.ID. Let's say this is the first row to be inserted into this table, so the COFFFEE.ID value generated is "1"

Then, the SQLServer TRIGGER to INSERT into COFFEE_AUDIT will occur, outside of EclipseLink's control:

    INSERT INTO COFFEE_AUDIT (COFFEE_ID, NAME, PRICE) VALUES (2, 'Caoa', 4.2)

This will cause an IDENTITY generation for COFFEE_AUDIT.AUDIT_ID. Let's say the COFFEE_AUDIT table has 3 previous rows in the table, so the COFFEE_AUDIT.AUDIT_ID value generated is "4".

Finally, EclipseLink will execute the separate identity query:

    SELECT @@IDENTITY

The value returned from SELECT @@IDENTITY will be "4"; the value for the COFFEE_AUDIT.AUDIT_ID table, not the value generated for COFFFEE.ID.

[edburns]

Hello @dazey3 , I'm keenly interested in this one for one of our customers. Do you have any ETA?

[dazey3]

@edburns @lukasj @rfelcman I appreciate the interest! I have posted a potential fix for the development HEAD (master) and am now just awaiting other members of the community to provide a code review and/or feedback before I can merge and backport. Unfortunately, I cannot provide an ETA on when they may get around to taking a look. Hopefully soon as I am also working on this for a customer.

Feel free to provide your own feedback as well. For instance, when I backport to EclipseLink 2.7 and EclipseLink 3.0, the fix will not be enabled by default and will instead require a persistence property to enable. I decided to go with this choice in order to preserve existing behavior as not all platforms support / require this fix and testing all platforms to prevent behavior breaks is not achievable. Would requiring a persistence property be acceptable to your customer?

[lukasj]

I have it on my plate tonight, so probably by the end of this week/early next week (as I'll by off for few days)

[dazey3]

@lukasj I really appreciate it! Thank you for taking a look!

[lukasj]

btw @dazey3 let us know if you want this in 3.0.3, so we can postpone the next RC a bit

[dazey3]

@lukasj

let us know if you want this in 3.0.3, so we can postpone the next RC a bit

That would be beneficial. Hopefully there are no major issues with my proposed changes. This is priority work for me, so let me know if there are any questions or concerns so I can make changes.

[edburns]

@lukasj @dazey3 any update? It would indeed be great to get this into 3.0.3.

[MelleD]

@dazey3 Is there a plan to backport it to 2.7.x?

[dazey3]

@MelleD

Is there a plan to backport it to 2.7.x?

Yes. I plan on backporting this to 3.0, 2.7, 2.6_WAS. I would backport to 3.1, but I'm not sure how. Maybe @rfelcman or @lukasj can explain how I can port changes into that release branch?

[MelleD]

Nice :)