Pull request to fix the issue: #655

[andreconrado]

Pull request to fix the issue: #655

Refactor indexViolation to handle wrapped exceptions and improve clarity.

• Unwrap UndeclaredThrowableException for proxy-related exceptions.
• Unwrap InvocationTargetException for reflection-related exceptions.
• Add null-safe handling for exception messages.
• Improve readability of PostgreSQL-specific checks.
• Improve readability of SQL Server-specific checks.
• Ensure compatibility with standard JDBC constraint violations.

[badgerwithagun]

Hey @andreconrado . This looks great. Could you add a cross-dialect test for this? Adding it to AbstractPersistorTest should mean it's tested for all dialects.

[badgerwithagun]

Is this actually to fix #655 ?

[andreconrado]

Yes, sorry, I had it wrong in the title and description, I copied the wrong link 😁
I’ve changed it now.

[andreconrado]

Hi @badgerwithagun,

I only added some validations before checking the source of the Exception, so the existing tests should confirm that my changes work - or at least don’t affect what was already there.

In this case, do I need to change the tests?
That would mean more work/time, since I’d have to analyze how to modify/create the test.

[badgerwithagun]

@andreconrado

It's pretty straightforward; I'm happy to help.

Obviously for the existing scenarios we have this test.

You've added support for wrapped exceptions, so all we need to do is create a version of that test which causes the exception to be wrapped. First thing I would try is something like this.

In this new test, copy the existing one but change:

txManager().inTransactionThrows(tx -> persistor().save(tx, entry1));

To

txManager().inTransactionThrows(tx -> {
  var wrappedTxn =  Mockito.spy(tx);
  Mockito.when(wrappedTxn.prepareBatchStatement(any()))
    .thenAnswer(args -> {
      var wrappedStmt = Mockito.spy(args.callRealMethod());
      Mockito.doAnswer(args -> {
        try {
          args.callRealMethod();
          return null;
        } catch (Exception e) {
          throw new UndeclaredThrowableException(e);
        }
      }).when(wrappedStmt.executeUpdate());
      return wrappedStmt;
    });
  persistor().save(wrappedTxn, entry1);
});

This should result in any exceptions in the save method being wrapped in UndeclaredThrowableException. Not tested the code. Probably doesn't compile. But should speed things along.

[andreconrado]

Hi @badgerwithagun, thanks for the tips!

I think I’ll still need your help. The line var wrappedTxn = Mockito.spy(tx); gives an error:

Caused by: org.mockito.exceptions.base.MockitoException: 
Cannot mock/spy class com.gruelbox.transactionoutbox.spi.SimpleTransaction
Mockito cannot mock/spy the following:

The SimpleTransaction class is final, so I can’t mock it. Is there an example I can use as a reference to mock this?

[andreconrado]

Hi again @badgerwithagun,

To be able to run the test, we need to change the dependency from:

<dependency>
  <groupId>org.mockito</groupId>
  <artifactId>mockito-all</artifactId>
  <version>1.10.19</version>
  <scope>test</scope>
</dependency>

to:

<dependency>
  <groupId>org.mockito</groupId>
  <artifactId>mockito-core</artifactId>
  <version>5.19.0</version>
  <scope>test</scope>
</dependency>

Are you interested in making this change? Is it worth it just for this test?

Thanks.

[andreconrado]

Hi again @badgerwithagun ,

Have you been able to look into this situation?

Thanks.

[badgerwithagun]

Are we on a really old Mockito? Yes quite happy to update.

[andreconrado]

Hi @badgerwithagun

Having a more recent version normally is always better, but I’m not sure it’s worth doing it just for these tests.

I’ve merged the latest changes, and this PR no longer has any conflicts.

[andreconrado]

Hi again @badgerwithagun ,

Have you been able to look into this situation?

Thanks.

[badgerwithagun]

Hi @andreconrado , apologies for letting this gather dust.

AbstractPersistorTest runs for every persistor implementation (e.g. https://github.com/gruelbox/transaction-outbox/blob/master/transactionoutbox-acceptance/src/test/java/com/gruelbox/transactionoutbox/acceptance/TestMSSqlServer2022.java).

Each implementation specifies the TransactionManager implementation to use and it's the behaviour of the transaction manager that you are testing here. The Transaction implementtion is intentionally final.

Rather than messing around with mocking final classes (which is a debatable pattern) I suggest you:

1. Modify your test in AbstractPersistorTest to simply test that duplicate records are handled correctly without messing around with the Transaction.
2. Create a test in transactionoutbox-acceptance/src/test/java/com/gruelbox/transactionoutbox/acceptance called something like BadTransactionManagerTest. Copy TestDefaultPersistorH2 to create it and replace

  private final TransactionManager txManager =
      TransactionManager.fromConnectionDetails(
          "org.h2.Driver",
          "jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DEFAULT_LOCK_TIMEOUT=2000;LOB_TIMEOUT=2000;MV_STORE=TRUE",
          "test",
          "test");

with a transaction manager that displays the error you are trying to protect against.

The key thing here is to start from whatever mistake a developer might make, simulate that mistake and work backwards from there.

[badgerwithagun]

Closing stale PR