From 31e5014decf3933443f60e7b133e542ee7c15724 Mon Sep 17 00:00:00 2001 From: guqing <38999863+guqing@users.noreply.github.com> Date: Thu, 30 Mar 2023 16:34:14 +0800 Subject: [PATCH] refactor: merge patch default values to the existing config for theme and plugin setting (#3616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### What type of PR is this? /kind improvement /area core /milestone 2.4.x #### What this PR does / why we need it: 修复升级主题或插件时新增加的配置默认值没有更新的问题 how to test it? 1. 安装包含设置的主题后获取主题的 ConfigMap 并记下 2. 修改（增加/更新）主题的设置（Setting）的默认值，模拟更新主题 3. 期望默认值改变不会影响到之前已有的配置，对于新增加的配置的默认值会被合并到已有的 ConfigMap 中 #### Which issue(s) this PR fixes: Fixes #3446 #### Does this PR introduce a user-facing change? ```release-note 修复升级主题或插件时新增加的配置默认值没有更新的问题 ``` --- .../reconciler/PluginReconciler.java | 15 +-- .../extension/reconciler/ThemeReconciler.java | 26 +--- .../core/extension/theme/SettingUtils.java | 118 ++++++++++++++++++ .../reconciler/ThemeReconcilerTest.java | 5 +- .../extension/theme/SettingUtilsTest.java | 70 +++++++++++ 5 files changed, 195 insertions(+), 39 deletions(-) diff --git a/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java b/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java index c8cbaad46..acc253bf9 100644 --- a/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java +++ b/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java @@ -38,7 +38,6 @@ import run.halo.app.core.extension.Plugin; import run.halo.app.core.extension.ReverseProxy; import run.halo.app.core.extension.Setting; import run.halo.app.core.extension.theme.SettingUtils; -import run.halo.app.extension.ConfigMap; import run.halo.app.extension.ExtensionClient; import run.halo.app.extension.GroupVersionKind; import run.halo.app.extension.Metadata; @@ -223,19 +222,7 @@ public class PluginReconciler implements Reconciler { return false; } - boolean existConfigMap = client.fetch(ConfigMap.class, configMapNameToUse) - .isPresent(); - if (existConfigMap) { - return false; - } - - var data = SettingUtils.settingDefinedDefaultValueMap(settingOption.get()); - // Create with or without default value - ConfigMap configMap = new ConfigMap(); - configMap.setMetadata(new Metadata()); - configMap.getMetadata().setName(configMapNameToUse); - configMap.setData(data); - client.create(configMap); + SettingUtils.createOrUpdateConfigMap(client, settingName, configMapNameToUse); return false; } diff --git a/application/src/main/java/run/halo/app/core/extension/reconciler/ThemeReconciler.java b/application/src/main/java/run/halo/app/core/extension/reconciler/ThemeReconciler.java index 82e2b8987..038e8c237 100644 --- a/application/src/main/java/run/halo/app/core/extension/reconciler/ThemeReconciler.java +++ b/application/src/main/java/run/halo/app/core/extension/reconciler/ThemeReconciler.java @@ -1,5 +1,7 @@ package run.halo.app.core.extension.reconciler; +import static org.apache.commons.lang3.ObjectUtils.defaultIfNull; + import java.io.IOException; import java.nio.file.Path; import java.time.Instant; @@ -9,7 +11,6 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.UUID; -import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.springframework.retry.support.RetryTemplate; import org.springframework.stereotype.Component; @@ -18,9 +19,7 @@ import run.halo.app.core.extension.AnnotationSetting; import run.halo.app.core.extension.Setting; import run.halo.app.core.extension.Theme; import run.halo.app.core.extension.theme.SettingUtils; -import run.halo.app.extension.ConfigMap; import run.halo.app.extension.ExtensionClient; -import run.halo.app.extension.Metadata; import run.halo.app.extension.MetadataUtil; import run.halo.app.extension.controller.Controller; import run.halo.app.extension.controller.ControllerBuilder; @@ -87,7 +86,7 @@ public class ThemeReconciler implements Reconciler { void reconcileStatus(String name) { client.fetch(Theme.class, name).ifPresent(theme -> { final Theme.ThemeStatus status = - ObjectUtils.defaultIfNull(theme.getStatus(), new Theme.ThemeStatus()); + defaultIfNull(theme.getStatus(), new Theme.ThemeStatus()); final Theme.ThemeStatus oldStatus = JsonUtils.deepCopy(status); theme.setStatus(status); @@ -143,23 +142,8 @@ public class ThemeReconciler implements Reconciler { final String configMapNameToUse = StringUtils.defaultIfBlank(userDefinedConfigMapName, newConfigMapName); - - boolean existConfigMap = client.fetch(ConfigMap.class, configMapNameToUse) - .isPresent(); - if (existConfigMap) { - return; - } - - client.fetch(Setting.class, theme.getSpec().getSettingName()) - .ifPresent(setting -> { - var data = SettingUtils.settingDefinedDefaultValueMap(setting); - // Whether there is a default value or not - ConfigMap configMap = new ConfigMap(); - configMap.setMetadata(new Metadata()); - configMap.getMetadata().setName(configMapNameToUse); - configMap.setData(data); - client.create(configMap); - }); + SettingUtils.createOrUpdateConfigMap(client, theme.getSpec().getSettingName(), + configMapNameToUse); } private void addFinalizerIfNecessary(Theme oldTheme) { diff --git a/application/src/main/java/run/halo/app/core/extension/theme/SettingUtils.java b/application/src/main/java/run/halo/app/core/extension/theme/SettingUtils.java index b89ab062c..90c4322c4 100644 --- a/application/src/main/java/run/halo/app/core/extension/theme/SettingUtils.java +++ b/application/src/main/java/run/halo/app/core/extension/theme/SettingUtils.java @@ -1,14 +1,27 @@ package run.halo.app.core.extension.theme; +import static org.apache.commons.lang3.ObjectUtils.defaultIfNull; + +import com.fasterxml.jackson.core.JacksonException; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.github.fge.jsonpatch.JsonPatchException; +import com.github.fge.jsonpatch.mergepatch.JsonMergePatch; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; import lombok.experimental.UtilityClass; import org.springframework.lang.NonNull; +import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import run.halo.app.core.extension.Setting; +import run.halo.app.extension.ConfigMap; +import run.halo.app.extension.ExtensionClient; +import run.halo.app.extension.Metadata; +import run.halo.app.infra.utils.JsonParseException; import run.halo.app.infra.utils.JsonUtils; @UtilityClass @@ -45,4 +58,109 @@ public class SettingUtils { } return data; } + + /** + * Create or update config map by provided setting name and configMapName. + * + * @param client extension client + * @param settingName a name for {@link Setting} + * @param configMapName a name for {@link ConfigMap} + */ + public static void createOrUpdateConfigMap(ExtensionClient client, String settingName, + String configMapName) { + Assert.notNull(client, "Extension client must not be null"); + Assert.hasText(settingName, "Setting name must not be blank"); + Assert.hasText(configMapName, "Config map name must not be blank"); + + client.fetch(Setting.class, settingName) + .ifPresent(setting -> { + final var source = SettingUtils.settingDefinedDefaultValueMap(setting); + client.fetch(ConfigMap.class, configMapName) + .ifPresentOrElse(configMap -> { + Map modified = defaultIfNull(configMap.getData(), Map.of()); + final var oldData = JsonUtils.deepCopy(modified); + + Map merged = SettingUtils.mergePatch(modified, source); + configMap.setData(merged); + + if (!Objects.equals(oldData, configMap.getData())) { + client.update(configMap); + } + }, () -> { + ConfigMap configMap = new ConfigMap(); + configMap.setMetadata(new Metadata()); + configMap.getMetadata().setName(configMapName); + configMap.setData(source); + client.create(configMap); + }); + }); + } + + /** + * Construct a JsonMergePatch from a difference between two Maps and apply patch to + * {@code source}. + * + * @param modified the modified object + * @param source the source object + * @return patched map object + */ + public static Map mergePatch(Map modified, + Map source) { + JsonNode modifiedJson = mapToJsonNode(modified); + // original + JsonNode sourceJson = mapToJsonNode(source); + try { + // patch + JsonMergePatch jsonMergePatch = JsonMergePatch.fromJson(modifiedJson); + // apply patch to original + JsonNode patchedNode = jsonMergePatch.apply(sourceJson); + return jsonNodeToStringMap(patchedNode); + } catch (JsonPatchException e) { + throw new JsonParseException(e); + } + } + + JsonNode mapToJsonNode(Map map) { + ObjectNode objectNode = JsonNodeFactory.instance.objectNode(); + map.forEach((k, v) -> { + if (isJson(v)) { + JsonNode value = JsonUtils.jsonToObject(v, JsonNode.class); + objectNode.set(k, value); + return; + } + objectNode.put(k, v); + }); + return objectNode; + } + + Map jsonNodeToStringMap(JsonNode node) { + Map stringMap = new LinkedHashMap<>(); + node.fields().forEachRemaining(entry -> { + String k = entry.getKey(); + JsonNode v = entry.getValue(); + if (v == null || v.isNull() || v.isMissingNode()) { + stringMap.put(k, null); + return; + } + if (v.isTextual()) { + stringMap.put(k, v.asText()); + return; + } + if (v.isContainerNode()) { + stringMap.put(k, JsonUtils.objectToJson(v)); + return; + } + stringMap.put(k, v.asText()); + }); + return stringMap; + } + + boolean isJson(String jsonString) { + try { + JsonUtils.DEFAULT_JSON_MAPPER.readTree(jsonString); + return true; + } catch (JacksonException e) { + return false; + } + } } diff --git a/application/src/test/java/run/halo/app/core/extension/reconciler/ThemeReconcilerTest.java b/application/src/test/java/run/halo/app/core/extension/reconciler/ThemeReconcilerTest.java index 54ffd0758..8b2b8e9d6 100644 --- a/application/src/test/java/run/halo/app/core/extension/reconciler/ThemeReconcilerTest.java +++ b/application/src/test/java/run/halo/app/core/extension/reconciler/ThemeReconcilerTest.java @@ -25,7 +25,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.stubbing.Answer; import org.skyscreamer.jsonassert.JSONAssert; @@ -271,8 +270,6 @@ class ThemeReconcilerTest { // setting exists themeSpec.setSettingName("theme-test-setting"); - when(extensionClient.fetch(eq(ConfigMap.class), any())) - .thenReturn(Optional.of(Mockito.mock(ConfigMap.class))); assertThat(theme.getSpec().getConfigMapName()).isNull(); ArgumentCaptor captor = ArgumentCaptor.forClass(Theme.class); themeReconciler.reconcile(new Reconciler.Request(metadata.getName())); @@ -291,7 +288,7 @@ class ThemeReconcilerTest { when(extensionClient.fetch(eq(Setting.class), eq(themeSpec.getSettingName()))) .thenReturn(Optional.of(getFakeSetting())); themeReconciler.reconcile(new Reconciler.Request(metadata.getName())); - verify(extensionClient, times(1)) + verify(extensionClient, times(2)) .fetch(eq(Setting.class), eq(themeSpec.getSettingName())); ArgumentCaptor configMapCaptor = ArgumentCaptor.forClass(ConfigMap.class); verify(extensionClient, times(1)).create(any(ConfigMap.class)); diff --git a/application/src/test/java/run/halo/app/core/extension/theme/SettingUtilsTest.java b/application/src/test/java/run/halo/app/core/extension/theme/SettingUtilsTest.java index 952a8e62e..bbed17119 100644 --- a/application/src/test/java/run/halo/app/core/extension/theme/SettingUtilsTest.java +++ b/application/src/test/java/run/halo/app/core/extension/theme/SettingUtilsTest.java @@ -1,5 +1,8 @@ package run.halo.app.core.extension.theme; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Map; import org.json.JSONException; import org.junit.jupiter.api.Test; import org.skyscreamer.jsonassert.JSONAssert; @@ -27,6 +30,73 @@ class SettingUtilsTest { true); } + @Test + void mergePatch() throws JSONException { + Map defaultValue = + Map.of("comment", "{\"enable\":true,\"requireReviewForNew\":true}", + "basic", "{\"title\":\"guqing's blog\"}", + "authProvider", "{\"github\":{\"clientId\":\"fake-client-id\"}}"); + Map modified = Map.of("comment", + "{\"enable\":true,\"requireReviewForNew\":true,\"systemUserOnly\":false}", + "basic", "{\"title\":\"guqing's blog\", \"subtitle\": \"fake-sub-title\"}"); + + Map result = SettingUtils.mergePatch(modified, defaultValue); + Map excepted = Map.of("comment", + "{\"enable\":true,\"requireReviewForNew\":true,\"systemUserOnly\":false}", + "basic", "{\"title\":\"guqing's blog\",\"subtitle\":\"fake-sub-title\"}", + "authProvider", "{\"github\":{\"clientId\":\"fake-client-id\"}}"); + JSONAssert.assertEquals(JsonUtils.objectToJson(excepted), JsonUtils.objectToJson(result), + true); + } + + @Test + void mergePatchWithMoreType() throws JSONException { + Map defaultValue = Map.of( + "array", "[1,2,3]", + "number", "1", + "boolean", "false", + "string", "new-default-string-value", + "object", "{\"name\":\"guqing\"}" + ); + Map modified = Map.of( + "stringArray", "[\"hello\", \"world\"]", + "boolean", "true", + "string", "hello", + "object", "{\"name\":\"guqing\", \"age\": 18}" + ); + Map result = SettingUtils.mergePatch(modified, defaultValue); + Map excepted = Map.of( + "array", "[1,2,3]", + "number", "1", + "boolean", "true", + "string", "hello", + "object", "{\"name\":\"guqing\",\"age\":18}", + "stringArray", "[\"hello\",\"world\"]" + ); + JSONAssert.assertEquals(JsonUtils.objectToJson(excepted), JsonUtils.objectToJson(result), + true); + } + + @Test + void isJson() { + assertThat(SettingUtils.isJson("[1,2,3]")).isTrue(); + assertThat(SettingUtils.isJson("[\"hello\"]")).isTrue(); + assertThat(SettingUtils.isJson("{\"name\":\"guqing\",\"age\":18}")).isTrue(); + assertThat(SettingUtils.isJson("{ \"flag\":true }")).isTrue(); + assertThat(SettingUtils.isJson(""" + [ + { "K1": "value-1", "K2":"value1-2" } + ] + """)).isTrue(); + assertThat(SettingUtils.isJson(""" + { + "sites": [{ "name":"halo" , "url":"halo.run" }] + } + """)).isTrue(); + assertThat(SettingUtils.isJson("{\"name\":\"guqing\"")).isFalse(); + assertThat(SettingUtils.isJson("hello")).isFalse(); + } + private static Setting getFakeSetting() { String settingJson = """ {