From e1017a4c7e4331f591c5f89dff1215556e83f046 Mon Sep 17 00:00:00 2001 From: Paint_Ninja Date: Mon, 3 Oct 2022 19:50:32 +0100 Subject: [PATCH] Fix logically breaking change to ForgeConfigSpec.Builder#comment where modders could not add a empty line to the start of comments. (#9061) --- .../common/ForgeConfigSpec.java | 62 +++++++++++-------- .../living/LivingChangeTargetEvent.java | 5 ++ .../commontest/ForgeConfigSpecTest.java | 16 +++++ .../debug/block/OnTreeGrowBlockTest.java | 5 ++ .../LivingSetAttackTargetEventTest.java | 5 ++ .../debug/world/BlockGrowFeatureTest.java | 5 ++ 6 files changed, 73 insertions(+), 25 deletions(-) diff --git a/src/main/java/net/minecraftforge/common/ForgeConfigSpec.java b/src/main/java/net/minecraftforge/common/ForgeConfigSpec.java index cd042e2657..05ae2a25f0 100644 --- a/src/main/java/net/minecraftforge/common/ForgeConfigSpec.java +++ b/src/main/java/net/minecraftforge/common/ForgeConfigSpec.java @@ -44,6 +44,7 @@ import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.Lists; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; /* @@ -60,6 +61,8 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper, String> levelComments, Map, String> levelTranslationKeys) { super(storage); this.values = values; @@ -79,12 +82,12 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper - LogManager.getLogger().warn(CORE, "Incorrect key {} was corrected from {} to its default, {}. {}", DOT_JOINER.join( path ), incorrectValue, correctedValue, incorrectValue == correctedValue ? "This seems to be an error." : ""), + LOGGER.warn(CORE, "Incorrect key {} was corrected from {} to its default, {}. {}", DOT_JOINER.join( path ), incorrectValue, correctedValue, incorrectValue == correctedValue ? "This seems to be an error." : ""), (action, path, incorrectValue, correctedValue) -> - LogManager.getLogger().debug(CORE, "The comment on key {} does not match the spec. This may create a backup.", DOT_JOINER.join( path ))); + LOGGER.debug(CORE, "The comment on key {} does not match the spec. This may create a backup.", DOT_JOINER.join( path ))); if (config instanceof FileConfig) { ((FileConfig) config).save(); @@ -263,12 +266,9 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper 0 && string2.length() > 0) + if (string1.length() > 0 && string2.length() > 0) { return string1.replaceAll("\r\n", "\n") .equals(string2.replaceAll("\r\n", "\n")); @@ -310,7 +310,7 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper ConfigValue define(List path, Supplier defaultSupplier, Predicate validator, Class clazz) { context.setClazz(clazz); - return define(path, new ValueSpec(defaultSupplier, validator, context), defaultSupplier); + return define(path, new ValueSpec(defaultSupplier, validator, context, path), defaultSupplier); } public ConfigValue define(List path, ValueSpec value, Supplier defaultSupplier) { // This is the root where everything at the end of the day ends up. if (!currentPath.isEmpty()) { @@ -363,17 +363,17 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper ConfigValue> defineList(List path, Supplier> defaultSupplier, Predicate elementValidator) { context.setClazz(List.class); - return define(path, new ValueSpec(defaultSupplier, x -> x instanceof List && ((List) x).stream().allMatch( elementValidator ), context) { + return define(path, new ValueSpec(defaultSupplier, x -> x instanceof List && ((List) x).stream().allMatch( elementValidator ), context, path) { @Override public Object correct(Object value) { if (value == null || !(value instanceof List) || ((List)value).isEmpty()) { - LogManager.getLogger().debug(CORE, "List on key {} is deemed to need correction. It is null, not a list, or an empty list. Modders, consider defineListAllowEmpty?", path.get(path.size() - 1)); + LOGGER.debug(CORE, "List on key {} is deemed to need correction. It is null, not a list, or an empty list. Modders, consider defineListAllowEmpty?", path.get(path.size() - 1)); return getDefault(); } List list = Lists.newArrayList((List) value); list.removeIf(elementValidator.negate()); if (list.isEmpty()) { - LogManager.getLogger().debug(CORE, "List on key {} is deemed to need correction. It failed validation.", path.get(path.size() - 1)); + LOGGER.debug(CORE, "List on key {} is deemed to need correction. It failed validation.", path.get(path.size() - 1)); return getDefault(); } return list; @@ -383,17 +383,17 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper ConfigValue> defineListAllowEmpty(List path, Supplier> defaultSupplier, Predicate elementValidator) { context.setClazz(List.class); - return define(path, new ValueSpec(defaultSupplier, x -> x instanceof List && ((List) x).stream().allMatch( elementValidator ), context) { + return define(path, new ValueSpec(defaultSupplier, x -> x instanceof List && ((List) x).stream().allMatch( elementValidator ), context, path) { @Override public Object correct(Object value) { if (value == null || !(value instanceof List)) { - LogManager.getLogger().debug(CORE, "List on key {} is deemed to need correction, as it is null or not a list.", path.get(path.size() - 1)); + LOGGER.debug(CORE, "List on key {} is deemed to need correction, as it is null or not a list.", path.get(path.size() - 1)); return getDefault(); } List list = Lists.newArrayList((List) value); list.removeIf(elementValidator.negate()); if (list.isEmpty()) { - LogManager.getLogger().debug(CORE, "List on key {} is deemed to need correction. It failed validation.", path.get(path.size() - 1)); + LOGGER.debug(CORE, "List on key {} is deemed to need correction. It failed validation.", path.get(path.size() - 1)); return getDefault(); } return list; @@ -479,7 +479,7 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper(this, define(path, new ValueSpec(defaultSupplier, validator, context), defaultSupplier).getPath(), defaultSupplier, converter, clazz); + return new EnumValue(this, define(path, new ValueSpec(defaultSupplier, validator, context, path), defaultSupplier).getPath(), defaultSupplier, converter, clazz); } //boolean @@ -578,7 +578,7 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper path) { currentPath.addAll(path); if (context.hasComment()) { - levelComments.put(new ArrayList(currentPath), context.buildComment()); + levelComments.put(new ArrayList<>(currentPath), context.buildComment(path)); context.clearComment(); // Set to empty } if (context.getTranslationKey() != null) { @@ -635,15 +635,27 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper 0; } - public String buildComment() { return LINE_JOINER.join(comment); } + public String buildComment() { return buildComment(List.of("unknown", "unknown")); } + public String buildComment(final List path) + { + if (comment.stream().allMatch(String::isBlank)) + { + if (FMLEnvironment.production) + LOGGER.warn(CORE, "Detected a comment that is all whitespace for config option {}, which causes obscure bugs in Forge's config system and will cause a crash in the future. Please report this to the mod author.", + DOT_JOINER.join(path)); + else + throw new IllegalStateException("Can not build comment for config option " + DOT_JOINER.join(path) + " as it comprises entirely of blank lines/whitespace. This is not allowed as it causes a \"constantly correcting config\" bug with NightConfig in Forge's config system."); + + return "A developer of this mod has defined this config option with a blank comment, which causes obscure bugs in Forge's config system and will cause a crash in the future. Please report this to the mod author."; + } + + return LINE_JOINER.join(comment); + } public void setTranslationKey(String value) { this.langKey = value; } public String getTranslationKey() { return this.langKey; } public > void setRange(Range value) @@ -710,7 +722,7 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper= 0 && c.compareTo(max) <= 0; if(!result) { - LogManager.getLogger().debug(CORE, "Range value {} is not within its bounds {}-{}", c, min, max); + LOGGER.debug(CORE, "Range value {} is not within its bounds {}-{}", c, min, max); } return result; } @@ -762,12 +774,12 @@ public class ForgeConfigSpec extends UnmodifiableConfigWrapper validator; private Object _default = null; - private ValueSpec(Supplier supplier, Predicate validator, BuilderContext context) + private ValueSpec(Supplier supplier, Predicate validator, BuilderContext context, List path) { Objects.requireNonNull(supplier, "Default supplier can not be null"); Objects.requireNonNull(validator, "Validator can not be null"); - this.comment = context.hasComment() ? context.buildComment() : null; + this.comment = context.hasComment() ? context.buildComment(path) : null; this.langKey = context.getTranslationKey(); this.range = context.getRange(); this.worldRestart = context.needsWorldRestart(); diff --git a/src/main/java/net/minecraftforge/event/entity/living/LivingChangeTargetEvent.java b/src/main/java/net/minecraftforge/event/entity/living/LivingChangeTargetEvent.java index e01d442be8..968a382697 100644 --- a/src/main/java/net/minecraftforge/event/entity/living/LivingChangeTargetEvent.java +++ b/src/main/java/net/minecraftforge/event/entity/living/LivingChangeTargetEvent.java @@ -1,3 +1,8 @@ +/* + * Copyright (c) Forge Development LLC and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + package net.minecraftforge.event.entity.living; import net.minecraft.world.entity.LivingEntity; diff --git a/src/test/java/net/minecraftforge/commontest/ForgeConfigSpecTest.java b/src/test/java/net/minecraftforge/commontest/ForgeConfigSpecTest.java index d31aec228b..76e5e09997 100644 --- a/src/test/java/net/minecraftforge/commontest/ForgeConfigSpecTest.java +++ b/src/test/java/net/minecraftforge/commontest/ForgeConfigSpecTest.java @@ -63,6 +63,22 @@ public class ForgeConfigSpecTest executeSpeedTest("test.test.test.test.test.test.test.test.test.test", "deepKeyValue", "deepKeySpeedTest"); } + @Test(expected = IllegalStateException.class) + public void allEmptyCommentTest() throws IllegalStateException + { + final ForgeConfigSpec.Builder builder = new ForgeConfigSpec.Builder(); + final ForgeConfigSpec.ConfigValue simpleValue = builder.comment("", "").define("allEmptyCommentTest", "someDefaultValue"); + final ForgeConfigSpec spec = builder.build(); + } + + @Test + public void topPaddedCommentTest() + { + final ForgeConfigSpec.Builder builder = new ForgeConfigSpec.Builder(); + final ForgeConfigSpec.ConfigValue simpleValue = builder.comment("", "Test").define("topPaddedCommentTest", "someDefaultValue"); + final ForgeConfigSpec spec = builder.build(); + } + private void executeSpeedTest(final String configKey, final T defaultKeyValue, final String testName) throws IOException { final ForgeConfigSpec.Builder builder = new ForgeConfigSpec.Builder(); diff --git a/src/test/java/net/minecraftforge/debug/block/OnTreeGrowBlockTest.java b/src/test/java/net/minecraftforge/debug/block/OnTreeGrowBlockTest.java index 97fc320d07..b87d4b48a7 100644 --- a/src/test/java/net/minecraftforge/debug/block/OnTreeGrowBlockTest.java +++ b/src/test/java/net/minecraftforge/debug/block/OnTreeGrowBlockTest.java @@ -1,3 +1,8 @@ +/* + * Copyright (c) Forge Development LLC and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + package net.minecraftforge.debug.block; import net.minecraft.core.BlockPos; diff --git a/src/test/java/net/minecraftforge/debug/entity/living/LivingSetAttackTargetEventTest.java b/src/test/java/net/minecraftforge/debug/entity/living/LivingSetAttackTargetEventTest.java index adb7ac065a..aa49d4adc3 100644 --- a/src/test/java/net/minecraftforge/debug/entity/living/LivingSetAttackTargetEventTest.java +++ b/src/test/java/net/minecraftforge/debug/entity/living/LivingSetAttackTargetEventTest.java @@ -1,3 +1,8 @@ +/* + * Copyright (c) Forge Development LLC and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + package net.minecraftforge.debug.entity.living; import net.minecraft.world.entity.monster.piglin.AbstractPiglin; diff --git a/src/test/java/net/minecraftforge/debug/world/BlockGrowFeatureTest.java b/src/test/java/net/minecraftforge/debug/world/BlockGrowFeatureTest.java index 4650030a3c..57dc3a5088 100644 --- a/src/test/java/net/minecraftforge/debug/world/BlockGrowFeatureTest.java +++ b/src/test/java/net/minecraftforge/debug/world/BlockGrowFeatureTest.java @@ -1,3 +1,8 @@ +/* + * Copyright (c) Forge Development LLC and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + package net.minecraftforge.debug.world; import net.minecraft.data.worldgen.features.TreeFeatures;