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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kota65535
Copy link

@kota65535 kota65535 commented Dec 24, 2023

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.

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 24, 2023
* @param dataAccessStrategy must not be {@literal null}.
* @since 1.1
*/
public JdbcAggregateTemplate(ApplicationContext publisher, RelationalMappingContext context, JdbcConverter converter,
public JdbcAggregateTemplate(ApplicationContext publisher, JdbcConverter converter,
Copy link
Author

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?

Copy link
Contributor

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;
Copy link
Author

@kota65535 kota65535 Dec 24, 2023

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

Copy link
Contributor

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) {
Copy link
Author

@kota65535 kota65535 Dec 24, 2023

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).

Copy link
Contributor

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

Copy link
Author

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.

@kota65535 kota65535 force-pushed the jdbc-aggregate-operations-ref branch from bbe3881 to aa2040d Compare January 5, 2024 10:58
@kota65535
Copy link
Author

Hi @mp911de @schauder, could you please take a look?

Copy link
Contributor

@mipo256 mipo256 left a 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,
Copy link
Contributor

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;
Copy link
Contributor

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) {
Copy link
Contributor

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

@kota65535
Copy link
Author

@mikhail2048 Thanks for your review. I've fixed the points you mentined, please check it.

@kota65535
Copy link
Author

@mp911de @schauder Could this PR be included to 3.3 M2? Thank you!

@kota65535
Copy link
Author

@mp911de @schauder @mikhail2048 Could you please review when you have time?

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 9, 2024
@sebutzu
Copy link

sebutzu commented Oct 8, 2024

When can we hope to have this merged? It is quite nasty to use manual workarounds for multiple data sources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple datasources support [DATAJDBC-321]
5 participants