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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Sort;
import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
import org.springframework.data.jdbc.core.convert.JdbcConverter;
import org.springframework.data.relational.core.query.Query;
import org.springframework.lang.Nullable;

Expand All @@ -34,6 +36,7 @@
* @author Chirag Tailor
* @author Diego Krupitza
* @author Myeonghyeon Lee
* @author Tomohiko Ozawa
*/
public interface JdbcAggregateOperations {

Expand Down Expand Up @@ -312,4 +315,18 @@ default <T> void delete(T aggregateRoot, Class<T> domainType) {
default <T> void deleteAll(Iterable<? extends T> aggregateRoots, Class<T> domainType) {
deleteAll(aggregateRoots);
}

/**
* Returns the {@link JdbcConverter}.
*
* @return the {@link JdbcConverter}.
*/
JdbcConverter getConverter();

/**
* Return the {@link DataAccessStrategy}
*
* @return the {@link DataAccessStrategy}
*/
DataAccessStrategy getDataAccessStrategy();
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
* @author Myeonghyeon Lee
* @author Chirag Tailor
* @author Diego Krupitza
* @author Tomohiko Ozawa
*/
public class JdbcAggregateTemplate implements JdbcAggregateOperations {

Expand All @@ -86,22 +87,20 @@ public class JdbcAggregateTemplate implements JdbcAggregateOperations {
* {@link DataAccessStrategy}.
*
* @param publisher must not be {@literal null}.
* @param context must not be {@literal null}.
* @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

DataAccessStrategy dataAccessStrategy) {

Assert.notNull(publisher, "ApplicationContext must not be null");
Assert.notNull(context, "RelationalMappingContext must not be null");
Assert.notNull(converter, "RelationalConverter must not be null");
Assert.notNull(dataAccessStrategy, "DataAccessStrategy must not be null");

this.eventDelegate.setPublisher(publisher);
this.context = context;
this.accessStrategy = dataAccessStrategy;
this.converter = converter;
this.accessStrategy = dataAccessStrategy;
this.context = converter.getMappingContext();

this.jdbcEntityDeleteWriter = new RelationalEntityDeleteWriter(context);

Expand All @@ -115,21 +114,19 @@ public JdbcAggregateTemplate(ApplicationContext publisher, RelationalMappingCont
* {@link RelationalMappingContext} and {@link DataAccessStrategy}.
*
* @param publisher must not be {@literal null}.
* @param context must not be {@literal null}.
* @param dataAccessStrategy must not be {@literal null}.
*/
public JdbcAggregateTemplate(ApplicationEventPublisher publisher, RelationalMappingContext context,
JdbcConverter converter, DataAccessStrategy dataAccessStrategy) {
public JdbcAggregateTemplate(ApplicationEventPublisher publisher, JdbcConverter converter,
DataAccessStrategy dataAccessStrategy) {

Assert.notNull(publisher, "ApplicationEventPublisher must not be null");
Assert.notNull(context, "RelationalMappingContext must not be null");
Assert.notNull(converter, "RelationalConverter must not be null");
Assert.notNull(dataAccessStrategy, "DataAccessStrategy must not be null");

this.eventDelegate.setPublisher(publisher);
this.context = context;
this.accessStrategy = dataAccessStrategy;
this.converter = converter;
this.accessStrategy = dataAccessStrategy;
this.context = converter.getMappingContext();

this.jdbcEntityDeleteWriter = new RelationalEntityDeleteWriter(context);
this.executor = new AggregateChangeExecutor(converter, accessStrategy);
Expand Down Expand Up @@ -656,9 +653,19 @@ private <T> T triggerBeforeDelete(@Nullable T aggregateRoot, Object id, MutableA
return null;
}

private record EntityAndPreviousVersion<T> (T entity, @Nullable Number version) {
private record EntityAndPreviousVersion<T>(T entity, @Nullable Number version) {
}

private record EntityAndChangeCreator<T> (T entity, Function<T, RootAggregateChange<T>> changeCreator) {
private record EntityAndChangeCreator<T>(T entity, Function<T, RootAggregateChange<T>> changeCreator) {
}

@Override
public DataAccessStrategy getDataAccessStrategy() {
return accessStrategy;
}

@Override
public JdbcConverter getConverter() {
return converter;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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


import java.sql.Connection;
import java.sql.DatabaseMetaData;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.springframework.beans.BeansException;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.context.annotation.Bean;
Expand All @@ -40,6 +39,7 @@
import org.springframework.data.jdbc.core.dialect.JdbcDialect;
import org.springframework.data.jdbc.core.mapping.JdbcMappingContext;
import org.springframework.data.jdbc.core.mapping.JdbcSimpleTypes;
import org.springframework.data.jdbc.dialect.DialectResolver;
import org.springframework.data.mapping.model.SimpleTypeHolder;
import org.springframework.data.relational.RelationalManagedTypes;
import org.springframework.data.relational.core.conversion.RelationalConverter;
Expand All @@ -61,6 +61,7 @@
* @author Christoph Strobl
* @author Myeonghyeon Lee
* @author Chirag Tailor
* @author Tomohiko Ozawa
* @since 1.1
*/
@Configuration(proxyBeanMethods = false)
Expand Down Expand Up @@ -103,7 +104,7 @@ public RelationalManagedTypes jdbcManagedTypes() throws ClassNotFoundException {
*
* @param namingStrategy optional {@link NamingStrategy}. Use
* {@link org.springframework.data.relational.core.mapping.DefaultNamingStrategy#INSTANCE} as fallback.
* @param customConversions see {@link #jdbcCustomConversions()}.
* @param customConversions see {@link #jdbcCustomConversions(Optional)}.
* @param jdbcManagedTypes JDBC managed types, typically discovered through {@link #jdbcManagedTypes() an entity
* scan}.
* @return must not be {@literal null}.
Expand All @@ -124,7 +125,7 @@ public JdbcMappingContext jdbcMappingContext(Optional<NamingStrategy> namingStra
* {@link #jdbcMappingContext(Optional, JdbcCustomConversions, RelationalManagedTypes)}.
*
* @see #jdbcMappingContext(Optional, JdbcCustomConversions, RelationalManagedTypes)
* @see #jdbcCustomConversions()
* @see #jdbcCustomConversions(Optional)
* @return must not be {@literal null}.
*/
@Bean
Expand All @@ -147,23 +148,17 @@ public JdbcConverter jdbcConverter(JdbcMappingContext mappingContext, NamedParam
* @return will never be {@literal null}.
*/
@Bean
public JdbcCustomConversions jdbcCustomConversions() {

try {

Dialect dialect = applicationContext.getBean(Dialect.class);
SimpleTypeHolder simpleTypeHolder = dialect.simpleTypes().isEmpty() ? JdbcSimpleTypes.HOLDER
: new SimpleTypeHolder(dialect.simpleTypes(), JdbcSimpleTypes.HOLDER);

return new JdbcCustomConversions(
CustomConversions.StoreConversions.of(simpleTypeHolder, storeConverters(dialect)), userConverters());

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

return dialect.map(d -> {
SimpleTypeHolder simpleTypeHolder = d.simpleTypes().isEmpty()
? JdbcSimpleTypes.HOLDER
: new SimpleTypeHolder(d.simpleTypes(), JdbcSimpleTypes.HOLDER);
return new JdbcCustomConversions(CustomConversions.StoreConversions.of(simpleTypeHolder, storeConverters(d)),
userConverters());
}).orElseGet(() -> {
LOG.warn("No dialect found; CustomConversions will be configured without dialect specific conversions");

return new JdbcCustomConversions();
}
});
}

protected List<?> userConverters() {
Expand Down Expand Up @@ -191,7 +186,7 @@ private List<Object> storeConverters(Dialect dialect) {
public JdbcAggregateTemplate jdbcAggregateTemplate(ApplicationContext applicationContext,
JdbcMappingContext mappingContext, JdbcConverter converter, DataAccessStrategy dataAccessStrategy) {

return new JdbcAggregateTemplate(applicationContext, mappingContext, converter, dataAccessStrategy);
return new JdbcAggregateTemplate(applicationContext, converter, dataAccessStrategy);
}

/**
Expand All @@ -207,8 +202,7 @@ public DataAccessStrategy dataAccessStrategyBean(NamedParameterJdbcOperations op

SqlGeneratorSource sqlGeneratorSource = new SqlGeneratorSource(context, jdbcConverter, dialect);
DataAccessStrategyFactory factory = new DataAccessStrategyFactory(sqlGeneratorSource, jdbcConverter, operations,
new SqlParametersFactory(context, jdbcConverter),
new InsertStrategyFactory(operations, dialect));
new SqlParametersFactory(context, jdbcConverter), new InsertStrategyFactory(operations, dialect));

return factory.create();
}
Expand All @@ -219,8 +213,7 @@ public DataAccessStrategy dataAccessStrategyBean(NamedParameterJdbcOperations op
* @param operations the {@link NamedParameterJdbcOperations} allowing access to a {@link java.sql.Connection}.
* @return the {@link Dialect} to be used.
* @since 2.0
* @throws org.springframework.data.jdbc.repository.config.DialectResolver.NoDialectException if the {@link Dialect}
* cannot be determined.
* @throws DialectResolver.NoDialectException if the {@link Dialect} cannot be determined.
*/
@Bean
public Dialect jdbcDialect(NamedParameterJdbcOperations operations) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@
/**
* Configures the name of the {@link org.springframework.data.jdbc.core.convert.DataAccessStrategy} bean definition to
* be used to create repositories discovered through this annotation. Defaults to {@code defaultDataAccessStrategy}.
* @deprecated since 3.3 use {@link #jdbcAggregateOperationsRef()} instead
*/
@Deprecated(since = "3.3")
String dataAccessStrategyRef() default "";

/**
Expand All @@ -133,6 +135,12 @@
*/
String transactionManagerRef() default "transactionManager";

/**
* Configure the name of the {@link org.springframework.data.jdbc.core.JdbcAggregateOperations} bean definition to be
* used to create repositories discovered through this annotation.
*/
String jdbcAggregateOperationsRef() default "";

/**
* Returns the key of the {@link QueryLookupStrategy} to be used for lookup queries for query methods. Defaults to
* {@link QueryLookupStrategy.Key#CREATE_IF_NOT_FOUND}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
* @author Fei Dong
* @author Mark Paluch
* @author Antoine Sauray
* @author Tomohiko Ozawa
*/
public class JdbcRepositoryConfigExtension extends RepositoryConfigurationExtensionSupport {

Expand Down Expand Up @@ -79,9 +80,15 @@ public void postProcess(BeanDefinitionBuilder builder, RepositoryConfigurationSo
Optional<String> transactionManagerRef = source.getAttribute("transactionManagerRef");
builder.addPropertyValue("transactionManager", transactionManagerRef.orElse(DEFAULT_TRANSACTION_MANAGER_BEAN_NAME));

builder.addPropertyValue("mappingContext", new RuntimeBeanReference(JdbcMappingContext.class));
builder.addPropertyValue("dialect", new RuntimeBeanReference(Dialect.class));
builder.addPropertyValue("converter", new RuntimeBeanReference(JdbcConverter.class));
Optional<String> jdbcAggregateOperationsRef = source.getAttribute("jdbcAggregateOperationsRef");

if (jdbcAggregateOperationsRef.isPresent()) {
builder.addPropertyReference("jdbcAggregateOperations", jdbcAggregateOperationsRef.get());
} else {
builder.addPropertyValue("mappingContext", new RuntimeBeanReference(JdbcMappingContext.class));
builder.addPropertyValue("dialect", new RuntimeBeanReference(Dialect.class));
builder.addPropertyValue("converter", new RuntimeBeanReference(JdbcConverter.class));
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@

import org.springframework.beans.factory.BeanFactory;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.data.jdbc.core.JdbcAggregateOperations;
import org.springframework.data.jdbc.core.JdbcAggregateTemplate;
import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
import org.springframework.data.jdbc.core.convert.JdbcConverter;
import org.springframework.data.jdbc.dialect.DialectResolver;
import org.springframework.data.jdbc.repository.QueryMappingConfiguration;
import org.springframework.data.mapping.callback.EntityCallbacks;
import org.springframework.data.relational.core.dialect.Dialect;
Expand All @@ -48,6 +50,7 @@
* @author Hebert Coelho
* @author Diego Krupitza
* @author Christopher Klein
* @author Tomohiko Ozawa
*/
public class JdbcRepositoryFactory extends RepositoryFactorySupport {

Expand All @@ -62,6 +65,20 @@ public class JdbcRepositoryFactory extends RepositoryFactorySupport {
private QueryMappingConfiguration queryMappingConfiguration = QueryMappingConfiguration.EMPTY;
private EntityCallbacks entityCallbacks;

public JdbcRepositoryFactory(ApplicationEventPublisher publisher, JdbcAggregateOperations jdbcAggregateOperations,
NamedParameterJdbcOperations operations) {
Assert.notNull(publisher, "ApplicationEventPublisher must not be null");
Assert.notNull(jdbcAggregateOperations, "JdbcAggregateOperations must not be null");
Assert.notNull(operations, "NamedParameterJdbcOperations must not be null");

this.converter = jdbcAggregateOperations.getConverter();
this.accessStrategy = jdbcAggregateOperations.getDataAccessStrategy();
this.context = jdbcAggregateOperations.getConverter().getMappingContext();
this.dialect = DialectResolver.getDialect(operations.getJdbcOperations());
this.operations = operations;
this.publisher = publisher;
}

/**
* Creates a new {@link JdbcRepositoryFactory} for the given {@link DataAccessStrategy},
* {@link RelationalMappingContext} and {@link ApplicationEventPublisher}.
Expand Down Expand Up @@ -114,7 +131,7 @@ public <T, ID> EntityInformation<T, ID> getEntityInformation(Class<T> aClass) {
@Override
protected Object getTargetRepository(RepositoryInformation repositoryInformation) {

JdbcAggregateTemplate template = new JdbcAggregateTemplate(publisher, context, converter, accessStrategy);
JdbcAggregateTemplate template = new JdbcAggregateTemplate(publisher, converter, accessStrategy);

if (entityCallbacks != null) {
template.setEntityCallbacks(entityCallbacks);
Expand Down
Loading