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.

Sign up for GitHub

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

GH-611 added JdbcExceptionTranslator #1956

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-611 added JdbcExceptionTranslator #1956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions ...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@
*/
package org.springframework.data.jdbc.core;

import org.springframework.dao.OptimisticLockingFailureException;
import java.util.List;
import java.util.Optional;
import org.springframework.dao.support.PersistenceExceptionTranslator;
import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
import org.springframework.data.jdbc.core.convert.JdbcConverter;
import org.springframework.data.relational.core.conversion.AggregateChange;
import org.springframework.data.relational.core.conversion.DbAction;
import org.springframework.data.relational.core.conversion.DbActionExecutionException;
import org.springframework.data.relational.core.conversion.MutableAggregateChange;

import java.util.List;

/**
* Executes an {@link MutableAggregateChange}.
*
Expand All @@ -37,11 +37,13 @@ class AggregateChangeExecutor {

private final JdbcConverter converter;
private final DataAccessStrategy accessStrategy;
private final PersistenceExceptionTranslator jdbcExceptionTranslator;

AggregateChangeExecutor(JdbcConverter converter, DataAccessStrategy accessStrategy) {

this.converter = converter;
this.accessStrategy = accessStrategy;
this.jdbcExceptionTranslator = new JdbcExceptionTranslator();
}

/**
Expand Down Expand Up @@ -110,12 +112,15 @@ private void execute(DbAction<?> action, JdbcAggregateChangeExecutionContext exe
} else {
throw new RuntimeException("unexpected action");
}
} catch (Exception e) {

if (e instanceof OptimisticLockingFailureException) {
throw e;
}
throw new DbActionExecutionException(action, e);
} catch (RuntimeException e) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that we should revisit our exception handling. DbActionExecutionException doesn't seem particularly useful to the final application whereas it might be useful for our own internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mp911de Yeah... But this is not a part of this issue. Maybe we can create another issue to revise the default exception to be thrown? Or what should we do?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that we're throwing DbActionExecutionException and not a DAO exception, so I think that is indeed part of the issue. Looking at the code, we do not catch DbActionExecutionException. That exception was introduced as part of introducing better means for debugging (#382) while the net effect was that we changed the API experience. What we could do is add DbActionExecutionException as suppressed exception to the original one.

Paging @schauder.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean by "adding as suppresed exception".

In my understanding Surpressed exceptions are ones that get lost when a finalizer throws an exception itself.

We could instantiate a new exception with the same type as the original with the additional information of the DAEE, or even the DAEE as cause. That would certainly look strange, but might actually be rather useful.

Copy link
Member

Choose a reason for hiding this comment

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

Something along the lines of:

catch(RuntimeException e) {
  e.addSuppressed(new DbActionExecutionException(action));
  throw e;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little bit confused with adding suppressed exception here. We're not really suppressing DbActionExecutionException here. I see it confusing to the end user TBH.

CC: @mp911de @schauder

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know addSuppressed. It is a little hackish, but seems fine to me. It's not worse then the other variants I had in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


throw Optional
.ofNullable(jdbcExceptionTranslator.translateExceptionIfPossible(e))
.map(it -> (RuntimeException) it)
.orElseGet(() -> {
e.addSuppressed(new DbActionExecutionException(action, e));
return e;
});
}
}
}
29 changes: 29 additions & 0 deletions ...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcExceptionTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.springframework.data.jdbc.core;

import java.util.Set;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.DuplicateKeyException;
import org.springframework.dao.OptimisticLockingFailureException;
import org.springframework.dao.support.PersistenceExceptionTranslator;

/**
* {@link PersistenceExceptionTranslator} that is capable to translate exceptions for JDBC module.
*
* @author Mikhail Polivakha
*/
public class JdbcExceptionTranslator implements PersistenceExceptionTranslator {

private static final Set<Class<? extends DataAccessException>> PASS_THROUGH_EXCEPTIONS = Set.of(
OptimisticLockingFailureException.class,
DuplicateKeyException.class
);

@Override
public DataAccessException translateExceptionIfPossible(RuntimeException ex) {
if (PASS_THROUGH_EXCEPTIONS.contains(ex.getClass())) {
return (DataAccessException) ex;
}

return null;
}
}
44 changes: 44 additions & 0 deletions ...ta-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcExceptionTranslatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.springframework.data.jdbc.core;

