From ce85b9853958cd9b4531219decd1db1685201238 Mon Sep 17 00:00:00 2001 From: John Niang Date: Thu, 23 Feb 2023 18:24:12 +0800 Subject: [PATCH] Discard watch when extension status is changed only (#3362) #### What type of PR is this? /kind improvement /area core #### What this PR does / why we need it: This PR mainly creates two things: 1. Create JsonExtension to represent any extension and make extension modification more convenient 2. Discard watch when we detect that the extension status is changed only. It's useful to prevent infinite loop of reconciler. #### Which issue(s) this PR fixes: Fixes https://github.com/halo-dev/halo/issues/3273 #### Special notes for your reviewer: Try to install test plugin from [plugin-comment-widget.zip](https://github.com/halo-dev/halo/files/10799875/plugin-comment-widget.zip) before checking out this PR. You will get a infinite reconciliation loop. Then, stop the process and checkout this PR and see the result. #### Does this PR introduce a user-facing change? ```release-note None ``` --- .../app/extension/JSONExtensionConverter.java | 3 +- .../run/halo/app/extension/JsonExtension.java | 232 ++++++++++++++++++ .../ReactiveExtensionClientImpl.java | 62 +++-- .../halo/app/extension/JsonExtensionTest.java | 80 ++++++ .../ReactiveExtensionClientTest.java | 14 +- 5 files changed, 367 insertions(+), 24 deletions(-) create mode 100644 src/main/java/run/halo/app/extension/JsonExtension.java create mode 100644 src/test/java/run/halo/app/extension/JsonExtensionTest.java diff --git a/src/main/java/run/halo/app/extension/JSONExtensionConverter.java b/src/main/java/run/halo/app/extension/JSONExtensionConverter.java index a77b3c4dc..0cfa1be10 100644 --- a/src/main/java/run/halo/app/extension/JSONExtensionConverter.java +++ b/src/main/java/run/halo/app/extension/JSONExtensionConverter.java @@ -53,8 +53,6 @@ public class JSONExtensionConverter implements ExtensionConverter { var scheme = schemeManager.get(gvk); try { - var data = objectMapper.writeValueAsBytes(extension); - var validation = new ValidationData<>(extension); var extensionJsonNode = objectMapper.valueToTree(extension); var validator = getValidator(scheme); @@ -68,6 +66,7 @@ public class JSONExtensionConverter implements ExtensionConverter { var version = extension.getMetadata().getVersion(); var storeName = ExtensionUtil.buildStoreName(scheme, extension.getMetadata().getName()); + var data = objectMapper.writeValueAsBytes(extensionJsonNode); return new ExtensionStore(storeName, data, version); } catch (IOException e) { throw new ExtensionConvertException("Failed write Extension as bytes", e); diff --git a/src/main/java/run/halo/app/extension/JsonExtension.java b/src/main/java/run/halo/app/extension/JsonExtension.java new file mode 100644 index 000000000..67ef64fc8 --- /dev/null +++ b/src/main/java/run/halo/app/extension/JsonExtension.java @@ -0,0 +1,232 @@ +package run.halo.app.extension; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.JsonSerializer; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializerProvider; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import com.fasterxml.jackson.databind.node.LongNode; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.fasterxml.jackson.databind.node.TextNode; +import java.io.IOException; +import java.time.Instant; +import java.util.Map; +import java.util.Set; + +/** + * JsonExtension is representation an extension using ObjectNode. This extension is preparing for + * patching in the future. + * + * @author johnniang + */ +@JsonSerialize(using = JsonExtension.ObjectNodeExtensionSerializer.class) +@JsonDeserialize(using = JsonExtension.ObjectNodeExtensionDeSerializer.class) +class JsonExtension implements Extension { + + private final ObjectMapper objectMapper; + + private final ObjectNode objectNode; + + public JsonExtension(ObjectMapper objectMapper) { + this(objectMapper, objectMapper.createObjectNode()); + } + + public JsonExtension(ObjectMapper objectMapper, ObjectNode objectNode) { + this.objectMapper = objectMapper; + this.objectNode = objectNode; + } + + public JsonExtension(ObjectMapper objectMapper, Extension e) { + this(objectMapper, (ObjectNode) objectMapper.valueToTree(e)); + } + + @Override + public MetadataOperator getMetadata() { + var metadataNode = objectNode.get("metadata"); + if (metadataNode == null) { + return null; + } + return new ObjectNodeMetadata((ObjectNode) metadataNode); + } + + @Override + public String getApiVersion() { + var apiVersionNode = objectNode.get("apiVersion"); + return apiVersionNode == null ? null : apiVersionNode.asText(); + } + + @Override + public String getKind() { + return objectNode.get("kind").asText(); + } + + @Override + public void setApiVersion(String apiVersion) { + objectNode.set("apiVersion", new TextNode(apiVersion)); + } + + @Override + public void setKind(String kind) { + objectNode.set("kind", new TextNode(kind)); + } + + @Override + public void setMetadata(MetadataOperator metadata) { + objectNode.set("metadata", objectMapper.valueToTree(metadata)); + } + + public static class ObjectNodeExtensionSerializer extends JsonSerializer { + + @Override + public void serialize(JsonExtension value, JsonGenerator gen, + SerializerProvider serializers) throws IOException { + gen.writeTree(value.objectNode); + } + } + + public static class ObjectNodeExtensionDeSerializer + extends JsonDeserializer { + + @Override + public JsonExtension deserialize(JsonParser p, DeserializationContext ctxt) + throws IOException { + var mapper = (ObjectMapper) p.getCodec(); + var treeNode = mapper.readTree(p); + return new JsonExtension(mapper, (ObjectNode) treeNode); + } + } + + /** + * Get internal representation. + * + * @return internal representation + */ + public ObjectNode getInternal() { + return objectNode; + } + + public MetadataOperator getMetadataOrCreate() { + var metadataNode = objectMapper.createObjectNode(); + objectNode.set("metadata", metadataNode); + return new ObjectNodeMetadata(metadataNode); + } + + class ObjectNodeMetadata implements MetadataOperator { + + private final ObjectNode objectNode; + + public ObjectNodeMetadata(ObjectNode objectNode) { + this.objectNode = objectNode; + } + + @Override + public String getName() { + var nameNode = objectNode.get("name"); + return objectMapper.convertValue(nameNode, String.class); + } + + @Override + public String getGenerateName() { + var generateNameNode = objectNode.get("generateName"); + return objectMapper.convertValue(generateNameNode, String.class); + } + + @Override + public Map getLabels() { + var labelsNode = objectNode.get("labels"); + return objectMapper.convertValue(labelsNode, new TypeReference<>() { + }); + } + + @Override + public Map getAnnotations() { + var annotationsNode = objectNode.get("annotations"); + return objectMapper.convertValue(annotationsNode, new TypeReference<>() { + }); + } + + @Override + public Long getVersion() { + JsonNode versionNode = objectNode.get("version"); + return objectMapper.convertValue(versionNode, Long.class); + } + + @Override + public Instant getCreationTimestamp() { + return objectMapper.convertValue(objectNode.get("creationTimestamp"), Instant.class); + } + + @Override + public Instant getDeletionTimestamp() { + return objectMapper.convertValue(objectNode.get("deletionTimestamp"), Instant.class); + } + + @Override + public Set getFinalizers() { + return objectMapper.convertValue(objectNode.get("finalizers"), new TypeReference<>() { + }); + } + + @Override + public void setName(String name) { + if (name != null) { + objectNode.set("name", TextNode.valueOf(name)); + } + } + + @Override + public void setGenerateName(String generateName) { + if (generateName != null) { + objectNode.set("generateName", TextNode.valueOf(generateName)); + } + } + + @Override + public void setLabels(Map labels) { + if (labels != null) { + objectNode.set("labels", objectMapper.valueToTree(labels)); + } + } + + @Override + public void setAnnotations(Map annotations) { + if (annotations != null) { + objectNode.set("annotations", objectMapper.valueToTree(annotations)); + } + } + + @Override + public void setVersion(Long version) { + if (version != null) { + objectNode.set("version", LongNode.valueOf(version)); + } + } + + @Override + public void setCreationTimestamp(Instant creationTimestamp) { + if (creationTimestamp != null) { + objectNode.set("creationTimestamp", objectMapper.valueToTree(creationTimestamp)); + } + } + + @Override + public void setDeletionTimestamp(Instant deletionTimestamp) { + if (deletionTimestamp != null) { + objectNode.set("deletionTimestamp", objectMapper.valueToTree(deletionTimestamp)); + } + } + + @Override + public void setFinalizers(Set finalizers) { + if (finalizers != null) { + objectNode.set("finalizers", objectMapper.valueToTree(finalizers)); + } + } + } +} diff --git a/src/main/java/run/halo/app/extension/ReactiveExtensionClientImpl.java b/src/main/java/run/halo/app/extension/ReactiveExtensionClientImpl.java index c07abd880..7466e958b 100644 --- a/src/main/java/run/halo/app/extension/ReactiveExtensionClientImpl.java +++ b/src/main/java/run/halo/app/extension/ReactiveExtensionClientImpl.java @@ -3,11 +3,13 @@ package run.halo.app.extension; import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic; import static org.springframework.util.StringUtils.hasText; +import com.fasterxml.jackson.databind.ObjectMapper; import java.time.Duration; import java.time.Instant; import java.util.Comparator; import java.util.List; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.data.util.Predicates; @@ -29,11 +31,14 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient { private final Watcher.WatcherComposite watchers; + private final ObjectMapper objectMapper; + public ReactiveExtensionClientImpl(ReactiveExtensionStoreClient client, - ExtensionConverter converter, SchemeManager schemeManager) { + ExtensionConverter converter, SchemeManager schemeManager, ObjectMapper objectMapper) { this.client = client; this.converter = converter; this.schemeManager = schemeManager; + this.objectMapper = objectMapper; this.watchers = new Watcher.WatcherComposite(); } @@ -110,7 +115,7 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient { if (!hasText(metadata.getGenerateName())) { throw new IllegalArgumentException( "The metadata.generateName must not be blank when metadata.name is " - + "blank"); + + "blank"); } // generate name with random text metadata.setName(metadata.getGenerateName() + randomAlphabetic(5)); @@ -124,40 +129,57 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient { .retryWhen(Retry.backoff(3, Duration.ofMillis(100)) // retry when generateName is set .filter(t -> t instanceof DataIntegrityViolationException - && hasText(extension.getMetadata().getGenerateName()))); + && hasText(extension.getMetadata().getGenerateName()))); } @Override public Mono update(E extension) { - // overwrite some fields - Mono mono; - if (extension instanceof Unstructured unstructured) { - mono = get(unstructured.groupVersionKind(), extension.getMetadata().getName()); - } else { - mono = get(extension.getClass(), extension.getMetadata().getName()); - } - return mono - .flatMap(old -> { - // reset some fields - var oldMetadata = old.getMetadata(); - var newMetadata = extension.getMetadata(); + // Refactor the atomic reference if we have a better solution. + final var statusChangeOnly = new AtomicBoolean(false); + return getLatest(extension) + .map(old -> new JsonExtension(objectMapper, old)) + .flatMap(oldJsonExt -> { + var newJsonExt = new JsonExtension(objectMapper, extension); + // reset some mandatory fields + var oldMetadata = oldJsonExt.getMetadata(); + var newMetadata = newJsonExt.getMetadata(); newMetadata.setCreationTimestamp(oldMetadata.getCreationTimestamp()); - newMetadata.setDeletionTimestamp(oldMetadata.getDeletionTimestamp()); - extension.setMetadata(newMetadata); - if (Objects.equals(old, extension)) { + newMetadata.setGenerateName(oldMetadata.getGenerateName()); + + var oldObjectNode = oldJsonExt.getInternal().deepCopy(); + var newObjectNode = newJsonExt.getInternal().deepCopy(); + if (Objects.equals(oldObjectNode, newObjectNode)) { + // if no data were changed, just skip updating. return Mono.empty(); } - return Mono.just(extension); + // check status is changed + oldObjectNode.remove("status"); + newObjectNode.remove("status"); + if (Objects.equals(oldObjectNode, newObjectNode)) { + statusChangeOnly.set(true); + } + return Mono.just(newJsonExt); }) .map(converter::convertTo) .flatMap(extensionStore -> client.update(extensionStore.getName(), extensionStore.getVersion(), extensionStore.getData())) .map(updated -> converter.convertFrom((Class) extension.getClass(), updated)) - .doOnNext(updated -> watchers.onUpdate(extension, updated)) + .doOnNext(updated -> { + if (!statusChangeOnly.get()) { + watchers.onUpdate(extension, updated); + } + }) .switchIfEmpty(Mono.defer(() -> Mono.just(extension))); } + private Mono getLatest(Extension extension) { + if (extension instanceof Unstructured unstructured) { + return get(unstructured.groupVersionKind(), unstructured.getMetadata().getName()); + } + return get(extension.getClass(), extension.getMetadata().getName()); + } + @Override public Mono delete(E extension) { // set deletionTimestamp diff --git a/src/test/java/run/halo/app/extension/JsonExtensionTest.java b/src/test/java/run/halo/app/extension/JsonExtensionTest.java new file mode 100644 index 000000000..62c5596ac --- /dev/null +++ b/src/test/java/run/halo/app/extension/JsonExtensionTest.java @@ -0,0 +1,80 @@ +package run.halo.app.extension; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.node.TextNode; +import org.json.JSONException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.skyscreamer.jsonassert.JSONAssert; + +class JsonExtensionTest { + + ObjectMapper objectMapper; + + @BeforeEach + void setUp() { + objectMapper = JsonMapper.builder().build(); + } + + @Test + void serializeEmptyExt() throws JsonProcessingException, JSONException { + var ext = new JsonExtension(objectMapper); + var json = objectMapper.writeValueAsString(ext); + JSONAssert.assertEquals("{}", json, true); + } + + @Test + void serializeExt() throws JsonProcessingException, JSONException { + var ext = new JsonExtension(objectMapper); + ext.setApiVersion("fake.halo.run/v1alpha"); + ext.setKind("Fake"); + var metadata = ext.getMetadataOrCreate(); + metadata.setName("fake-name"); + + ext.getInternal().set("data", TextNode.valueOf("halo")); + + JSONAssert.assertEquals(""" + { + "apiVersion": "fake.halo.run/v1alpha", + "kind": "Fake", + "metadata": { + "name": "fake-name" + }, + "data": "halo" + }""", objectMapper.writeValueAsString(ext), true); + } + + @Test + void deserialize() throws JsonProcessingException { + var json = """ + { + "apiVersion": "fake.halo.run/v1alpha1", + "kind": "Fake", + "metadata": { + "name": "faker" + }, + "otherProperty": "otherPropertyValue" + }"""; + + var ext = objectMapper.readValue(json, JsonExtension.class); + + assertEquals("fake.halo.run/v1alpha1", ext.getApiVersion()); + assertEquals("Fake", ext.getKind()); + assertNotNull(ext.getMetadata()); + assertEquals("faker", ext.getMetadata().getName()); + assertNull(ext.getMetadata().getVersion()); + assertNull(ext.getMetadata().getFinalizers()); + assertNull(ext.getMetadata().getAnnotations()); + assertNull(ext.getMetadata().getLabels()); + assertNull(ext.getMetadata().getGenerateName()); + assertNull(ext.getMetadata().getCreationTimestamp()); + assertNull(ext.getMetadata().getDeletionTimestamp()); + assertEquals("otherPropertyValue", ext.getInternal().get("otherProperty").asText()); + } +} \ No newline at end of file diff --git a/src/test/java/run/halo/app/extension/ReactiveExtensionClientTest.java b/src/test/java/run/halo/app/extension/ReactiveExtensionClientTest.java index abf01c8d1..7e60e74f6 100644 --- a/src/test/java/run/halo/app/extension/ReactiveExtensionClientTest.java +++ b/src/test/java/run/halo/app/extension/ReactiveExtensionClientTest.java @@ -20,6 +20,9 @@ import static org.mockito.Mockito.when; import static run.halo.app.extension.GroupVersionKind.fromAPIVersionAndKind; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; import java.util.List; import java.util.Map; import org.junit.jupiter.api.BeforeEach; @@ -29,6 +32,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.dao.DataIntegrityViolationException; import reactor.core.Exceptions; @@ -53,6 +57,11 @@ class ReactiveExtensionClientTest { @Mock SchemeManager schemeManager; + @Spy + ObjectMapper objectMapper = JsonMapper.builder() + .addModule(new JavaTimeModule()) + .build(); + @InjectMocks ReactiveExtensionClientImpl client; @@ -419,7 +428,7 @@ class ReactiveExtensionClientTest { .verifyComplete(); verify(storeClient).fetchByName(storeName); - verify(converter).convertTo(eq(fake)); + verify(converter).convertTo(isA(JsonExtension.class)); verify(converter, times(2)).convertFrom(same(FakeExtension.class), any()); verify(storeClient) .update(eq("/registry/fake.halo.run/fakes/fake"), eq(2L), any()); @@ -433,6 +442,7 @@ class ReactiveExtensionClientTest { Mono.just(createExtensionStore(storeName, 1L))); var oldFake = createFakeExtension("fake", 2L); + when(converter.convertFrom(same(FakeExtension.class), any())).thenReturn(oldFake); StepVerifier.create(client.update(fake)) @@ -470,7 +480,7 @@ class ReactiveExtensionClientTest { .verifyComplete(); verify(storeClient).fetchByName(name); - verify(converter).convertTo(eq(fake)); + verify(converter).convertTo(isA(JsonExtension.class)); verify(converter, times(2)).convertFrom(same(Unstructured.class), any()); verify(storeClient) .update(eq("/registry/fake.halo.run/fakes/fake"), eq(12345L), any());