NotEvil/TiedUp-
4
2
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

Fix RISK-001 : PlaySoundAction SoundSource codec via comapFlatMap

Browse Source 
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.
This commit is contained in:
notevil
2026-04-27 00:03:08 +02:00
parent fdf7330523
commit 9171ff0def
2 changed files with 154 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
src/main/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundAction.java
View File

@@ -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<SoundSource> 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<SoundSource> 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.
*
* <p>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<SoundSource> SOURCE_CODEC = Codec.STRING.comapFlatMap(
PlaySoundAction::parseSoundSource,
source -> source.getName()
);
/**
* MapCodec for the optional {@code category} field, with strict error
* propagation.
*
* <p><strong>Why not {@code SOURCE_CODEC.optionalFieldOf(...)}?</strong>
* In DFU 6.0.8 (the version shipped with Forge 1.20.1) the
* {@code OptionalFieldCodec.decode} implementation is
* <em>lenient by default</em> — 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.
*
* <p>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<SoundSource> 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<PlaySoundAction> 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

86
src/test/java/com/tiedup/remake/rig/anim/action/impl/PlaySoundActionTest.java
View File

@@ -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<PlaySoundAction> 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<PlaySoundAction> 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<PlaySoundAction> 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<JsonElement> 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<PlaySoundAction> 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);