import java.util.stream.Stream;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.DuplicateKeyException;
import org.springframework.dao.OptimisticLockingFailureException;

/**
* Unit tests for {@link JdbcExceptionTranslator}
*
* @author Mikhail Polivakha
*/
class JdbcExceptionTranslatorTest {

@ParameterizedTest
@MethodSource(value = "passThroughExceptionsSource")
void testPassThroughExceptions(DataAccessException exception) {

// when
DataAccessException translated = new JdbcExceptionTranslator().translateExceptionIfPossible(exception);

// then.
Assertions.assertThat(translated).isSameAs(exception);
}

@Test
void testUnrecognizedException() {

// when
DataAccessException translated = new JdbcExceptionTranslator().translateExceptionIfPossible(new IllegalArgumentException());

// then.
Assertions.assertThat(translated).isNull();
}

static Stream<Arguments> passThroughExceptionsSource() {
return Stream.of(Arguments.of(new OptimisticLockingFailureException("")), Arguments.of(new DuplicateKeyException("")));
}
}
60 changes: 60 additions & 0 deletions ...rc/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.function.Consumer;
import java.util.stream.Stream;

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
Expand All @@ -50,14 +51,17 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.core.io.ClassPathResource;
import org.springframework.dao.DuplicateKeyException;
import org.springframework.dao.IncorrectResultSizeDataAccessException;
import org.springframework.data.annotation.Id;
import org.springframework.data.annotation.Transient;
import org.springframework.data.domain.Example;
import org.springframework.data.domain.ExampleMatcher;
import org.springframework.data.domain.Limit;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Persistable;
import org.springframework.data.domain.ScrollPosition;
import org.springframework.data.domain.Slice;
import org.springframework.data.domain.Sort;
Expand Down Expand Up @@ -113,6 +117,8 @@ public class JdbcRepositoryIntegrationTests {

@Autowired NamedParameterJdbcTemplate template;
@Autowired DummyEntityRepository repository;

@Autowired ProvidedIdEntityRepository providedIdEntityRepository;
@Autowired MyEventListener eventListener;
@Autowired RootRepository rootRepository;

Expand Down Expand Up @@ -193,6 +199,18 @@ public void findAllFindsAllSpecifiedEntities() {
.containsExactlyInAnyOrder(entity.getIdProp(), other.getIdProp());
}

@Test // DATAJDBC-611
public void testDuplicateKeyExceptionIsThrownInCaseOfUniqueKeyViolation() {

// given.
ProvidedIdEntity first = ProvidedIdEntity.newInstance(1L, "name");
ProvidedIdEntity second = ProvidedIdEntity.newInstance(1L, "other");

// when/then
Assertions.assertThatCode(() -> providedIdEntityRepository.save(first)).doesNotThrowAnyException();
Assertions.assertThatThrownBy(() -> providedIdEntityRepository.save(second)).isInstanceOf(DuplicateKeyException.class);
}

@Test // DATAJDBC-97
public void countsEntities() {

Expand Down Expand Up @@ -1421,6 +1439,10 @@ interface DummyProjectExample {
String getName();
}

interface ProvidedIdEntityRepository extends CrudRepository<ProvidedIdEntity, Long> {

}

interface DummyEntityRepository extends CrudRepository<DummyEntity, Long>, QueryByExampleExecutor<DummyEntity> {

@Lock(LockMode.PESSIMISTIC_WRITE)
Expand Down Expand Up @@ -1526,6 +1548,11 @@ DummyEntityRepository dummyEntityRepository() {
return factory.getRepository(DummyEntityRepository.class);
}

@Bean
ProvidedIdEntityRepository providedIdEntityRepository() {
return factory.getRepository(ProvidedIdEntityRepository.class);
}

@Bean
RootRepository rootRepository() {
return factory.getRepository(RootRepository.class);
Expand Down Expand Up @@ -1839,6 +1866,39 @@ private static DummyEntity createEntity(String entityName, Consumer<DummyEntity>
return entity;
}


static class ProvidedIdEntity implements Persistable {

@Id
private Long id;

private String name;

@Transient
private boolean isNew;

private ProvidedIdEntity(Long id, String name, boolean isNew) {
this.id = id;
this.name = name;
this.isNew = isNew;
}

private static ProvidedIdEntity newInstance(Long id, String name) {
return new ProvidedIdEntity(id, name, true);
}

@Override
public Object getId() {
return id;
}

@Override
public boolean isNew() {
return isNew;
}
}


static class DummyEntity {

String name;
Expand Down
9 changes: 8 additions & 1 deletion ...resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-db2.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ DROP TABLE ROOT;
DROP TABLE INTERMEDIATE;
DROP TABLE LEAF;
DROP TABLE WITH_DELIMITED_COLUMN;
DROP TABLE PROVIDED_ID_ENTITY;

CREATE TABLE dummy_entity
(
Expand Down Expand Up @@ -45,4 +46,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY,
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
8 changes: 7 additions & 1 deletion .../resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-h2.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY,
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
8 changes: 7 additions & 1 deletion ...esources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-hsql.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY,
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
8 changes: 7 additions & 1 deletion ...urces/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mariadb.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT AUTO_INCREMENT PRIMARY KEY,
`ORG.XTUNIT.IDENTIFIER` VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
9 changes: 8 additions & 1 deletion ...sources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mssql.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ DROP TABLE IF EXISTS ROOT;
DROP TABLE IF EXISTS INTERMEDIATE;
DROP TABLE IF EXISTS LEAF;
DROP TABLE IF EXISTS WITH_DELIMITED_COLUMN;
DROP TABLE IF EXISTS PROVIDED_ID_ENTITY;

CREATE TABLE dummy_entity
(
Expand Down Expand Up @@ -45,4 +46,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT IDENTITY PRIMARY KEY,
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
8 changes: 7 additions & 1 deletion ...sources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mysql.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,10 @@ CREATE TABLE WITH_DELIMITED_COLUMN
ID BIGINT AUTO_INCREMENT PRIMARY KEY,
`ORG.XTUNIT.IDENTIFIER` VARCHAR(100),
STYPE VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
7 changes: 7 additions & 0 deletions ...ources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-oracle.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ DROP TABLE ROOT CASCADE CONSTRAINTS PURGE;
DROP TABLE INTERMEDIATE CASCADE CONSTRAINTS PURGE;
DROP TABLE LEAF CASCADE CONSTRAINTS PURGE;
DROP TABLE WITH_DELIMITED_COLUMN CASCADE CONSTRAINTS PURGE;
DROP TABLE PROVIDED_ID_ENTITY CASCADE CONSTRAINTS PURGE;

CREATE TABLE DUMMY_ENTITY
(
Expand Down Expand Up @@ -46,3 +47,9 @@ CREATE TABLE WITH_DELIMITED_COLUMN
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
STYPE VARCHAR(100)
)

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
9 changes: 8 additions & 1 deletion ...rces/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-postgres.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ DROP TABLE ROOT;
DROP TABLE INTERMEDIATE;
DROP TABLE LEAF;
DROP TABLE WITH_DELIMITED_COLUMN;
DROP TABLE PROVIDED_ID_ENTITY;

CREATE TABLE dummy_entity
(
Expand Down Expand Up @@ -45,4 +46,10 @@ CREATE TABLE "WITH_DELIMITED_COLUMN"
ID SERIAL PRIMARY KEY,
"ORG.XTUNIT.IDENTIFIER" VARCHAR(100),
"STYPE" VARCHAR(100)
);
);

CREATE TABLE PROVIDED_ID_ENTITY
(
ID BIGINT PRIMARY KEY,
NAME VARCHAR(30)
);
Loading