DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. #1102

jorgerod · 2021-03-17T12:49:41Z

Closes #955.
Original pull request: #1111.

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
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).

mikereiche

It appears that the net result is that instead of CouchbaseTemplateSupport methods being called, now the corresponding ReactiveCouchbaseTemplateSupport methods are being called, resulting in the auditing configured as ReactiveAuditing being called instead of the the (nonReactive)auditing being called. And CouchbaseTemplateSupport is no longer used.

mikereiche · 2021-03-22T21:29:57Z

.../java/org/springframework/data/couchbase/core/ExecutableFindByAnalyticsOperationSupport.java

@@ -53,7 +53,7 @@ public ExecutableFindByAnalyticsOperationSupport(final CouchbaseTemplate templat
 			this.domainType = domainType;
 			this.query = query;
 			this.reactiveSupport = new ReactiveFindByAnalyticsSupport<>(template.reactive(), domainType, query,
-					scanConsistency);
+					scanConsistency, new NonReactiveSupportWrapper(template.support()));


Isn't this NonReactiveSupportWrapper object the same as template.reactive().support() ?
Then this extra argument to the ReactiveSupport constructors is not necessary.
And the ReactiveTemplateSupport support object in the ReactiveSupport objects is not necessary - they can just use template.support().

Hi @mikereiche

In my opinion and following the implementations of other spring-data projects such as spring-data-mongo, I believe that in blocked stack we should launch EntityCallback and from the reactive stack we should launch ReactiveEntityCallback.
To have that differentiation, there needs to be one path for blocking (CouchbaseTemplateSupport) and another path for reactive (ReactiveCouchbaseTemplateSupport).

Also, I think that, additionally, the blocking audit event should be linked to AuditingEntityCallback instead of AuditingEventListener (this is not in the PR but you could add it).

How do you see it?

Got it. Thanks for the explanation.

This point

Also, I think that, additionally, the blocking audit event should be linked to AuditingEntityCallback instead of AuditingEventListener (this is not in the PR but you could add it).

Do you want me to add it to this PR?

Yes please.

mikereiche · 2021-03-22T21:46:00Z

src/main/java/org/springframework/data/couchbase/core/ReactiveInsertByIdOperationSupport.java

