From 9171ff0def02833190181d2fc2cf61ecd73ae20f Mon Sep 17 00:00:00 2001 From: notevil Date: Mon, 27 Apr 2026 00:03:08 +0200 Subject: [PATCH] Fix RISK-001 : PlaySoundAction SoundSource codec via comapFlatMap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer P3 review convergent finding (HIGH severity). Codec.xmap doesn't wrap IllegalArgumentException in DataResult.error. A typo like 'category: ambiant' (instead of 'ambient') would propagate the IAE up to AnimationManager.apply which catches it but silently skips the entire animation without clear field-level error. Fix : Codec.STRING.comapFlatMap with explicit try/catch + descriptive error message listing valid SoundSource values. Helps modders debug typos at parse-time instead of mysterious silent skips. Two surprises during implementation that required deeper changes : 1. SoundSource.name() != getName() — RECORDS/BLOCKS/PLAYERS have getName() == record/block/player (singular). The previous encoder used getName().toUpperCase() which produced 'BLOCK' but the decoder needed 'BLOCKS'. The new codec is roundtrip-safe via getName() on both sides. 2. DFU 6.0.8 OptionalFieldCodec.decode is lenient by default — a present-but-malformed field is silently mapped to Optional.empty() instead of propagating DataResult.error. The strict 'lenient=false' flag was added in a later DFU release. To surface the error at parse-time the optional category field is now decoded as Codec.STRING.optionalFieldOf().flatXmap(parseSoundSource), which propagates errors correctly. +4 tests : invalid SoundSource returns error, uppercase input accepted, error message lists valid values, encode/roundtrip uses lowercase getName form. --- .../rig/anim/action/impl/PlaySoundAction.java | 72 +++++++++++++++- .../anim/action/impl/PlaySoundActionTest.java | 86 +++++++++++++++++++ 2 files changed, 154 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundAction.java b/src/main/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundAction.java index 90e53d5..4cf3035 100644 --- a/src/main/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundAction.java +++ b/src/main/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundAction.java @@ -4,7 +4,14 @@ package com.tiedup.remake.rig.anim.action.impl; +import java.util.Arrays; +import java.util.Locale; +import java.util.Optional; +import java.util.stream.Collectors; + import com.mojang.serialization.Codec; +import com.mojang.serialization.DataResult; +import com.mojang.serialization.MapCodec; import com.mojang.serialization.codecs.RecordCodecBuilder; import net.minecraft.resources.ResourceLocation; @@ -47,16 +54,73 @@ public record PlaySoundAction( public static final ResourceLocation ID = TiedUpRigConstants.identifier("play_sound"); - private static final Codec SOURCE_CODEC = Codec.STRING.xmap( - s -> SoundSource.valueOf(s.toUpperCase()), - source -> source.getName().toUpperCase() + /** + * Parse a single string into a {@link SoundSource}, accepting the + * canonical lowercase {@link SoundSource#getName()} form (e.g. + * {@code "master"}, {@code "record"}, {@code "block"}, {@code "player"} + * — note these are the singular forms, not the enum constant names like + * {@code RECORDS} / {@code BLOCKS} / {@code PLAYERS}). + */ + private static DataResult parseSoundSource(String s) { + String normalized = s.toLowerCase(Locale.ROOT); + for (SoundSource src : SoundSource.values()) { + if (src.getName().equals(normalized)) { + return DataResult.success(src); + } + } + String valid = Arrays.stream(SoundSource.values()) + .map(SoundSource::getName) + .collect(Collectors.joining(", ")); + return DataResult.error(() -> "Unknown SoundSource: '" + s + "'. Valid values: " + valid); + } + + /** + * Codec for {@link SoundSource} as a non-optional field. + * + *

