From 9bb491e7312d6f8fd9de4d66c3ccfbf8a8e3257f Mon Sep 17 00:00:00 2001 From: Derek Wasinger Date: Wed, 9 Jun 2021 10:46:14 -0700 Subject: [PATCH] Allow unlaunch and Fix Potential Data Issue (#148) * moved immutable field checks to service layer * updates to tests * reverted to bson update instead of panache * remove commented code --- .../repository/EngagementRepository.java | 23 ++---- .../lodestar/service/EngagementService.java | 71 +++++++++++-------- .../EngagementResourceUpdateTest.java | 8 +-- .../service/EngagementServiceTest.java | 8 +-- .../zrepository/EngagementRepositoryTest.java | 20 +++++- 5 files changed, 71 insertions(+), 59 deletions(-) diff --git a/src/main/java/com/redhat/labs/lodestar/repository/EngagementRepository.java b/src/main/java/com/redhat/labs/lodestar/repository/EngagementRepository.java index ab25d453..3cb8f135 100644 --- a/src/main/java/com/redhat/labs/lodestar/repository/EngagementRepository.java +++ b/src/main/java/com/redhat/labs/lodestar/repository/EngagementRepository.java @@ -5,10 +5,9 @@ import static com.mongodb.client.model.Filters.regex; import static com.mongodb.client.model.Projections.exclude; import static com.mongodb.client.model.Projections.include; -import static com.mongodb.client.model.Updates.combine; import static com.mongodb.client.model.Updates.set; +import static com.mongodb.client.model.Updates.combine; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -61,10 +60,6 @@ public class EngagementRepository implements PanacheMongoRepository private static final String ARTIFACTS = "artifacts"; private static final String ARTIFACTS_TYPE = new StringBuilder(ARTIFACTS).append(".").append(TYPE).toString(); private static final String COUNT = "count"; - private static final String LAUNCH = "launch"; - - private static final List IMMUTABLE_FIELDS = new ArrayList<>( - Arrays.asList("uuid", "mongoId", "projectId", "creationDetails", "status", "commits", LAUNCH)); private ObjectMapper objectMapper = new ObjectMapper(); @@ -135,15 +130,13 @@ public Optional setCommits(String uuid, List commits) { * * @param replacement * @param lastUpdate - * @param skipLaunch * @return */ - public Optional updateEngagementIfLastUpdateMatched(Engagement toUpdate, String lastUpdate, - Boolean skipLaunch) { + public Optional updateEngagement(Engagement toUpdate, String lastUpdate) { // create the bson for filter and update Bson filter = createFilterForEngagement(toUpdate, lastUpdate); - Bson update = createUpdateDocument(toUpdate, skipLaunch); + Bson update = createUpdateDocument(toUpdate); FindOneAndUpdateOptions optionAfter = new FindOneAndUpdateOptions().returnDocument(ReturnDocument.AFTER); @@ -513,10 +506,9 @@ private Bson createFilterForEngagement(Engagement engagement, String lastUpdate) * {@link Engagement}. * * @param engagement - * @param skipLaunch * @return */ - private Bson createUpdateDocument(Engagement engagement, boolean skipLaunch) { + private Bson createUpdateDocument(Engagement engagement) { Bson updates = null; @@ -525,13 +517,6 @@ private Bson createUpdateDocument(Engagement engagement, boolean skipLaunch) { }; Map fieldMap = objectMapper.convertValue(engagement, typeRef); - // remove values that should not be updated - IMMUTABLE_FIELDS.forEach(f -> { - if (!f.equals(LAUNCH) || skipLaunch) { - fieldMap.remove(f); - } - }); - // add a set for each field in the update for (Entry entry : fieldMap.entrySet()) { diff --git a/src/main/java/com/redhat/labs/lodestar/service/EngagementService.java b/src/main/java/com/redhat/labs/lodestar/service/EngagementService.java index e50acbe5..d37cb80e 100644 --- a/src/main/java/com/redhat/labs/lodestar/service/EngagementService.java +++ b/src/main/java/com/redhat/labs/lodestar/service/EngagementService.java @@ -22,7 +22,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.redhat.labs.lodestar.model.Artifact; import com.redhat.labs.lodestar.model.Category; @@ -202,13 +201,12 @@ public Engagement update(Engagement engagement) { () -> new WebApplicationException("no engagement found, use POST to create", HttpStatus.SC_NOT_FOUND)); String currentLastUpdated = engagement.getLastUpdate(); + validateHostingEnvironments(engagement.getHostingEnvironments()); validateSubdomainOnUpdate(engagement); validateCustomerAndProjectNames(engagement, existing); setBeforeUpdate(engagement, existing); - boolean skipLaunch = skipLaunch(existing); - // create copy to send to git api Engagement copy = clone(engagement); @@ -218,7 +216,7 @@ public Engagement update(Engagement engagement) { engagement.getEngagementUsers().stream().forEach(u -> u.setReset(false)); } - Engagement updated = repository.updateEngagementIfLastUpdateMatched(engagement, currentLastUpdated, skipLaunch) + Engagement updated = repository.updateEngagement(engagement, currentLastUpdated) .orElseThrow(() -> new WebApplicationException( "Failed to modify engagement because request contained stale data. Please refresh and try again.", HttpStatus.SC_CONFLICT)); @@ -389,6 +387,8 @@ public Optional getBySubdomain(String subdomain) { */ void setBeforeUpdate(Engagement engagement, Engagement existing) { + setImmutableFieldsOnUpdate(engagement, existing); + setLastUpdate(engagement); // create new or use existing uuids for users @@ -415,6 +415,35 @@ void setBeforeUpdate(Engagement engagement, Engagement existing) { } + /** + * Uses values from the existing {@link Engagement} to set fields that should be + * immutable. + * + * @param engagement + * @param existing + */ + void setImmutableFieldsOnUpdate(Engagement engagement, Engagement existing) { + + // uuid + engagement.setUuid(existing.getUuid()); + + // mongo id + engagement.setMongoId(existing.getMongoId()); + + // project id + engagement.setProjectId(existing.getProjectId()); + + // creation details + engagement.setCreationDetails(existing.getCreationDetails()); + + // status + engagement.setStatus(existing.getStatus()); + + // commits + engagement.setCommits(existing.getCommits()); + + } + /** * Updates the UUID and/or timestamps if the {@link Artifact}, {@link Category}, * {@link UseCase}, {@link Score}, or {@link HostingEnvironment} is new or has @@ -535,17 +564,6 @@ void setLastUpdate(Engagement engagement) { engagement.setLastUpdate(getZuluTimeAsString()); } - /** - * Returns true if the {@link Launch} data is present on the {@link Engagement}. - * Otherwise, false. - * - * @param engagement - * @return - */ - boolean skipLaunch(Engagement engagement) { - return (null != engagement.getLaunch()); - } - /** * Sets the project ID for the {@link Engagement} with the matching UUID. * @@ -640,19 +658,19 @@ public Engagement getByCustomerAndProjectName(String customerName, String projec "no engagement found with customer:project " + customerName + ":" + projectName, HttpStatus.SC_NOT_FOUND)); } - + public Map getEngagementCountByStatus(LocalDateTime currentTime) { - + List engagementList = repository.listAll(); Map statusCounts = new EnumMap<>(EngagementState.class); - + for (Engagement engagement : engagementList) { - EngagementState state = engagement.getEngagementCurrentState(currentTime); - + EngagementState state = engagement.getEngagementCurrentState(currentTime); + int count = statusCounts.containsKey(state) ? statusCounts.get(state) + 1 : 1; statusCounts.put(state, count); } - + statusCounts.put(EngagementState.ANY, engagementList.size()); return statusCounts; @@ -1021,7 +1039,7 @@ public boolean persistEngagementIfNotFound(Engagement engagement) { engagement.setLastUpdate(getZuluTimeAsString()); repository.persist(engagement); - + return true; } @@ -1090,14 +1108,7 @@ String getZuluTimeAsString() { * @return */ Engagement clone(Engagement toClone) { - - try { - return objectMapper.readValue(objectMapper.writeValueAsString(toClone), Engagement.class); - } catch (JsonProcessingException e) { - throw new WebApplicationException("failed to create engagement for event. " + toClone, - HttpStatus.SC_INTERNAL_SERVER_ERROR); - } - + return jsonb.fromJson(jsonb.toJson(toClone), Engagement.class); } } \ No newline at end of file diff --git a/src/test/java/com/redhat/labs/lodestar/resource/EngagementResourceUpdateTest.java b/src/test/java/com/redhat/labs/lodestar/resource/EngagementResourceUpdateTest.java index a64e81ed..2353139b 100644 --- a/src/test/java/com/redhat/labs/lodestar/resource/EngagementResourceUpdateTest.java +++ b/src/test/java/com/redhat/labs/lodestar/resource/EngagementResourceUpdateTest.java @@ -46,7 +46,7 @@ void testPutEngagementWithAuthAndRoleSuccess() throws Exception { toUpdate.setDescription("testing"); Mockito.when(eRepository.findByUuid("1234")).thenReturn(Optional.of(persisted)); - Mockito.when(eRepository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()), Mockito.any())).thenReturn(Optional.of(toUpdate)); + Mockito.when(eRepository.updateEngagement(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()))).thenReturn(Optional.of(toUpdate)); String body = quarkusJsonb.toJson(toUpdate); @@ -145,7 +145,7 @@ void testPutEngagementWithAuthAndRoleDuplicateUsers() throws Exception { toUpdate.setEngagementUsers(Sets.newHashSet(user1, user2, user3)); Mockito.when(eRepository.findByUuid("1234")).thenReturn(Optional.of(persisted)); - Mockito.when(eRepository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()), Mockito.any())).thenReturn(Optional.of(toUpdate)); + Mockito.when(eRepository.updateEngagement(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()))).thenReturn(Optional.of(toUpdate)); String body = quarkusJsonb.toJson(toUpdate); @@ -208,7 +208,7 @@ void testLaunchEngagementWithAuthAndRoleSuccess() throws Exception { toUpdate.setDescription("testing"); Mockito.when(eRepository.findByUuid("1234")).thenReturn(Optional.of(persisted)); - Mockito.when(eRepository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()), Mockito.any())).thenReturn(Optional.of(toUpdate)); + Mockito.when(eRepository.updateEngagement(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()))).thenReturn(Optional.of(toUpdate)); String body = quarkusJsonb.toJson(toUpdate); @@ -358,7 +358,7 @@ void testPutEngagementByNamesWithAuthAndRoleSuccess() throws Exception { toUpdate.setDescription("testing"); Mockito.when(eRepository.findByCustomerNameAndProjectName("c1", "e2")).thenReturn(Optional.of(persisted)); - Mockito.when(eRepository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()), Mockito.any())).thenReturn(Optional.of(toUpdate)); + Mockito.when(eRepository.updateEngagement(Mockito.any(), Mockito.eq(toUpdate.getLastUpdate()))).thenReturn(Optional.of(toUpdate)); String body = quarkusJsonb.toJson(toUpdate); diff --git a/src/test/java/com/redhat/labs/lodestar/service/EngagementServiceTest.java b/src/test/java/com/redhat/labs/lodestar/service/EngagementServiceTest.java index 687762b8..26ad403d 100644 --- a/src/test/java/com/redhat/labs/lodestar/service/EngagementServiceTest.java +++ b/src/test/java/com/redhat/labs/lodestar/service/EngagementServiceTest.java @@ -292,7 +292,7 @@ void testUpdateSuccessNameChange() { Mockito.when(repository.findByUuid("1234")).thenReturn(Optional.of(persisted)); Mockito.when(repository.findByCustomerNameAndProjectName("c3", "p3", new FilterOptions())) .thenReturn(Optional.empty()); - Mockito.when(repository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.any(), Mockito.any())) + Mockito.when(repository.updateEngagement(Mockito.any(), Mockito.any())) .thenReturn(Optional.of(toUpdate)); Engagement updated = service.update(toUpdate); @@ -325,7 +325,7 @@ void testUpdateSuccessNoNameChange() throws Exception { Mockito.when(repository.findByUuid("1234")).thenReturn(Optional.of(persisted)); Mockito.when(repository.findByCustomerNameAndProjectName("c3", "p3", new FilterOptions())) .thenReturn(Optional.empty()); - Mockito.when(repository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.any(), Mockito.any())) + Mockito.when(repository.updateEngagement(Mockito.any(), Mockito.any())) .thenReturn(Optional.of(toUpdate)); Engagement updated = service.update(toUpdate); @@ -836,14 +836,14 @@ void testLaunch() { persisted.setProjectId(1111); Mockito.when(repository.findByUuid("1234")).thenReturn(Optional.of(persisted)); - Mockito.when(repository.updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.any(), Mockito.any())) + Mockito.when(repository.updateEngagement(Mockito.any(), Mockito.any())) .thenReturn(Optional.of(toUpdate)); Engagement updated = service.launch(toUpdate); assertNotNull(updated); assertNotNull(updated.getLaunch()); - Mockito.verify(repository).updateEngagementIfLastUpdateMatched(Mockito.any(), Mockito.any(), Mockito.any()); + Mockito.verify(repository).updateEngagement(Mockito.any(), Mockito.any()); Mockito.verify(eventBus).sendAndForget(Mockito.eq(EventType.UPDATE_ENGAGEMENT_EVENT_ADDRESS), Mockito.any()); } diff --git a/src/test/java/com/redhat/labs/lodestar/zrepository/EngagementRepositoryTest.java b/src/test/java/com/redhat/labs/lodestar/zrepository/EngagementRepositoryTest.java index f968fe27..aa66c8da 100644 --- a/src/test/java/com/redhat/labs/lodestar/zrepository/EngagementRepositoryTest.java +++ b/src/test/java/com/redhat/labs/lodestar/zrepository/EngagementRepositoryTest.java @@ -130,6 +130,22 @@ void testSetProjectIdNotFound() { // set commits // update engagement if last update matched + @Test + void testUpdateEngagementIfLastUpdateMatchedMultipleFieldUpdate() throws Exception { + + Engagement e1 = MockUtils.mockMinimumEngagement("c1", "c2", "1234"); + e1.setLastUpdate("value"); + repository.persist(e1); + + Engagement e2 = MockUtils.cloneEngagement(e1); + e2.setLastUpdate("updated"); + e2.setDescription("testing"); + + Optional optional = repository.updateEngagement(e2, "value"); + assertTrue(optional.isPresent()); + + } + @Test void testUpdateEngagementIfLastUpdateMatched() throws Exception { @@ -140,7 +156,7 @@ void testUpdateEngagementIfLastUpdateMatched() throws Exception { Engagement e2 = MockUtils.cloneEngagement(e1); e2.setDescription("testing"); - Optional optional = repository.updateEngagementIfLastUpdateMatched(e2, "value", false); + Optional optional = repository.updateEngagement(e2, "value"); assertTrue(optional.isPresent()); } @@ -155,7 +171,7 @@ void testUpdateEngagementIfLastUpdateMatchedStale() throws Exception { Engagement e2 = MockUtils.cloneEngagement(e1); e2.setDescription("testing"); - Optional optional = repository.updateEngagementIfLastUpdateMatched(e2, "value2", false); + Optional optional = repository.updateEngagement(e2, "value2"); assertTrue(optional.isEmpty()); }