-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add jdbcAggregateOperationsRef
to @EnableJdbcRepositories
#1704
base: main
Are you sure you want to change the base?
Add jdbcAggregateOperationsRef
to @EnableJdbcRepositories
#1704
Conversation
* @param dataAccessStrategy must not be {@literal null}. | ||
* @since 1.1 | ||
*/ | ||
public JdbcAggregateTemplate(ApplicationContext publisher, RelationalMappingContext context, JdbcConverter converter, | ||
public JdbcAggregateTemplate(ApplicationContext publisher, JdbcConverter converter, |
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.
Removed RelationalMappingContext because it can be obtained by JdbcConverter instance, but should it be kept for backward compatibility?
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 think it is better to create a new constructor here and mark the previous as deprecated in favor of new. We should not change public API for backward compatibility
@@ -13,7 +13,7 @@ | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
*/ | |||
package org.springframework.data.jdbc.repository.config; | |||
package org.springframework.data.jdbc.dialect; |
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.
Moved package to avoid circular dependency between package repository.config
and repository.support
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.
Agree, in r2dbc we already have it in dialect
package
|
||
} catch (NoSuchBeanDefinitionException exception) { | ||
|
||
public JdbcCustomConversions jdbcCustomConversions(Optional<Dialect> dialect) { |
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.
Optional bean dependency can be expressed by Optional
, which enables us to create JdbcCustomConversions beans easier for each data source (example).
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 change does not affect the feature and introduces the non-compatible change. Let's segregate it into separate PR so it can be merged in the next major release
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.
OK, I'll revert this change for now, and then create another PR that adds a factory class later.
bbe3881
to
aa2040d
Compare
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.
In general good, but I think that polishing and breaking changes caused by this should be moved into different commit, so it can be incorporated later.
* @param dataAccessStrategy must not be {@literal null}. | ||
* @since 1.1 | ||
*/ | ||
public JdbcAggregateTemplate(ApplicationContext publisher, RelationalMappingContext context, JdbcConverter converter, | ||
public JdbcAggregateTemplate(ApplicationContext publisher, JdbcConverter converter, |
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 think it is better to create a new constructor here and mark the previous as deprecated in favor of new. We should not change public API for backward compatibility
@@ -13,7 +13,7 @@ | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
*/ | |||
package org.springframework.data.jdbc.repository.config; | |||
package org.springframework.data.jdbc.dialect; |
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.
Agree, in r2dbc we already have it in dialect
package
|
||
} catch (NoSuchBeanDefinitionException exception) { | ||
|
||
public JdbcCustomConversions jdbcCustomConversions(Optional<Dialect> dialect) { |
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 change does not affect the feature and introduces the non-compatible change. Let's segregate it into separate PR so it can be merged in the next major release
@mikhail2048 Thanks for your review. I've fixed the points you mentined, please check it. |
@mp911de @schauder @mikhail2048 Could you please review when you have time? |
When can we hope to have this merged? It is quite nasty to use manual workarounds for multiple data sources. |
fix: #544, #687, #994
This PR adds
jdbcAggregateOperationsRef
to@EnableJdbcRepositories
that enables to specify JdbcAggregateOperations bean name to be used for creating repositories, which significantly simplifies the configuration for multiple repositories.I've also created a sample project for testing this feature. Multiple repositories with different dialects can be configured using two configurations (Db1Config.java, Db2Config.java).
As you can see, creating the JdbcAggregateOperations bean is still a bit messy. It may be better to have some factory class for creating them.