Skip to content
This repository was archived by the owner on Jan 19, 2022. It is now read-only.

Fix auditing when running through DatastoreTemplate.performTransaction #2604

Merged
merged 1 commit into from
Nov 30, 2020
Merged

Fix auditing when running through DatastoreTemplate.performTransaction #2604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions ...re/src/main/java/org/springframework/cloud/gcp/data/datastore/core/DatastoreTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,14 @@ public <A> A performTransaction(Function<DatastoreOperations, A> operations) {
}
return ((Datastore) getDatastoreReadWriter())
.runInTransaction(
(DatastoreReaderWriter readerWriter) -> operations.apply(new DatastoreTemplate(() -> readerWriter,
DatastoreTemplate.this.datastoreEntityConverter,
DatastoreTemplate.this.datastoreMappingContext,
DatastoreTemplate.this.objectToKeyFactory)));
(DatastoreReaderWriter readerWriter) -> {
DatastoreTemplate template = new DatastoreTemplate(() -> readerWriter,
DatastoreTemplate.this.datastoreEntityConverter,
DatastoreTemplate.this.datastoreMappingContext,
DatastoreTemplate.this.objectToKeyFactory);
template.setApplicationEventPublisher(DatastoreTemplate.this.eventPublisher);
return operations.apply(template);
});
}

@Override
Expand Down
19 changes: 18 additions & 1 deletion ...ava/org/springframework/cloud/gcp/data/datastore/core/DatastoreTemplateAuditingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017-2018 the original author or authors.
* Copyright 2017-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -53,6 +53,7 @@
* Tests the auditing features of the template.
*
* @author Chengyuan Zhao
* @author Frank Pavageau
*/
@RunWith(SpringRunner.class)
@ContextConfiguration
Expand All @@ -62,6 +63,8 @@ public class DatastoreTemplateAuditingTests {

@Autowired
DatastoreTemplate datastoreTemplate;
@Autowired
Datastore datastore;

@Test
public void testModifiedNullProperties() {
Expand All @@ -82,6 +85,20 @@ public void testModifiedPrevProperties() {
this.datastoreTemplate.saveAll(Collections.singletonList(testEntity));
}

@Test
public void testInTransaction() {
when(datastore.runInTransaction(any()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see mock setup all done in datastoreTemplate(). Would it make sense to move it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer keeping the specific setup where it's actually used, and Mockito could complain about unnecessary setup in the other tests, depending on its configuration. However, I've tested moving the code and it works, so Mockito is not in strict mode and the code can be arranged however you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly either way. I'm also not sure it's good that the verification is outside the test method, but that's a problem for another time. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I'll leave it that way then, not specifying every possible behavior for all the tests.

I agree the verification in an Answer is brittle, which is when I first ran the test, without the added behavior on the mock, it passed: the Datastore.put() method was never called! We could extract an assertion method which would use an ArgumentCaptor to verify the call and its parameter, instead. However it's not the only occurence in the project of assertions in Answers, so just fixing this one instance probably doesn't really make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no need to address that here.

.thenAnswer(invocation -> {
Datastore.TransactionCallable<?> callable = invocation.getArgument(0);
return callable.run(datastore);
});

TestEntity testEntity = new TestEntity();
testEntity.id = "a";

this.datastoreTemplate.performTransaction(operations -> operations.save(testEntity));
}

/**
* Spring config for the tests.
*/
Expand Down