refactor: merge patch default values to the existing config for theme and plugin setting (#3616)

#### 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
修复升级主题或插件时新增加的配置默认值没有更新的问题
```
pull/3640/head
guqing 2023-03-30 16:34:14 +08:00 committed by GitHub
parent a77756abad
commit 31e5014dec
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 195 additions and 39 deletions

View File

@ -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<Request> {
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;
}

View File

@ -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<Request> {
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<Request> {
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) {

View File

@ -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<String, String> modified = defaultIfNull(configMap.getData(), Map.of());
final var oldData = JsonUtils.deepCopy(modified);
Map<String, String> 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<String, String> mergePatch(Map<String, String> modified,
Map<String, String> 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<String, String> 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<String, String> jsonNodeToStringMap(JsonNode node) {
Map<String, String> 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;
}
}
}

View File

@ -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<Theme> 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<ConfigMap> configMapCaptor = ArgumentCaptor.forClass(ConfigMap.class);
verify(extensionClient, times(1)).create(any(ConfigMap.class));

View File

@ -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<String, String> defaultValue =
Map.of("comment", "{\"enable\":true,\"requireReviewForNew\":true}",
"basic", "{\"title\":\"guqing's blog\"}",
"authProvider", "{\"github\":{\"clientId\":\"fake-client-id\"}}");
Map<String, String> modified = Map.of("comment",
"{\"enable\":true,\"requireReviewForNew\":true,\"systemUserOnly\":false}",
"basic", "{\"title\":\"guqing's blog\", \"subtitle\": \"fake-sub-title\"}");
Map<String, String> result = SettingUtils.mergePatch(modified, defaultValue);
Map<String, String> 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<String, String> defaultValue = Map.of(
"array", "[1,2,3]",
"number", "1",
"boolean", "false",
"string", "new-default-string-value",
"object", "{\"name\":\"guqing\"}"
);
Map<String, String> modified = Map.of(
"stringArray", "[\"hello\", \"world\"]",
"boolean", "true",
"string", "hello",
"object", "{\"name\":\"guqing\", \"age\": 18}"
);
Map<String, String> result = SettingUtils.mergePatch(modified, defaultValue);
Map<String, String> 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 = """
{