-
Notifications
You must be signed in to change notification settings - Fork 373
DATAJDBC-123 - MyBatis integration on DbAction level. #16
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
Conversation
If MyBatis-spring is available and a SqlSessionFactory in the Application Context we look for matching mapped sql statements instead of using the default generated SQL. Introduced DataAccessStrategy as a new abstraction level, operating on a single entity. JdbcEntityOperations only contain operations related to complete Aggregates. Thereby also solving DATAJDBC-132. Integration tests ending in HsqlIntegrationTest will only get executed using HsqlDb. Related issue: DATAJDBC-132.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Very clean. I can see how you moved stuff around so you can then slide in the MyBatis implementation without tearing everything to pieces. :)
|
||
void delete(Object id, Class<?> domainType); | ||
|
||
/** Deletes all entities reachable via {@literal propertyPath} from the instance identified by {@literal rootId}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the prose to the next line.
<T> void deleteAll(Class<T> domainType); | ||
|
||
/** Deletes all entities reachable via {@literal propertyPath} from any instance. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
HashMap<String, Object> parameters = new HashMap<>(); | ||
parameters.put("rootId", rootId); | ||
operations.update(format, parameters); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous blank line.
|
||
@Override | ||
public <T> void deleteAll(Class<T> domainType) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
||
@Override | ||
public <T> void deleteAll(PropertyPath propertyPath) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
||
/** | ||
* Generates and executes actual SQL statements. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a little more explanation here? Like "The default {@link DataAccessStrategy} is to generate SQL statements directly."?
|| idProperty == null // | ||
|| (idProperty.getType() == int.class && idValue.equals(0)) // | ||
|| (idProperty.getType() == long.class && idValue.equals(0L)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name seems a little off. What about isIdPropertyNullOrScalarZero
?
delegatingDataAccessStrategy.setDelegate(strategy); | ||
|
||
return new JdbcRepositoryFactory(applicationEventPublisher, context, strategy); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part confuses me.
. The final strategy for the factory is Cascading.
. Cascading contains MyBatis and Delegating.
. Delegating delegates to Cascading, which contains Delegating.
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It IS confusing and I hope it will eventually go away, but currently, I'm not sure about the correct way to resolve this.
The challenge is that the DefaultDataAccessStrategy
when used for reading needs a DataAccessStrategy
for loading referenced entities (see. DefaultDataAccessStrategy.getEntityRowMapper(Class)
(https://github.com/spring-projects/spring-data-jdbc/blob/issue/DATAJDBC-123/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java#L289). But it should use all configured DataAccessStrategies
for this. This basically creates a cyclic dependency, hence the complex structure.
I extract the construction into a separate method with a lengthy comment to explain this.
doReturn(session).when(sessionFactory).openSession(); | ||
doReturn(false).when(session).selectOne(any(), any()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a non-static code block run when the class is created? Or for every test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both. It is an initializer (https://docs.oracle.com/javase/specs/jls/se7/html/jls-12.html#jls-12.1.3), which gets executed during instantiation before any constructor.
And since JUnit creates a new instance for each test it is almost equivalent to a @Before
annotated method. The difference is that any runners (Spring/Mockito) might initialize stuff before a @Before
method, but not before the initializer.
I like them because for me they combine nicely with field initializations and have less ceremony than @Before public void setup()
. As usual, I'm fine with adapting my style to whatever the team prefers/agrees to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turned it into a before method.
|
||
@org.springframework.context.annotation.Configuration | ||
@Import(TestConfiguration.class) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous blank line
Incorporating review comments. Code formatting. Improved comments. Better method name. Converted initialiser to before method.
We now provide an abstract R2DBC configuration that registers beans required for R2DBC's DatabaseClient. Original pull request: #17.
If MyBatis-spring is available and a SqlSessionFactory in the Application Context we look for matching mapped sql statements instead of using the default generated SQL.
Introduced DataAccessStrategy as a new abstraction level, operating on a single entity. JdbcEntityOperations only contain operations related to complete Aggregates. Thereby also solving DATAJDBC-132.
Integration tests ending in HsqlIntegrationTest will only get executed using HsqlDb.
Related issue: DATAJDBC-132.