Skip to content

Conversation

DavideD
Copy link
Member

@DavideD DavideD commented Sep 2, 2025

Closes #1906

This is PR #2476 with a few extra commits.
It's just a bit of refactoring that also removes some unused or wrongly included code (probably).

@dreab8, please, could you check these things and let me know if they make sense:

Regarding the tests there are a couple of things that I don't understand.

  1. Why are we checking if the quey contains "update"? Because if we take a test as example, the query looks like this:

    select iav1_0.id,iav1_0.data,iav1_0.name,iav1_0.updateDate from IdentityAndValues iav1_0 where iav1_0.id=$1

    Is the intent to test that it contains the column updateDate or an update sql command? We should at least add a comment

  2. there's a method called assertNumberOfOccurrenceInQueryNoSpace that I don't undestand what it does. Could you add a javadoc with an example of the queries? Maybe specify which databases we support actually run that check.
    Also the assertions looks like this:

    assertThat( actual ).as( "number of " + toCheck ).isEqualTo( expectedNumberOfOccurrences );

    Meaning that a message error looks like this org.opentest4j.AssertionFailedError: [number of data]

    I suppose it should be:

    assertThat( actual ).as( "Unexpecte number of '" + toCheck + "' in the query " + query ).isEqualTo( expectedNumberOfOccurrences );

I think we can merge this PR and close the issue for now (after a review of the extra commits). But let me know.

dreab8 and others added 6 commits September 2, 2025 15:59
This commit :
- Deprecate GeneratedValuesMutationDelegateAdaptor because no longger used
- Introduce ReactiveGetGeneratedKeysDelegate so avoid adapting the SQL String for Mysql and Oracle
- Add support for multi value generation for dialects not supporting returning clause
- Deprecate ReactiveConnection insertAndSelectIdentifier and insertAndSelectIdentifierAsResultSet methods in favour of methods supporting multi generated value
- Implement ReactiveBasicSelectingDelegate for dealing with Identity columns where the dialect requires an additional command execution to retrieve the generated value
- Implement ReaCtiveUniqueKeySelectingDelegate that uses a unique key of the inserted entity to locate the newly inserted row.
It doesn't seem to have any purpose at the moment
It makes it easier to remove the hack once the issue
in  Hibernate ORM is solved: https://hibernate.atlassian.net/browse/HHH-19717
@DavideD DavideD requested a review from dreab8 September 2, 2025 18:20
@DavideD DavideD mentioned this pull request Sep 2, 2025
@@ -38,4 +42,18 @@
default CompletionStage<Id> generate(ReactiveConnectionSupplier session, Object owner, Object currentValue, EventType eventType) {
return generate( session, owner );
}

@Override

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method ReactiveIdentifierGenerator.generate(..) could be confused with overloaded method
generate
, since dispatch depends on static types.
}

@Override
default Object generate(SharedSessionContractImplementor session, Object object){

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method ReactiveIdentifierGenerator.generate(..) could be confused with overloaded method
generate
, since dispatch depends on static types.
…utationDelegateIdentityTest update query assertions
@dreab8
Copy link
Member

dreab8 commented Sep 3, 2025

Hi @DavideD I added a comment to assertNumberOfOccurrenceInQueryNoSpace hoping it clarifies the method role and also fixed the contains update assertions basically we were checking the wrong query (see also hibernate/hibernate-orm#10864).
Marco Belladelli explained that for update and insert we are using contains instead of startsWith because some delegates will prepend some other statements before the insert (e.g. DB2 will use select ... from new table(insert into ...)) or update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support @IdGeneratorType
2 participants