-
Notifications
You must be signed in to change notification settings - Fork 373
Make Spring Data JDBC work with Spring Data REST and Spring Boot DevTools. #24
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
@schauder I have a demo app using this branch => https://github.com/gregturn/spring-data-jdbc-demo |
This code now works with Spring Data REST, however, it surely isn't the best solution for the issues at hand. |
d1c51f9
to
8a5df30
Compare
@schauder I've run this latest version against our Spring Data JDBC demo (listed above) and it works perfectly, with the following configuration:
|
*/ | ||
package org.springframework.data.jdbc.mybatis; | ||
|
||
import static org.assertj.core.api.Assertions.*; |
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.
Looks we have different code formatters. I have the eclipse formatter configured with the import order coming from spring-data-build/etc/ide/eclipse-formatting.xml
.
I think my configuration matches what I see for example in https://github.com/spring-projects/spring-data-jpa/blob/master/src/test/java/org/springframework/data/jpa/repository/support/QuerydslIntegrationTests.java
Applies to all files.
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.
I'll check my IDE settings.
* @author Jens Schauder | ||
* @author Greg Turnquist | ||
*/ | ||
@RunWith(MockitoJUnitRunner.Silent.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.
Why the Silent
runner? The normal one seems to work just as fine.
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.
Ollie used it in the JPA variant. At point it seemed to make a difference if we don't need it then we don't need it.
} | ||
|
||
private Condition<? super RepositoryFactorySupport> using(JdbcOperations expectedOperations) { | ||
private class DummyJpaRepositoryFactoryBean<T extends CrudRepository<S, ID>, S, ID extends Serializable> |
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.
I guess that should be DummyJdbcRep..., right?
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.
Heh. Yes
assertThat(loadedClass).isNotNull(); | ||
ReflectionUtils.getAllDeclaredMethods(loadedClass); | ||
// Setup standard configuration | ||
factoryBean = new DummyJpaRepositoryFactoryBean<>(DummyEntityRepository.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.
If I replace this with
factoryBean = new JdbcRepositoryFactoryBean<>(DummyEntityRepository.class);
the tests seem to work just as well.
I'm afraid I don't understand what you try to achieve.
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.
I used Jpa's variant of this unit test as a guide.
public void setsUpBasicInstanceCorrectly() { | ||
|
||
private Condition<? super RepositoryFactorySupport> using(NamedParameterJdbcOperations expectedOperations) { | ||
factoryBean.setBeanFactory(beanFactory); |
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 line doesn't seem to have an effect on the outcome of the class.
Class<?> testClass() { | ||
return EnableJdbcRepositoriesIntegrationTests.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.
extra line
In the past, Spring Data JDBC performed autoconfiguration such as gleaning whether or not MyBatis is on the classpath, and also whether or not certain other beans exist. This commit removes such flexible settings and instead wires up a JdbcMappingContext seeking an optional NamingStrategy and optional ConversionCustomizer. The other required beans will alert the end user if they don't exist. All relevant test cases are updated to inject the proper components. All autoconfiguration is being moved outside Spring Data JDBC, to eventually join Spring Boot after being shook out as an independent module.
8a5df30
to
3c395ca
Compare
@schauder I processed all your comments. I also spotted that I wasn't using the Eclipse Code Formatter. Switched that on, using all the settings providing by spring-data-build, and reformatting imports for all touched files. |
Resolved via: b044a13 |
…QL Server. We now provide dialect support for H2, PostgreSQL, and Microsoft SQL Server databases, configurable through AbstractR2dbcConfiguration. By default, we obtain the Dialect by inspecting ConnectionFactoryMetadata to identify the database and the most likely dialect to use. BindableOperation encapsulates statements/queries that can accept parameters. Use BindableOperation for statements through DatabaseClient. Extract SQL creation. Split integration test into abstract base class that can be implemented with a database-specific test class. Original pull request: #24.
Rename Database.latestDialect() to defaultDialect(). Rename Dialect.returnGeneratedKeys() to Dialect.generatedKeysClause(). Simplify IndexBindMarkers. Create BindableOperation.bind(Statement, SettableValue) to reduce code duplicates. Rename BindSpecWrapper to BindSpecAdapter. Original pull request: #24.
No description provided.