Uses {@link Codec#comapFlatMap} so that an unknown name produces a + * {@link DataResult#error} with a descriptive message rather than an + * uncaught {@link IllegalArgumentException}. Without this, a typo like + * {@code "ambiant"} would crash the whole animation parse with no clue + * which field was at fault. + */ + private static final Codec SOURCE_CODEC = Codec.STRING.comapFlatMap( + PlaySoundAction::parseSoundSource, + source -> source.getName() ); + /** + * MapCodec for the optional {@code category} field, with strict error + * propagation. + * + *

Why not {@code SOURCE_CODEC.optionalFieldOf(...)}? + * In DFU 6.0.8 (the version shipped with Forge 1.20.1) the + * {@code OptionalFieldCodec.decode} implementation is + * lenient by default — when the inner codec returns + * {@code DataResult.error} for a present value, the optional codec + * silently swallows it and yields {@code Optional.empty()}. The strict + * {@code lenient=false} flag was only added in a later DFU release. + * + *

To surface artist typos like {@code "category": "ambiant"} as a + * real parse error (RISK-001), we instead first decode the raw string + * via {@code Codec.STRING.optionalFieldOf("category")} (which always + * succeeds for strings) and then validate via {@code flatXmap} which + * properly propagates {@link DataResult#error} from the + * {@link #parseSoundSource} validator. + */ + private static final MapCodec CATEGORY_FIELD = Codec.STRING + .optionalFieldOf("category") + .flatXmap( + rawOpt -> rawOpt.isPresent() + ? parseSoundSource(rawOpt.get()).map(s -> s) + : DataResult.success(SoundSource.NEUTRAL), + source -> DataResult.success(Optional.of(source.getName())) + ); + public static final Codec CODEC = RecordCodecBuilder.create(i -> i.group( ResourceLocation.CODEC.fieldOf("sound").forGetter(PlaySoundAction::sound), Codec.FLOAT.optionalFieldOf("volume", 1.0F).forGetter(PlaySoundAction::volume), Codec.FLOAT.optionalFieldOf("pitch", 1.0F).forGetter(PlaySoundAction::pitch), - SOURCE_CODEC.optionalFieldOf("category", SoundSource.NEUTRAL).forGetter(PlaySoundAction::category) + CATEGORY_FIELD.forGetter(PlaySoundAction::category) ).apply(i, PlaySoundAction::new)); @Override diff --git a/src/test/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundActionTest.java b/src/test/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundActionTest.java index 828a078..b11a11f 100644 --- a/src/test/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundActionTest.java +++ b/src/test/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundActionTest.java @@ -115,6 +115,92 @@ class PlaySoundActionTest { assertEquals(1.2F, json.get("pitch").getAsFloat(), 0.0001F); } + /** + * RISK-001 regression — a typo like {@code "ambiant"} (instead of + * {@code "ambient"}) used to throw {@link IllegalArgumentException} from + * {@code SoundSource.valueOf}, which {@link com.mojang.serialization.Codec#xmap} + * does not catch. The fix uses {@code comapFlatMap} so the error becomes + * a proper {@link DataResult#error}. + */ + @Test + void parse_invalidSoundSource_returnsError() { + JsonObject json = new JsonObject(); + json.add("sound", new JsonPrimitive("minecraft:entity.player.levelup")); + json.add("category", new JsonPrimitive("ambiant")); // typo : should be 'ambient' + + DataResult parsed = PlaySoundAction.CODEC.parse(JsonOps.INSTANCE, json); + assertFalse(parsed.result().isPresent(), "parse with invalid SoundSource must fail (no crash)"); + assertTrue(parsed.error().isPresent(), "parse must surface a DataResult error"); + } + + /** + * The decode path normalises input to lowercase before matching against + * {@link SoundSource#getName()} — so accidental casing variants from + * artists still parse correctly. + */ + @Test + void parse_validSoundSource_uppercase_works() { + JsonObject json = new JsonObject(); + json.add("sound", new JsonPrimitive("minecraft:entity.player.levelup")); + json.add("category", new JsonPrimitive("AMBIENT")); // uppercase + + DataResult parsed = PlaySoundAction.CODEC.parse(JsonOps.INSTANCE, json); + assertTrue(parsed.result().isPresent(), "parse with uppercase category must succeed"); + assertEquals(SoundSource.AMBIENT, parsed.result().get().category()); + } + + /** + * The error message must list the valid {@code getName()} values so + * modders can immediately see what to write — without diving into the + * source. + */ + @Test + void parse_unknownCategory_errorMessageListsValidValues() { + JsonObject json = new JsonObject(); + json.add("sound", new JsonPrimitive("minecraft:entity.player.levelup")); + json.add("category", new JsonPrimitive("nonsense")); + + DataResult parsed = PlaySoundAction.CODEC.parse(JsonOps.INSTANCE, json); + assertTrue(parsed.error().isPresent(), "must produce error"); + + String message = parsed.error().get().message(); + // The user-facing names from SoundSource.getName() — singular and + // lowercase, NOT the enum constant names (RECORDS / BLOCKS / PLAYERS). + assertTrue(message.contains("master"), "error must list 'master' : " + message); + assertTrue(message.contains("record"), "error must list 'record' : " + message); + assertTrue(message.contains("block"), "error must list 'block' : " + message); + assertTrue(message.contains("player"), "error must list 'player' : " + message); + assertTrue(message.contains("ambient"),"error must list 'ambient' : "+ message); + assertTrue(message.contains("nonsense"), "error must echo the bad input : " + message); + } + + /** + * Non-default category encodes to its canonical lowercase + * {@link SoundSource#getName()} form — guarantees JSON roundtrip safety + * (the previous {@code .toUpperCase()} encoder was NOT roundtrip-safe + * for {@code RECORDS} / {@code BLOCKS} / {@code PLAYERS} because their + * {@code getName()} differs from their {@code name()}). + */ + @Test + void encode_category_usesLowercaseGetName() { + PlaySoundAction action = new PlaySoundAction( + new ResourceLocation("minecraft", "entity.player.levelup"), + 0.8F, 1.2F, + SoundSource.PLAYERS // name()='PLAYERS', getName()='player' + ); + + DataResult encoded = PlaySoundAction.CODEC.encodeStart(JsonOps.INSTANCE, action); + assertTrue(encoded.result().isPresent()); + + JsonObject json = encoded.result().get().getAsJsonObject(); + assertEquals("player", json.get("category").getAsString()); + + // Roundtrip : the encoded form must decode back to the original. + DataResult reparsed = PlaySoundAction.CODEC.parse(JsonOps.INSTANCE, json); + assertTrue(reparsed.result().isPresent(), "roundtrip must succeed"); + assertEquals(SoundSource.PLAYERS, reparsed.result().get().category()); + } + @Test void type_isStable() { assertEquals(new ResourceLocation("tiedup", "play_sound"), PlaySoundAction.ID);