-						});
+						.insert(converted.getId(), converted.export(), buildInsertOptions(converted)).flatMap(result ->
+							support.applyUpdatedId(object, converted.getId())
+								.map(updatedObject -> support.applyUpdatedCas(updatedObject, result.cas())));
 			}).onErrorMap(throwable -> {


I had test failures here with. src/test/resources/integration.properties, cluster.type=unmanaged
mvn integration-test
I think this code needs to be as follows (similar to ReactiveUpsertByIdOperationSupport.one()

public Mono<T> one(T object) { return (Mono<T>)Mono.just(object).flatMap(template.support()::encodeEntity).flatMap(converted -> template.getCollection(collection).reactive() .insert(converted.getId(), converted.export(), buildInsertOptions(converted)).flatMap(result -> template.support().applyUpdatedId(object, converted.getId()) .flatMap( updatedObject -> template.support().applyUpdatedCas(updatedObject, result.cas()))) ).onErrorMap(throwable -> { if (throwable instanceof RuntimeException) { return template.potentiallyConvertRuntimeException((RuntimeException) throwable); } else { return throwable; } }); }

Hi @mikereiche

This test fails because if you enable auditing, field annoted with @createdby, @CreatedDate, @LastModifiedBy and @LastModifiedDate are injected.

IMHO, the test is not entirely correct as the assertion should be between the User obtained from userRepository.save(user).block() and the User obtained from userRepository.findById(user.getId()).blockOptional().

Please set src/test/resources/integration.properties, cluster.type=unmanaged
and run mvn integration-test. It gives ClassCastExceptions. If you remove the cast to (Mono<T>) your IDE should tell you that you are returning a <Mono<Mono> instead of a <Mono. The issues is the .map() on line 78 should be a .flatMap().
The code in ReactiveUpsertByIdOperationSupport is correct, just cut-and-paste and change the upsert and buildUpsertOptions to insert and buildInsertOptions.

Yes, it should be comparing the entities from save and find, but that's not causing the ClassCast error.

[ERROR] Errors:
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.existsById:251 ClassCast reactor.cor...
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.insertById:231 ClassCast reactor.cor...
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.insertByIdwithDurability:240 ClassCast
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.saveAndFindImmutableById:266 ClassCast
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.withDurability:115 ClassCast reactor...
[ERROR] CouchbaseTemplateKeyValueIntegrationTests.withExpiryAndExpiryAnnotation:157 ClassCast

mikereiche

There are four fixes in comments, then I will approve and merge it.
It can be manually tested by adding the @EnableReactiveCouchbaseAuditing annotation to ReactiveCouchbaseRepositoryKeyValueIntegrationTests.Config
And the @EnableCouchbaseAuditing annotation to CouchbaseRepositoryKeyValueIntegrationTests.Config
then changing src/test/resources/integration.properties
cluster.type=unmanaged

mikereiche · 2021-03-23T14:55:09Z

src/main/java/org/springframework/data/couchbase/core/ReactiveCouchbaseTemplateSupport.java

+
+	protected <T> Mono<T> maybeCallAfterConvert(T object, CouchbaseDocument document, String collection) {
+		if (null != reactiveEntityCallbacks) {
+			return reactiveEntityCallbacks.callback(AfterConvertCallback.class, object, document, collection);


I think this should use ReactiveAfterConvertCallback.class, right?

mikereiche · 2021-03-23T15:01:12Z

...va/org/springframework/data/couchbase/core/mapping/event/ReactiveAuditingEntityCallback.java

+ * @author Jorge Rodríguez Martín
+ * @since 4.2
+ */
+public class ReactiveAuditingEntityCallback implements ReactiveBeforeConvertCallback<Object>,


Make this also implement ReactiveAfterConvertCallback. Add onAfterConvert(Object, CouchbaseDocument, String)

I think it is not correct to add ReactiveAfterConvertCallback. Audit event should only be associated to a single event (ReactiveBeforeConvertCallback).

If we also add the ReactiveAfterConvertCallback, in each conversion the information of the fields annotated with @createdby, @CreatedDate, @LastModifiedBy and @LastModifiedDate will be modified 2 times and the returned object will have different values for the date type fields than the saved CouchbaseDocument.

mikereiche · 2021-03-23T15:03:27Z

...java/org/springframework/data/couchbase/core/mapping/event/ReactiveAfterConvertCallback.java

+	 * @param collection name of the collection.
+	 * @return a {@link Publisher} emitting the domain object to be persisted.
+	 */
+	Publisher<T> onAfterConvert(T entity, Document document, String collection);


I think this should be CouchbaseDocument (org.springframework.data.couchbase.core.mapping.Document is an annotation)

Closes spring-projects#1105. Co-authored-by: mikereiche <[email protected]>

It was present on non-Reactive, but missing from reactive. Closes spring-projects#1096. Original pull request spring-projects#1108. Co-authored-by: mikereiche <[email protected]>

Add QueryCriteria arrayContaining which maps to n1ql array_containing. Closes spring-projects#1073. Original pull request spring-projects#1109. Co-authored-by: mikereiche <[email protected]>

Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded() and also call that from MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite() Closes spring-projects#1069. Original pull request spring-projects#1112. Co-authored-by: mikereiche <[email protected]>

* Add QueryCriteria arrayContaining. Add QueryCriteria arrayContaining which maps to n1ql array_containing. Closes spring-projects#1073. Original pull request spring-projects#1109. * Support enum parameters to repository queries. Support enums in AbstractCouchbaseConverter.convertForWriteIfNeeded(). Closes spring-projects#1069. Original pull request spring-projects#1110. Co-authored-by: mikereiche <[email protected]>

mikereiche · 2021-03-26T19:53:07Z

Couple little things - they changed the format for Title/Description. The no longer want the DATACOUCH-955 in the title, it should just be "Add support for reactive auditing and ReactiveEntityCallbacks"
And in the description where you have "Fixes #955" it should be as below (Closes, Original pull request:)

Closes #955.
Original pull request: #1102.

I'll have one of my colleagues review it as well before merging.

mikereiche · 2021-03-29T16:48:25Z

@jorgerod -

Can you add a test in ReactiveCouchbaseRepositoryQueryIntegrationTests? And an analogous test in CouchbaseRepositoryIntegrationTests to check for regression.

Add createdBy to Airport

@CreatedBy String createdBy;
public String getCreatedBy(){
	return createdBy;
}

add

@Test
void findBySimplePropertyAudited() {
	Airport vie = null;
	try {
		vie = new Airport("airports::vie", "vie", "low2");
		Airport saved = airportRepository.save(vie).block();
		List<Airport> airports1 = airportRepository.findAllByIata("vie").collectList().block();
		assertEquals(saved, airports1.get(0));
		assertEquals(saved.getCreatedBy(), "auditor"); // NaiveAuditorAware will provide this
	} finally {
		airportRepository.delete(vie).block();
	}
}

Updated the Config class in ReactiveCouchbaseRepositoryQueryIntegrationTests. NaiveAuditorAware and AuditingDateTimeProvider already exist.

@Configuration
@EnableReactiveCouchbaseRepositories("org.springframework.data.couchbase")
@EnableCouchbaseAuditing(auditorAwareRef = "auditorAwareRef", dateTimeProviderRef = "dateTimeProviderRef")
static class Config extends AbstractCouchbaseConfiguration {

	@Override
	public String getConnectionString() {
		return connectionString();
	}

	@Override
	public String getUserName() {
		return config().adminUsername();
	}

	@Override
	public String getPassword() {
		return config().adminPassword();
	}

	@Override
	public String getBucketName() {
		return bucketName();
	}

	@Bean(name = "auditorAwareRef")
	public NaiveAuditorAware testAuditorAware() {
		return new NaiveAuditorAware();
	}

	@Bean(name = "dateTimeProviderRef")
	public DateTimeProvider testDateTimeProvider() {
		return new AuditingDateTimeProvider();
	}

}

…allbacks. Also, adapt CouchbaseAuditingRegistrar for support AuditingEntityCallback. Co-authored-by: Carlos Espinado <carlosemart>

jorgerod · 2021-03-30T08:59:30Z

@mikereiche I have added the tests you asked for

…ta-couchbase into upstream-master

…_auditing

Add support for reactive auditing and ReactiveEntityCallbacks. Also, adapt CouchbaseAuditingRegistrar for support AuditingEntityCallback. Closes #955. Original pull request: #1102. Co-authored-by: Carlos Espinado <carlosemart> Co-authored-by: mikereiche <[email protected]>

mikereiche · 2021-05-27T20:47:53Z

.../java/org/springframework/data/couchbase/repository/auditing/CouchbaseAuditingRegistrar.java

@@ -84,7 +86,7 @@ protected void registerAuditListenerBeanDefinition(BeanDefinition auditingHandle
 				.addConstructorArgValue(ParsingUtils.getObjectFactoryBeanDefinition(getAuditingHandlerBeanName(), registry));

 		registerInfrastructureBeanWithId(listenerBeanDefinitionBuilder.getBeanDefinition(),
-				AuditingEventListener.class.getName(), registry);
+				AuditingEntityCallback.class.getName(), registry);


I think this just changes the name used in registering? On line 84, the rootBeanDefinitions is still AuditingEventListener.class - and after this change, the AuditEventListener is still the listener class is being used. And it already had an onApplicationEvent() that calls markedAudit(). The changed line above can even be changed to as below and it still works. So I don't think this particular change is needed - and I don't think the AuditingEventCallback class is needed.

registerInfrastructureBeanWithId(listenerBeanDefinitionBuilder.getBeanDefinition(),
/*AuditingEntityCallback.class.getName()*/ "My_Friend_Dave", registry);

ok - I just looked at the ReactiveCouchbaseAuditingRegistrar - and it appears that the intention is to use the new AuditEntityCallback.class - both when creating the listenerBeanDefinitionBuilder and when registering. And to no longer use the AuditEventListener.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 17, 2021

jorgerod force-pushed the datacouch_955_add_enable_reactive_auditing branch from 57ce457 to 4c91e66 Compare March 17, 2021 13:03

jorgerod changed the title ~~DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityC…~~ DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. Mar 18, 2021

jorgerod marked this pull request as ready for review March 18, 2021 09:22

Fix indent (spring-projects#1094)

3787fcc

mikereiche requested changes Mar 22, 2021

View reviewed changes

mikereiche reviewed Mar 23, 2021

View reviewed changes

mikereiche and others added 3 commits March 24, 2021 07:00

Update versions to correct values. (spring-projects#1106)

24116ae

Closes spring-projects#1105. Co-authored-by: mikereiche <[email protected]>

Use queryScanConsistency on reactive deleteAll(). (spring-projects#1108)

214b250

It was present on non-Reactive, but missing from reactive. Closes spring-projects#1096. Original pull request spring-projects#1108. Co-authored-by: mikereiche <[email protected]>

Add QueryCriteria arrayContaining. (spring-projects#1110)

521006f

Add QueryCriteria arrayContaining which maps to n1ql array_containing. Closes spring-projects#1073. Original pull request spring-projects#1109. Co-authored-by: mikereiche <[email protected]>

mikereiche added status: ideal-for-contribution An issue that a contributor can help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 24, 2021

mikereiche and others added 2 commits March 24, 2021 14:25

jorgerod force-pushed the datacouch_955_add_enable_reactive_auditing branch 2 times, most recently from e569e59 to 09b1503 Compare March 26, 2021 14:56

jorgerod force-pushed the datacouch_955_add_enable_reactive_auditing branch from 09b1503 to 8db28c9 Compare March 30, 2021 08:35

DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityC…

b0dd3d9

…allbacks. Also, adapt CouchbaseAuditingRegistrar for support AuditingEntityCallback. Co-authored-by: Carlos Espinado <carlosemart>

jorgerod force-pushed the datacouch_955_add_enable_reactive_auditing branch from 8db28c9 to b0dd3d9 Compare March 30, 2021 08:55

mikereiche force-pushed the master branch 3 times, most recently from ef9c5f4 to d16bbff Compare March 30, 2021 21:32

jorgerod added 2 commits April 1, 2021 15:49

Merge branch 'master' of https://github.com/spring-projects/spring-da…

f38cebc

…ta-couchbase into upstream-master

Merge branch 'upstream-master' into datacouch_955_add_enable_reactive…

3f72746

…_auditing

jorgerod requested a review from mikereiche April 1, 2021 15:39

Merge branch 'master' into datacouch_955_add_enable_reactive_auditing

e3a021d

mikereiche merged commit e350f58 into spring-projects:master Apr 6, 2021

jorgerod deleted the datacouch_955_add_enable_reactive_auditing branch April 6, 2021 15:30

mikereiche reviewed May 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. #1102

DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. #1102

jorgerod commented Mar 17, 2021 •

edited

Loading

mikereiche left a comment

mikereiche Mar 22, 2021

jorgerod Mar 23, 2021

mikereiche Mar 23, 2021

jorgerod Mar 23, 2021

mikereiche Mar 23, 2021

jorgerod Mar 26, 2021

mikereiche Mar 22, 2021

jorgerod Mar 26, 2021

mikereiche Mar 26, 2021 •

edited

Loading

jorgerod Mar 26, 2021

mikereiche left a comment

mikereiche Mar 23, 2021

jorgerod Mar 26, 2021

mikereiche Mar 23, 2021

jorgerod Mar 26, 2021

mikereiche Mar 23, 2021

jorgerod Mar 26, 2021

mikereiche commented Mar 26, 2021

mikereiche commented Mar 29, 2021

jorgerod commented Mar 30, 2021

mikereiche May 27, 2021 •

edited

Loading

mikereiche May 27, 2021

DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. #1102

DATACOUCH-955 - Add support for reactive auditing and ReactiveEntityCallbacks. #1102

Conversation

jorgerod commented Mar 17, 2021 • edited Loading

mikereiche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikereiche Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikereiche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikereiche commented Mar 26, 2021

mikereiche commented Mar 29, 2021

jorgerod commented Mar 30, 2021

mikereiche May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgerod commented Mar 17, 2021 •

edited

Loading

mikereiche Mar 26, 2021 •

edited

Loading

mikereiche May 27, 2021 •

edited

Loading