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

FISH-7451 Fixes for Issue 6274 #6275

Merged
merged 2 commits into from
Jul 18, 2023
Merged

FISH-7451 Fixes for Issue 6274 #6275

merged 2 commits into from
Jul 18, 2023

Conversation

thehpi
Copy link

@thehpi thehpi commented May 2, 2023

Description

This PR fixes issue 6274.

When using @transactional(Transactional.TxType.REQUIRES_NEW) the interceptor suspends and resumes the transaction but also the transaction in the current invocation. But when resuming it sets the tx in the invocation also when there was none when the interceptor started. This causes issues when using an observer method with @observes(during = TransactionPhase.AFTER_SUCCESS) because when this method is executed the tx is already committed and when calling datasource.getConnection() this causes an exception because it is checking the tx status in the current invocation.

Testing

New tests

No new tests because the current code do not allow for setting or mocking the invocationmanager/current invocation.

Testing Performed

Reran the reproducer, no more exception.

Testing Environment

MacOS Ventura 13.3.1, openjdk 11.0.17 2022-10-18, mvn version 3.8.6

Documentation

See reproducer: https://github.com/thehpi/payara-after-success

Notes for Reviewers

This is a quick fix, it might be better to fix it in TransactionalInterceptorBase so other interceptor implementations are also fixed but I cannot oversee this.

@thehpi
Copy link
Author

thehpi commented May 12, 2023

Dear payara, I found a bug and spent considerable time to a) create a reproducer and b) to create a fix. I would be grateful if you could acknowledge this is a bug and have a look at the fix.

I would be happy to improve on the fix or maybe apply it in other places (interceptors).

@Pandrex247 Pandrex247 changed the title Fixes for Issue 6274 FISH-7451 Fixes for Issue 6274 Jun 19, 2023
@bramn
Copy link

bramn commented Jun 21, 2023

+1 for this bug fix.

@Pandrex247 Pandrex247 added the PR: CLA CLA submitted on PR by the contributor label Jun 26, 2023
@aubi
Copy link
Contributor

aubi commented Jul 3, 2023

Jenkins test please

@aubi
Copy link
Contributor

aubi commented Jul 18, 2023

@thehpi
Thank you for your PR, I was happy to review it and it looks well and works well.
The only problem was the reproducer, PostgreSQL complained during delete about the UUID:

db_1      | ERROR:  operator does not exist: character varying = uuid at character 28
db_1      | HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
db_1      | STATEMENT:  DELETE FROM HANS WHERE (ID = $1)

As the topic of this PR is somewhere else, I modified it to use String instead and I've got the expected results.
So I'm going to merge it.
Thank you!

Copy link
Contributor

@aubi aubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants