From 46f3e7767b68ecd9bde1d4d927f980af9dbb14d9 Mon Sep 17 00:00:00 2001 From: jfaulkner Date: Thu, 2 Nov 2023 17:17:48 +0000 Subject: [PATCH 1/5] fix: Handle firing delete events twice --- .../state/DefaultEntityViewUpdater.java | 20 ++++++++++++++++++- .../state/EntityViewUpdaterTest.java | 19 ++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java b/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java index de6d0cb12..0d11f06ce 100644 --- a/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java +++ b/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java @@ -90,7 +90,7 @@ private , S extends Specification> Entity update(S } private , S extends Specification> Entity delete(SpecificationDeletionEvent event) { - val oldEntity = (Entity) getExistingEntity(event.getKey()); + val oldEntity = (Entity) getPossiblyDeletedEntity(event.getKey()); entities.put(event.getKey(), deleted(oldEntity)); log.debug("Deleted entity for {}", event.getKey()); return oldEntity; @@ -108,6 +108,24 @@ private , S extends Specification> Entity delete(S return oldEntity; } + /** + * There is a chance the entity will have already been deleted. + */ + private Entity getPossiblyDeletedEntity(Entity.Key key) { + val stateValue = Optional.ofNullable(entities.get(key)); + stateValue.ifPresent(it -> { + if (it.deleted) { + log.debug("Found deleted entity for key={}", key); + } else { + log.debug("Found entity for key={}", key); + } + } + ); + return stateValue + .map(it -> it.entity) + .orElse(null); + } + private Entity getExistingEntity(Entity.Key key) { return Optional.ofNullable(entities.get(key)) .filter(it -> !it.deleted) diff --git a/state/core/src/test/java/com/expediagroup/streamplatform/streamregistry/state/EntityViewUpdaterTest.java b/state/core/src/test/java/com/expediagroup/streamplatform/streamregistry/state/EntityViewUpdaterTest.java index 42ff199a2..a57d17330 100644 --- a/state/core/src/test/java/com/expediagroup/streamplatform/streamregistry/state/EntityViewUpdaterTest.java +++ b/state/core/src/test/java/com/expediagroup/streamplatform/streamregistry/state/EntityViewUpdaterTest.java @@ -138,6 +138,25 @@ public void deleteSpecificationEvent() { assertThat(entities, hasEntry(key, deleted(entity.withStatus(oldStatus)))); } + @Test + public void doubleDeleteSpecificationEvent() { + val previousEntity = underTest.update(specificationEvent); + + assertThat(previousEntity, is(nullValue())); + assertThat(entities, is(aMapWithSize(1))); + assertThat(entities, hasEntry(key, existing(entity.withStatus(oldStatus)))); + + val deletedEntity = underTest.update(specificationDeletionEvent); + assertThat(deletedEntity, is(entity.withStatus(oldStatus))); + assertThat(entities, is(aMapWithSize(1))); + assertThat(entities, hasEntry(key, deleted(entity.withStatus(oldStatus)))); + + val doubleDeletedEntity = underTest.update(specificationDeletionEvent); + assertThat(doubleDeletedEntity, is(entity.withStatus(oldStatus))); + assertThat(entities, is(aMapWithSize(1))); + assertThat(entities, hasEntry(key, deleted(entity.withStatus(oldStatus)))); + } + @Test public void deleteStatusEvent() { entities.put(key, existing(oldEntity)); From 8206f0db23b9290ae1a5920df226b4c3230e6c0c Mon Sep 17 00:00:00 2001 From: jfaulkner Date: Thu, 2 Nov 2023 17:23:33 +0000 Subject: [PATCH 2/5] fix: CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3cffecb7..769fe2352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [unreleased] ### Fixed - KafkaEventReceiver progress logger will actually log the process during normal application bootstrapping. +- `DefaultEntityViewUpdater` to handle receiving double delete events correctly. ## [1.4.2] 2023-09-21 ### No change From 6b9473075f0ca97363af7398cdffbf8a828ed8c8 Mon Sep 17 00:00:00 2001 From: jfaulkner Date: Thu, 2 Nov 2023 17:28:23 +0000 Subject: [PATCH 3/5] fix: Attempting IT. --- .../streamregistry/state/it/AgentIT.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java b/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java index c0fb2b44d..578264f99 100644 --- a/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java +++ b/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java @@ -180,6 +180,21 @@ public void testDeletedEntities() { assertThat(dummyAgent.events, hasItem(Pair.of(data.getEntity(), specificationDeletion(data.getKey())))); // the agent would be expected to handle the deletion event. }); + // this shouldn't happen very often, but sending the same delete event should result in the old deleted entity still being available. + sendSync(kafkaEventSender, specificationDeletion(data.getKey())); + await.untilAsserted(() -> { + // entity no longer exists + assertThat(domainEvents(entityView), hasSize(0)); + // entity is marked as deleted + assertThat(deletedDomainEvents(entityView), is(aMapWithSize(1))); + assertThat(deletedDomainEvents(entityView), hasEntry(data.getKey(), Optional.of(data.getEntity()))); + + // onEvent has been called for the deleted entity + assertThat(dummyAgent.events, hasSize(2)); + assertThat(dummyAgent.events, hasItem(Pair.of(null, data.getSpecificationEvent()))); + assertThat(dummyAgent.events, hasItem(Pair.of(data.getEntity(), specificationDeletion(data.getKey())))); // the agent would be expected to handle the deletion event. + }); + // purge would be called by the agent after the delete has been handled entityView.purgeDeleted(data.getKey()); // the delete is removed from everywhere From 0cef7ebb7e0ba3e91cebc6f40b602222332c1a24 Mon Sep 17 00:00:00 2001 From: jfaulkner Date: Fri, 3 Nov 2023 09:42:38 +0000 Subject: [PATCH 4/5] fix: Attempting to fix IT. --- .../streamplatform/streamregistry/state/it/AgentIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java b/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java index 578264f99..a38891237 100644 --- a/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java +++ b/state/it/src/test/java/com/expediagroup/streamplatform/streamregistry/state/it/AgentIT.java @@ -189,8 +189,8 @@ public void testDeletedEntities() { assertThat(deletedDomainEvents(entityView), is(aMapWithSize(1))); assertThat(deletedDomainEvents(entityView), hasEntry(data.getKey(), Optional.of(data.getEntity()))); - // onEvent has been called for the deleted entity - assertThat(dummyAgent.events, hasSize(2)); + // onEvent has been called for the deleted entity twice + assertThat(dummyAgent.events, hasSize(3)); assertThat(dummyAgent.events, hasItem(Pair.of(null, data.getSpecificationEvent()))); assertThat(dummyAgent.events, hasItem(Pair.of(data.getEntity(), specificationDeletion(data.getKey())))); // the agent would be expected to handle the deletion event. }); From 0438588547edb864c7bdfd367a1f732ba2bccbb1 Mon Sep 17 00:00:00 2001 From: jfaulkner Date: Fri, 3 Nov 2023 09:58:25 +0000 Subject: [PATCH 5/5] fix: merge --- CHANGELOG.md | 3 +-- .../state/DefaultEntityViewUpdater.java | 20 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 091d0ad0c..8e89bf184 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [unreleased] ### Fixed -- KafkaEventReceiver progress logger will actually log the process during normal application bootstrapping. - `DefaultEntityViewUpdater` to handle receiving double delete events correctly. -## [unreleased] +## [2.0.0] 2023-11-02 ### Fixed - KafkaEventReceiver progress logger will actually log the process during normal application bootstrapping. - Fixing test dependencies to be able to run on modern Mac hardware, Java 17 and Docker Desktop versions diff --git a/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java b/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java index 4377883c8..0f4c18a81 100644 --- a/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java +++ b/state/core/src/main/java/com/expediagroup/streamplatform/streamregistry/state/DefaultEntityViewUpdater.java @@ -91,7 +91,7 @@ private , S extends Specification> Entity update(S } private , S extends Specification> Entity delete(SpecificationDeletionEvent event) { - val oldEntity = (Entity) getPossiblyDeletedEntity(event.getKey()); + val oldEntity = (Entity) getEntity(event.getKey()); entities.put(event.getKey(), deleted(oldEntity)); log.debug("Deleted entity for {}", event.getKey()); return oldEntity; @@ -109,10 +109,17 @@ private , S extends Specification> Entity delete(S return oldEntity; } + private Entity getExistingEntity(Entity.Key key) { + return Optional.ofNullable(entities.get(key)) + .filter(it -> !it.deleted) + .map(it -> it.entity) + .orElse(null); + } + /** - * There is a chance the entity will have already been deleted. + * There is a chance the entity will have already been deleted. Only use this method if you don't care. */ - private Entity getPossiblyDeletedEntity(Entity.Key key) { + private Entity getEntity(Entity.Key key) { val stateValue = Optional.ofNullable(entities.get(key)); stateValue.ifPresent(it -> { if (it.deleted) { @@ -126,11 +133,4 @@ private , S extends Specification> Entity delete(S .map(it -> it.entity) .orElse(null); } - - private Entity getExistingEntity(Entity.Key key) { - return Optional.ofNullable(entities.get(key)) - .filter(it -> !it.deleted) - .map(it -> it.entity) - .orElse(null); - } }