From 7b4dd59c58b0db0367544b9fe5e988dfcf081b9c Mon Sep 17 00:00:00 2001 From: guqing <38999863+guqing@users.noreply.github.com> Date: Mon, 24 Oct 2022 15:44:10 +0800 Subject: [PATCH] refactor: cascade delete snapshots and comments when post or page deleted (#2601) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### What type of PR is this? /kind bug /area core /milestone 2.0 #### What this PR does / why we need it: 当删除文章或自定义页面时级联删除内容快照和评论及计数器 see also #2602 #### Which issue(s) this PR fixes: Fixes #2599 #### Special notes for your reviewer: how to test it? 1. 新建一篇文章并创建一些评论（需要安装评论插件 [plugin-comment-widget](https://github.com/halo-sigs/plugin-comment-widget)） 2. 逻辑删除文章时评论不会被删除，真实删除则会 3. 新建一个文章,再删除它，然后查询 snaphost 模型看关联文章的数据有没有被删除 4. 自定义页面同上 5. 删除文章或自定义页面时会删除对应的 Counter 记录（`GET /apis/metrics.halo.run/v1alpha1/counters`） /cc @halo-dev/sig-halo #### Does this PR introduce a user-facing change? ```release-note 修复删除文章或自定义页面时没有级联删除内容快照和评论的问题 ``` --- .../app/config/ExtensionConfiguration.java | 10 +++-- .../extension/reconciler/PostReconciler.java | 38 ++++++++++++++---- .../reconciler/SinglePageReconciler.java | 35 ++++++++++++++++- .../halo/app/extension/ExtensionOperator.java | 5 +++ .../run/halo/app/metrics/CounterService.java | 3 ++ .../halo/app/metrics/CounterServiceImpl.java | 29 +++++++++++--- .../reconciler/SinglePageReconcilerTest.java | 7 +++- .../app/extension/ExtensionOperatorTest.java | 16 ++++++++ .../app/metrics/CounterServiceImplTest.java | 39 ++++++++++++++++++- 9 files changed, 161 insertions(+), 21 deletions(-) diff --git a/src/main/java/run/halo/app/config/ExtensionConfiguration.java b/src/main/java/run/halo/app/config/ExtensionConfiguration.java index b147689e2..ee7a32d43 100644 --- a/src/main/java/run/halo/app/config/ExtensionConfiguration.java +++ b/src/main/java/run/halo/app/config/ExtensionConfiguration.java @@ -55,6 +55,7 @@ import run.halo.app.extension.router.ExtensionCompositeRouterFunction; import run.halo.app.infra.ExternalUrlSupplier; import run.halo.app.infra.SystemConfigurableEnvironmentFetcher; import run.halo.app.infra.properties.HaloProperties; +import run.halo.app.metrics.CounterService; import run.halo.app.plugin.ExtensionComponentsFinder; import run.halo.app.plugin.HaloPluginManager; import run.halo.app.plugin.resources.ReverseProxyRouterFunctionRegistry; @@ -147,9 +148,10 @@ public class ExtensionConfiguration { @Bean Controller postController(ExtensionClient client, ContentService contentService, - PostPermalinkPolicy postPermalinkPolicy) { + PostPermalinkPolicy postPermalinkPolicy, CounterService counterService) { return new ControllerBuilder("post-controller", client) - .reconciler(new PostReconciler(client, contentService, postPermalinkPolicy)) + .reconciler(new PostReconciler(client, contentService, postPermalinkPolicy, + counterService)) .extension(new Post()) .build(); } @@ -195,10 +197,10 @@ public class ExtensionConfiguration { @Bean Controller singlePageController(ExtensionClient client, ContentService contentService, - ApplicationContext applicationContext) { + ApplicationContext applicationContext, CounterService counterService) { return new ControllerBuilder("single-page-controller", client) .reconciler(new SinglePageReconciler(client, contentService, - applicationContext) + applicationContext, counterService) ) .extension(new SinglePage()) .build(); diff --git a/src/main/java/run/halo/app/core/extension/reconciler/PostReconciler.java b/src/main/java/run/halo/app/core/extension/reconciler/PostReconciler.java index 6f0247625..56dad9d26 100644 --- a/src/main/java/run/halo/app/core/extension/reconciler/PostReconciler.java +++ b/src/main/java/run/halo/app/core/extension/reconciler/PostReconciler.java @@ -12,13 +12,18 @@ import org.jsoup.Jsoup; import org.springframework.util.Assert; import run.halo.app.content.ContentService; import run.halo.app.content.permalinks.PostPermalinkPolicy; +import run.halo.app.core.extension.Comment; import run.halo.app.core.extension.Post; import run.halo.app.core.extension.Snapshot; import run.halo.app.extension.ExtensionClient; +import run.halo.app.extension.ExtensionOperator; +import run.halo.app.extension.Ref; import run.halo.app.extension.controller.Reconciler; import run.halo.app.infra.Condition; import run.halo.app.infra.ConditionStatus; import run.halo.app.infra.utils.JsonUtils; +import run.halo.app.metrics.CounterService; +import run.halo.app.metrics.MeterUtils; /** *

Reconciler for {@link Post}.

@@ -37,22 +42,29 @@ public class PostReconciler implements Reconciler { private final ExtensionClient client; private final ContentService contentService; private final PostPermalinkPolicy postPermalinkPolicy; + private final CounterService counterService; public PostReconciler(ExtensionClient client, ContentService contentService, - PostPermalinkPolicy postPermalinkPolicy) { + PostPermalinkPolicy postPermalinkPolicy, CounterService counterService) { this.client = client; this.contentService = contentService; this.postPermalinkPolicy = postPermalinkPolicy; + this.counterService = counterService; } @Override public Result reconcile(Request request) { client.fetch(Post.class, request.name()) .ifPresent(post -> { - if (isDeleted(post)) { + if (ExtensionOperator.isDeleted(post)) { cleanUpResourcesAndRemoveFinalizer(request.name()); return; } + if (Objects.equals(true, post.getSpec().getDeleted())) { + // remove permalink from permalink indexer + postPermalinkPolicy.onPermalinkDelete(post); + return; + } addFinalizerIfNecessary(post); reconcileMetadata(request.name()); @@ -198,6 +210,23 @@ public class PostReconciler implements Reconciler { private void cleanUpResources(Post post) { // remove permalink from permalink indexer postPermalinkPolicy.onPermalinkDelete(post); + + // clean up snapshots + Snapshot.SubjectRef subjectRef = + Snapshot.SubjectRef.of(Post.KIND, post.getMetadata().getName()); + client.list(Snapshot.class, + snapshot -> subjectRef.equals(snapshot.getSpec().getSubjectRef()), null) + .forEach(client::delete); + + // clean up comments + Ref ref = Ref.of(post); + client.list(Comment.class, comment -> comment.getSpec().getSubjectRef().equals(ref), + null) + .forEach(client::delete); + + // delete counter + counterService.deleteByName(MeterUtils.nameOf(Post.class, post.getMetadata().getName())) + .block(); } private Map getLabelsOrDefault(Post post) { @@ -224,9 +253,4 @@ public class PostReconciler implements Reconciler { private boolean isPublished(Post post) { return Objects.equals(true, post.getSpec().getPublished()); } - - private boolean isDeleted(Post post) { - return Objects.equals(true, post.getSpec().getDeleted()) - || post.getMetadata().getDeletionTimestamp() != null; - } } diff --git a/src/main/java/run/halo/app/core/extension/reconciler/SinglePageReconciler.java b/src/main/java/run/halo/app/core/extension/reconciler/SinglePageReconciler.java index 197228748..6e51ca0ac 100644 --- a/src/main/java/run/halo/app/core/extension/reconciler/SinglePageReconciler.java +++ b/src/main/java/run/halo/app/core/extension/reconciler/SinglePageReconciler.java @@ -16,16 +16,21 @@ import org.springframework.context.ApplicationContext; import org.springframework.util.Assert; import run.halo.app.content.ContentService; import run.halo.app.content.permalinks.ExtensionLocator; +import run.halo.app.core.extension.Comment; import run.halo.app.core.extension.Post; import run.halo.app.core.extension.SinglePage; import run.halo.app.core.extension.Snapshot; import run.halo.app.extension.ExtensionClient; +import run.halo.app.extension.ExtensionOperator; import run.halo.app.extension.GroupVersionKind; +import run.halo.app.extension.Ref; import run.halo.app.extension.controller.Reconciler; import run.halo.app.infra.Condition; import run.halo.app.infra.ConditionStatus; import run.halo.app.infra.utils.JsonUtils; import run.halo.app.infra.utils.PathUtils; +import run.halo.app.metrics.CounterService; +import run.halo.app.metrics.MeterUtils; import run.halo.app.theme.router.PermalinkIndexAddCommand; import run.halo.app.theme.router.PermalinkIndexDeleteCommand; @@ -47,12 +52,14 @@ public class SinglePageReconciler implements Reconciler { private final ExtensionClient client; private final ContentService contentService; private final ApplicationContext applicationContext; + private final CounterService counterService; public SinglePageReconciler(ExtensionClient client, ContentService contentService, - ApplicationContext applicationContext) { + ApplicationContext applicationContext, CounterService counterService) { this.client = client; this.contentService = contentService; this.applicationContext = applicationContext; + this.counterService = counterService; } @Override @@ -60,10 +67,16 @@ public class SinglePageReconciler implements Reconciler { client.fetch(SinglePage.class, request.name()) .ifPresent(singlePage -> { SinglePage oldPage = JsonUtils.deepCopy(singlePage); - if (isDeleted(oldPage)) { + if (ExtensionOperator.isDeleted(singlePage)) { cleanUpResourcesAndRemoveFinalizer(request.name()); return; } + + if (Objects.equals(true, singlePage.getSpec().getDeleted())) { + // remove permalink from permalink indexer + permalinkOnDelete(singlePage); + return; + } addFinalizerIfNecessary(oldPage); reconcileStatus(request.name()); @@ -92,6 +105,24 @@ public class SinglePageReconciler implements Reconciler { private void cleanUpResources(SinglePage singlePage) { // remove permalink from permalink indexer permalinkOnDelete(singlePage); + + // clean up snapshot + Snapshot.SubjectRef subjectRef = + Snapshot.SubjectRef.of(SinglePage.KIND, singlePage.getMetadata().getName()); + client.list(Snapshot.class, + snapshot -> subjectRef.equals(snapshot.getSpec().getSubjectRef()), null) + .forEach(client::delete); + + // clean up comments + Ref ref = Ref.of(singlePage); + client.list(Comment.class, comment -> comment.getSpec().getSubjectRef().equals(ref), + null) + .forEach(client::delete); + + // delete counter for single page + counterService.deleteByName( + MeterUtils.nameOf(SinglePage.class, singlePage.getMetadata().getName())) + .block(); } private void cleanUpResourcesAndRemoveFinalizer(String pageName) { diff --git a/src/main/java/run/halo/app/extension/ExtensionOperator.java b/src/main/java/run/halo/app/extension/ExtensionOperator.java index 49005dcc1..75b361f21 100644 --- a/src/main/java/run/halo/app/extension/ExtensionOperator.java +++ b/src/main/java/run/halo/app/extension/ExtensionOperator.java @@ -95,4 +95,9 @@ public interface ExtensionOperator { static Predicate isNotDeleted() { return ext -> ext.getMetadata().getDeletionTimestamp() == null; } + + static boolean isDeleted(ExtensionOperator extension) { + return extension.getMetadata() != null + && extension.getMetadata().getDeletionTimestamp() != null; + } } diff --git a/src/main/java/run/halo/app/metrics/CounterService.java b/src/main/java/run/halo/app/metrics/CounterService.java index f1fb9c12c..3fb70acd9 100644 --- a/src/main/java/run/halo/app/metrics/CounterService.java +++ b/src/main/java/run/halo/app/metrics/CounterService.java @@ -1,5 +1,6 @@ package run.halo.app.metrics; +import reactor.core.publisher.Mono; import run.halo.app.core.extension.Counter; /** @@ -9,4 +10,6 @@ import run.halo.app.core.extension.Counter; public interface CounterService { Counter getByName(String counterName); + + Mono deleteByName(String counterName); } diff --git a/src/main/java/run/halo/app/metrics/CounterServiceImpl.java b/src/main/java/run/halo/app/metrics/CounterServiceImpl.java index d9d44a295..f391bfc8c 100644 --- a/src/main/java/run/halo/app/metrics/CounterServiceImpl.java +++ b/src/main/java/run/halo/app/metrics/CounterServiceImpl.java @@ -4,8 +4,10 @@ import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Tag; import java.util.Collection; import org.springframework.stereotype.Service; +import reactor.core.publisher.Mono; import run.halo.app.core.extension.Counter; import run.halo.app.extension.Metadata; +import run.halo.app.extension.ReactiveExtensionClient; /** * Counter service implementation. @@ -17,24 +19,39 @@ import run.halo.app.extension.Metadata; public class CounterServiceImpl implements CounterService { private final MeterRegistry meterRegistry; + private final ReactiveExtensionClient client; - public CounterServiceImpl(MeterRegistry meterRegistry) { + public CounterServiceImpl(MeterRegistry meterRegistry, ReactiveExtensionClient client) { this.meterRegistry = meterRegistry; + this.client = client; } @Override public Counter getByName(String counterName) { - Tag commonTag = MeterUtils.METRICS_COMMON_TAG; - Collection counters = meterRegistry.find(counterName) - .tag(commonTag.getKey(), - valueMatch -> commonTag.getValue().equals(valueMatch)) - .counters(); + Collection counters = + findCounters(counterName); Counter counter = emptyCounter(counterName); counter.populateFrom(counters); return counter; } + private Collection findCounters(String counterName) { + Tag commonTag = MeterUtils.METRICS_COMMON_TAG; + return meterRegistry.find(counterName) + .tag(commonTag.getKey(), + valueMatch -> commonTag.getValue().equals(valueMatch)) + .counters(); + } + + @Override + public Mono deleteByName(String counterName) { + return client.fetch(Counter.class, counterName) + .flatMap(counter -> client.delete(counter) + .doOnNext(deleted -> findCounters(counterName).forEach(meterRegistry::remove)) + .thenReturn(counter)); + } + private Counter emptyCounter(String name) { Counter counter = new Counter(); counter.setMetadata(new Metadata()); diff --git a/src/test/java/run/halo/app/core/extension/reconciler/SinglePageReconcilerTest.java b/src/test/java/run/halo/app/core/extension/reconciler/SinglePageReconcilerTest.java index a9a3a5949..dee2d22e5 100644 --- a/src/test/java/run/halo/app/core/extension/reconciler/SinglePageReconcilerTest.java +++ b/src/test/java/run/halo/app/core/extension/reconciler/SinglePageReconcilerTest.java @@ -30,6 +30,7 @@ import run.halo.app.core.extension.Snapshot; import run.halo.app.extension.ExtensionClient; import run.halo.app.extension.Metadata; import run.halo.app.extension.controller.Reconciler; +import run.halo.app.metrics.CounterService; import run.halo.app.theme.router.PermalinkIndexAddCommand; import run.halo.app.theme.router.PermalinkIndexDeleteCommand; import run.halo.app.theme.router.PermalinkIndexUpdateCommand; @@ -50,11 +51,15 @@ class SinglePageReconcilerTest { @Mock private ApplicationContext applicationContext; + @Mock + private CounterService counterService; + private SinglePageReconciler singlePageReconciler; @BeforeEach void setUp() { - singlePageReconciler = new SinglePageReconciler(client, contentService, applicationContext); + singlePageReconciler = new SinglePageReconciler(client, contentService, applicationContext, + counterService); } @Test diff --git a/src/test/java/run/halo/app/extension/ExtensionOperatorTest.java b/src/test/java/run/halo/app/extension/ExtensionOperatorTest.java index 08534e89e..bdb20f1a5 100644 --- a/src/test/java/run/halo/app/extension/ExtensionOperatorTest.java +++ b/src/test/java/run/halo/app/extension/ExtensionOperatorTest.java @@ -22,4 +22,20 @@ class ExtensionOperatorTest { when(metadata.getDeletionTimestamp()).thenReturn(Instant.now()); assertFalse(ExtensionOperator.isNotDeleted().test(ext)); } + + @Test + void testIsDeleted() { + var ext = mock(ExtensionOperator.class); + + when(ext.getMetadata()).thenReturn(null); + assertFalse(ExtensionOperator.isDeleted(ext)); + + var metadata = mock(Metadata.class); + when(ext.getMetadata()).thenReturn(metadata); + when(metadata.getDeletionTimestamp()).thenReturn(null); + assertFalse(ExtensionOperator.isDeleted(ext)); + + when(metadata.getDeletionTimestamp()).thenReturn(Instant.now()); + assertTrue(ExtensionOperator.isDeleted(ext)); + } } \ No newline at end of file diff --git a/src/test/java/run/halo/app/metrics/CounterServiceImplTest.java b/src/test/java/run/halo/app/metrics/CounterServiceImplTest.java index e5b12343f..9e3ea80d1 100644 --- a/src/test/java/run/halo/app/metrics/CounterServiceImplTest.java +++ b/src/test/java/run/halo/app/metrics/CounterServiceImplTest.java @@ -1,11 +1,24 @@ package run.halo.app.metrics; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; +import run.halo.app.core.extension.Counter; import run.halo.app.core.extension.Post; +import run.halo.app.extension.Metadata; +import run.halo.app.extension.ReactiveExtensionClient; /** * Tests for {@link CounterServiceImpl}. @@ -13,20 +26,32 @@ import run.halo.app.core.extension.Post; * @author guqing * @since 2.0.0 */ +@ExtendWith(MockitoExtension.class) class CounterServiceImplTest { + @Mock + private ReactiveExtensionClient client; private CounterServiceImpl counterService; @BeforeEach void setUp() { SimpleMeterRegistry simpleMeterRegistry = new SimpleMeterRegistry(); - counterService = new CounterServiceImpl(simpleMeterRegistry); + counterService = new CounterServiceImpl(simpleMeterRegistry, client); String counterName = MeterUtils.nameOf(Post.class, "fake-post"); MeterUtils.visitCounter(simpleMeterRegistry, counterName).increment(); MeterUtils.approvedCommentCounter(simpleMeterRegistry, counterName) .increment(2); + + Counter counter = new Counter(); + counter.setMetadata(new Metadata()); + counter.getMetadata().setName(counterName); + counter.setVisit(2); + + lenient().when(client.delete(any())).thenReturn(Mono.just(counter)); + lenient().when(client.fetch(eq(Counter.class), eq(counterName))) + .thenReturn(Mono.just(counter)); } @Test @@ -38,4 +63,16 @@ class CounterServiceImplTest { assertThat(counter.getTotalComment()).isEqualTo(0); assertThat(counter.getApprovedComment()).isEqualTo(2); } + + @Test + void deleteByName() { + String counterName = MeterUtils.nameOf(Post.class, "fake-post"); + counterService.deleteByName(counterName) + .as(StepVerifier::create) + .consumeNextWith(counter -> { + verify(client, times(1)).delete(any(Counter.class)); + assertThat(counterService.getByName(counterName).getVisit()).isEqualTo(0); + }) + .verifyComplete(); + } } \ No newline at end of file