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.

Sign up for GitHub

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

Make integration tests for content entity rest endpoints multithreaded again #2124

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

Make integration tests for content entity rest endpoints multithreaded again #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a500930
Make multipart tests able to be run concurrently again
rschev Sep 17, 2024
b021ec2
Fix sending afterCreateEvent too early
rschev Sep 17, 2024
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
6 changes: 3 additions & 3 deletions ...ringframework/data/rest/extensions/entitycontent/RepositoryEntityMultipartController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,6 @@ public ResponseEntity<RepresentationModel<?>> createEntityAndContent(RootResourc
// Save the entity and re-assign the result to savedEntity, so that it exists in the repository before content is added to it.
savedEntity = repoInvokerFactory.getInvokerFor(domainType).invokeSave(savedEntity);

AfterCreateEvent afterCreate = new AfterCreateEvent(savedEntity);
publisher.publishEvent(afterCreate);

StoreInfo info = this.stores.getStore(Store.class, StoreUtils.withStorePath(store));
if (info != null) {
ContentStoreContentService service = new ContentStoreContentService(restConfig, info, repoInvokerFactory.getInvokerFor(domainType), mappingContext, exportedMappingContext, byteRangeRestRequestHandler);
Expand All @@ -130,6 +127,9 @@ public ResponseEntity<RepresentationModel<?>> createEntityAndContent(RootResourc
}
}

AfterCreateEvent afterCreate = new AfterCreateEvent(savedEntity);
publisher.publishEvent(afterCreate);

Comment on lines +130 to +132
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not only changing the integration tests if you're moving the place where this event is emitted.

Copy link
Owner

Choose a reason for hiding this comment

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

This is true. I am not sure I have a strong opinion in this case. I cant thing of a reason why the publish cant be at the end of the call, rather than just after the event. I dont think it materially effects the behavior. WDYT?

Copy link
Contributor

@vierbergenlars vierbergenlars Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The difference is in the ordering of events.

Previously, the AfterCreateEvent is emitted before the set content events (which I believe are emitted internally inside the ContentStoreContentService methods).
Now, the AfterCreateEvent is emitted after the set content events.

It is a behavior that I think should explicitly be called out, not just sneaked in together with some changes in integration tests.

Copy link
Owner

Choose a reason for hiding this comment

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

Right. I would agree. But I doubt many folks are using this controller yet besides you folks. But that is valid. The only reason for this change is because of my general desire to support multi-threaded tests but we lapse in a bunch of places so if you are not comfortable with the event ordering changing then we probably should force it through in a test change.

Optional<PersistentEntityResource> resource = Optional.ofNullable(assembler.toFullResource(savedEntity));
headers.setContentType(new MediaType("application", "hal+json"));

Expand Down
38 changes: 20 additions & 18 deletions ...t/internal/org/springframework/content/rest/controllers/ContentEntityRestEndpointsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@
import java.util.Optional;

import internal.org.springframework.content.rest.support.*;
import java.util.function.Predicate;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.content.commons.store.StoreAccessException;
import org.springframework.content.rest.config.HypermediaConfiguration;
import org.springframework.content.rest.config.RestConfiguration;
import org.springframework.data.domain.Example;
import org.springframework.data.domain.ExampleMatcher;
import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.mock.web.MockMultipartFile;
Expand All @@ -40,7 +42,7 @@
import com.github.paulcwarren.ginkgo4j.Ginkgo4jSpringRunner;

@RunWith(Ginkgo4jSpringRunner.class)
@Ginkgo4jConfiguration(threads=1)
//@Ginkgo4jConfiguration(threads=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code

@WebAppConfiguration
@ContextConfiguration(classes = {
StoreConfig.class,
Expand Down Expand Up @@ -440,25 +442,22 @@ public class ContentEntityRestEndpointsIT {
});

Context("given a multipart/form POST and an event listener", () -> {
BeforeEach(() -> {
eventListener.clear();
});
It("should create a new entity and fire the onBeforeCreate/onAfterCreate events", () -> {

// POST the entity
MockHttpServletResponse response = mvc.perform(multipart("/testEntity3s")
.param("name", "foo foo")
.param("name", "of the rose")
.param("hidden", "bar bar")
.param("ying", "yang")
.param("things", "one", "two"))

.andExpect(status().isCreated())
.andReturn().getResponse();

assertThat(eventListener.getBeforeCreate().size(), is(1));
assertThat(eventListener.getAfterCreate().size(), is(1));
assertThat(((TestEntity3) eventListener.getBeforeCreate().get(0)).getName(), is("foo foo"));
assertThat(((TestEntity3) eventListener.getAfterCreate().get(0)).getName(), is("foo foo"));
Predicate<? super Object> filter = (entity) -> entity instanceof TestEntity3
&& ((TestEntity3) entity).getName().equals("of the rose");
assertThat(eventListener.getBeforeCreate().stream().filter(filter).count(), is(1L));
assertThat(eventListener.getAfterCreate().stream().filter(filter).count(), is(1L));
});
});

Expand All @@ -468,22 +467,25 @@ public class ContentEntityRestEndpointsIT {
MockMultipartFile file = new MockMultipartFile("oopsDoesntExist", "filename.txt", "text/plain",
"foo".getBytes());

var before = repo3.count();

// POST the entity
assertThrows(ServletException.class, () ->
mvc.perform(multipart("/testEntity3s")
.file(file)
.param("name", "foo foo")
.param("hidden", "bar bar")
.param("ying", "yang")
.param("things", "one", "two"))
.param("name", "of the wind")
.param("ying", "dynasty"))

.andExpect(status().isCreated())
.andReturn().getResponse()
);

assertThat(repo3.count(), is(before));
TestEntity3 example = new TestEntity3();
example.setName("of the wind");
example.setYang("dynasty");
assertThat(repo3.findBy(Example.of(example, ExampleMatcher.matching()), q -> q.count()), is(0L));

Predicate<? super Object> filter = (entity) -> entity instanceof TestEntity3
&& ((TestEntity3) entity).getName().equals("of the wind");
assertThat(eventListener.getBeforeCreate().stream().filter(filter).count(), is(1L));
assertThat(eventListener.getAfterCreate().stream().filter(filter).count(), is(0L));
});
});

Expand Down
Loading