danhaywood commented on code in PR #2412:
URL: https://github.com/apache/causeway/pull/2412#discussion_r1611337410
##########
api/applib/src/main/java/org/apache/causeway/applib/services/repository/RepositoryService.java:
##########
@@ -84,6 +85,14 @@ public interface RepositoryService {
*/
<T> T detachedEntity(@NonNull T entity);
+ /**
+ * Enables bulk mode for all {@param aClass} in {@param callable}.
Review Comment:
This javadoc is out of date.
##########
api/applib/src/main/java/org/apache/causeway/applib/services/publishing/spi/EntityPropertyChangeSubscriber.java:
##########
@@ -48,4 +49,6 @@ public interface EntityPropertyChangeSubscriber extends
HasEnabling {
*/
void onChanging(EntityPropertyChange entityPropertyChange);
+ default void onBulkChanging(Can<EntityPropertyChange>
entityPropertyChanges) {
Review Comment:
Shouldn't the default implementation iterate over the `Can<>` and call
`onChanging(...)` ?
##########
extensions/security/audittrail/applib/src/main/java/org/apache/causeway/extensions/audittrail/applib/dom/AuditTrailEntryRepository.java:
##########
@@ -38,6 +39,10 @@ public interface AuditTrailEntryRepository {
AuditTrailEntry createFor(final EntityPropertyChange change);
+ default Can<AuditTrailEntry> createForBulk(final Can<EntityPropertyChange>
entityPropertyChanges) {
+ return Can.empty();
Review Comment:
I think that the default implementation of this should iterate over the
provided `Can<..>` and call the original `createFor(...)`, mapping to the
returned `Can<>`.
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -78,6 +83,7 @@ public class RepositoryServiceDefault
@Getter(onMethod_ = {@Override})
final MetaModelContext metaModelContext;
+ private ThreadLocal<Map<Class, Boolean>> threadLocal =
ThreadLocal.withInitial(HashMap::new);
Review Comment:
give the `threadLocal` field a better name.
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -138,7 +167,9 @@ public void remove(final Object domainObject) {
@Override
public void removeAndFlush(final Object domainObject) {
remove(domainObject);
- transactionService.flushTransaction();
+ if (!threadLocal.get().getOrDefault(domainObject.getClass(),
Boolean.FALSE)) {
Review Comment:
same observation re JPA subtypes.
##########
core/config/src/main/java/org/apache/causeway/core/config/CausewayConfiguration.java:
##########
@@ -2085,6 +2085,19 @@ public static class Po {
Mode mode = Mode.WRITE;
}
}
+
+ private final EntityPropertyChangePublisher
entityPropertyChangePublisher = new EntityPropertyChangePublisher();
+
+ @Data
+ public static class EntityPropertyChangePublisher {
+
+ private final Bulk bulk = new Bulk();
+
+ @Data
+ public static class Bulk {
+ int threshold = 1;
Review Comment:
We need an integration test that demonstrates when the value of this is
changed from its default.
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -120,7 +147,9 @@ public <T> T persist(final T domainObject) {
@Override
public <T> T persistAndFlush(final T object) {
persist(object);
- transactionService.flushTransaction();
+ if (!threadLocal.get().getOrDefault(object.getClass(), Boolean.FALSE))
{
Review Comment:
I'm wondering if this would work for JPA ... doesn't dynamic weaving create
a subclass of the original entity?
In which case, perhaps we can simplify - is there any reason to turn on bulk
mode on a class-by-class basis ... why not simply turn it on "globally" (for
the thread, that is)?
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -99,6 +105,27 @@ public <T> T detachedEntity(final @NonNull T entity) {
return factoryService.detachedEntity(entity);
}
+ @Override
+ public <T> T execInBulk(Callable<T> callable, Class<?>... classes) {
+ try {
+ Arrays.stream(classes).forEach(aClass -> setBulkMode(aClass,
Boolean.TRUE));
+ return callable.call();
+ } catch (Exception e) {
Review Comment:
I don't think this should swallow the exception... it should be propagated
so that the transaction is aborted if there's a problem.
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -99,6 +105,27 @@ public <T> T detachedEntity(final @NonNull T entity) {
return factoryService.detachedEntity(entity);
}
+ @Override
+ public <T> T execInBulk(Callable<T> callable, Class<?>... classes) {
Review Comment:
A unit test for this method would be worthwhile, I think.
##########
extensions/security/audittrail/applib/src/main/java/org/apache/causeway/extensions/audittrail/applib/dom/AuditTrailEntryRepositoryAbstract.java:
##########
@@ -60,12 +63,25 @@ public Class<E> getEntityClass() {
return auditTrailEntryClass;
}
+ @Override
public AuditTrailEntry createFor(final EntityPropertyChange change) {
E entry = factoryService.detachedEntity(auditTrailEntryClass);
entry.init(change);
return repositoryService.persistAndFlush(entry);
}
+ @Override
+ public Can<AuditTrailEntry> createForBulk(Can<EntityPropertyChange>
entityPropertyChanges) {
+ Collection<AuditTrailEntry> auditTrailEntries =
repositoryService.execInBulk(() -> entityPropertyChanges.stream()
Review Comment:
If the definition of the supertype's `default` impl is the same as the
callable, ie:
```
return entityPropertyChanges.stream()
.map(change -> {
E entry =
factoryService.detachedEntity(auditTrailEntryClass);
entry.init(change);
return repositoryService.persist(entry);
}).collect(Collectors.toList());
```
then it would then allow this override to be simplified to something like:
```
return Can.ofCollection(
repositoryService.execInBulk( () ->
super.createForBulk(entityPropertyChanges) )
);
```
##########
persistence/commons/src/main/java/org/apache/causeway/persistence/commons/integration/repository/RepositoryServiceDefault.java:
##########
@@ -99,6 +105,27 @@ public <T> T detachedEntity(final @NonNull T entity) {
return factoryService.detachedEntity(entity);
}
+ @Override
+ public <T> T execInBulk(Callable<T> callable, Class<?>... classes) {
+ try {
+ Arrays.stream(classes).forEach(aClass -> setBulkMode(aClass,
Boolean.TRUE));
+ return callable.call();
+ } catch (Exception e) {
+ log.error("Error executing bulk", e);
+ return null;
+ } finally {
+ Arrays.stream(classes).forEach(aClass -> setBulkMode(aClass,
Boolean.FALSE));
+ }
+ }
+
+ protected <T extends Class> void setBulkMode(final T aClass, final
Boolean bulkMode) {
Review Comment:
Let's make this `private` visibility ; if it were subclassed then it could
change the semantics completely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@causeway.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
