Skip to content

Commit 68d52f7

Browse files
SplatcrafterSplatcrafter
committed
Refactor error classification, enhance XML/YAML formatting, and improve validation in CLI commands and metrics
1 parent 95f0808 commit 68d52f7

9 files changed

Lines changed: 135 additions & 67 deletions

File tree

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/InfoCommand.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -186,14 +186,14 @@ public Integer call() {
186186
}
187187

188188
if (this.bootstrapClass != null) {
189+
if (this.toVersion == null) {
190+
System.err.println("Error: --to version is required when using --bootstrap");
191+
return 1;
192+
}
193+
189194
try {
190195
final DataFixerBootstrap bootstrap = BootstrapLoader.load(this.bootstrapClass);
191196

192-
if (this.toVersion == null) {
193-
System.err.println("Error: --to version is required when using --bootstrap");
194-
return 1;
195-
}
196-
197197
final AetherDataFixer fixer = new DataFixerRuntimeFactory()
198198
.create(new de.splatgames.aether.datafixers.api.DataVersion(this.toVersion), bootstrap);
199199

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/ValidateCommand.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,10 @@ public Integer call() {
235235
try {
236236
final DataFixerBootstrap bootstrap = BootstrapLoader.load(this.bootstrapClass);
237237
final DataVersion targetVersion = new DataVersion(this.toVersion);
238-
new DataFixerRuntimeFactory()
239-
.create(targetVersion, bootstrap);
238+
// Validate that the bootstrap configuration is correct by creating a DataFixer.
239+
// The instance itself is not needed for version checking, but creation verifies
240+
// that schemas and fixes register without errors.
241+
new DataFixerRuntimeFactory().create(targetVersion, bootstrap);
240242

241243
final FormatHandler<?> handler = FormatRegistry.get(this.format);
242244
if (handler == null) {

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/format/FormatRegistry.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,15 @@ private FormatRegistry() {
161161
public static void register(@NotNull final FormatHandler<?> handler) {
162162
Preconditions.checkNotNull(handler, "handler must not be null");
163163

164-
HANDLERS.put(handler.formatId(), handler);
164+
final String id = handler.formatId();
165+
Preconditions.checkArgument(!id.isEmpty(),
166+
"formatId must not be null or empty");
167+
168+
final String[] extensions = handler.fileExtensions();
169+
Preconditions.checkArgument(extensions != null && extensions.length > 0,
170+
"fileExtensions must not be null or empty");
171+
172+
HANDLERS.put(id, handler);
165173
}
166174

167175
/**

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/format/XmlJacksonFormatHandler.java‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,15 @@
2525
import com.fasterxml.jackson.core.JsonProcessingException;
2626
import com.fasterxml.jackson.databind.JsonNode;
2727
import com.fasterxml.jackson.databind.SerializationFeature;
28+
import com.fasterxml.jackson.dataformat.xml.XmlFactory;
2829
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
2930
import com.google.common.base.Preconditions;
3031
import de.splatgames.aether.datafixers.api.dynamic.DynamicOps;
3132
import de.splatgames.aether.datafixers.codec.xml.jackson.JacksonXmlOps;
3233
import org.jetbrains.annotations.NotNull;
3334

35+
import javax.xml.stream.XMLInputFactory;
36+
3437
/**
3538
* Format handler for XML using the Jackson Dataformat XML library.
3639
*
@@ -77,19 +80,35 @@ public class XmlJacksonFormatHandler implements FormatHandler<JsonNode> {
7780
/**
7881
* XmlMapper instance for compact XML serialization.
7982
*
80-
* <p>Uses default Jackson XML configuration.</p>
83+
* <p>Configured with XXE protection disabled to prevent XML External Entity attacks.</p>
8184
*/
82-
private static final XmlMapper MAPPER = new XmlMapper();
85+
private static final XmlMapper MAPPER = createSecureMapper(false);
8386

8487
/**
8588
* XmlMapper instance for pretty-printed XML serialization.
8689
*
8790
* <p>Configured with {@link SerializationFeature#INDENT_OUTPUT} to produce
88-
* human-readable output with indentation.</p>
91+
* human-readable output with indentation, and XXE protection disabled.</p>
92+
*/
93+
private static final XmlMapper MAPPER_PRETTY = createSecureMapper(true);
94+
95+
/**
96+
* Creates a secure XmlMapper with XXE protection disabled.
97+
*
98+
* @param prettyPrint whether to enable indented output
99+
* @return a configured XmlMapper instance
89100
*/
90-
private static final XmlMapper MAPPER_PRETTY = XmlMapper.builder()
91-
.enable(SerializationFeature.INDENT_OUTPUT)
92-
.build();
101+
private static XmlMapper createSecureMapper(final boolean prettyPrint) {
102+
final XMLInputFactory inputFactory = XMLInputFactory.newInstance();
103+
inputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
104+
inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
105+
106+
final XmlMapper.Builder builder = XmlMapper.builder(new XmlFactory(inputFactory, null));
107+
if (prettyPrint) {
108+
builder.enable(SerializationFeature.INDENT_OUTPUT);
109+
}
110+
return builder.build();
111+
}
93112

94113
/**
95114
* {@inheritDoc}

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/format/YamlSnakeYamlFormatHandler.java‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -71,32 +71,34 @@ public class YamlSnakeYamlFormatHandler implements FormatHandler<Object> {
7171
private final Yaml yaml;
7272

7373
/**
74-
* DumperOptions for compact YAML serialization.
74+
* Yaml instance for compact serialization.
7575
*
7676
* <p>Uses flow style for compact output.</p>
7777
*/
78-
private final DumperOptions compactOptions;
78+
private final Yaml compactYaml;
7979

8080
/**
81-
* DumperOptions for pretty-printed YAML serialization.
81+
* Yaml instance for pretty-printed serialization.
8282
*
8383
* <p>Uses block style with indentation for human-readable output.</p>
8484
*/
85-
private final DumperOptions prettyOptions;
85+
private final Yaml prettyYaml;
8686

8787
/**
8888
* Creates a new SnakeYAML format handler with default configuration.
8989
*/
9090
public YamlSnakeYamlFormatHandler() {
9191
this.yaml = new Yaml();
9292

93-
this.compactOptions = new DumperOptions();
94-
this.compactOptions.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW);
93+
final DumperOptions compactOptions = new DumperOptions();
94+
compactOptions.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW);
95+
this.compactYaml = new Yaml(compactOptions);
9596

96-
this.prettyOptions = new DumperOptions();
97-
this.prettyOptions.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
98-
this.prettyOptions.setIndent(2);
99-
this.prettyOptions.setPrettyFlow(true);
97+
final DumperOptions prettyOptions = new DumperOptions();
98+
prettyOptions.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
99+
prettyOptions.setIndent(2);
100+
prettyOptions.setPrettyFlow(true);
101+
this.prettyYaml = new Yaml(prettyOptions);
100102
}
101103

102104
/**
@@ -190,8 +192,7 @@ public Object parse(@NotNull final String content) {
190192
public String serialize(@NotNull final Object data) {
191193
Preconditions.checkNotNull(data, "data must not be null");
192194

193-
final Yaml compactYaml = new Yaml(this.compactOptions);
194-
return compactYaml.dump(data);
195+
return this.compactYaml.dump(data);
195196
}
196197

197198
/**
@@ -207,7 +208,6 @@ public String serialize(@NotNull final Object data) {
207208
public String serializePretty(@NotNull final Object data) {
208209
Preconditions.checkNotNull(data, "data must not be null");
209210

210-
final Yaml prettyYaml = new Yaml(this.prettyOptions);
211-
return prettyYaml.dump(data);
211+
return this.prettyYaml.dump(data);
212212
}
213213
}

‎aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/report/TextReportFormatter.java‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,7 @@ public String formatSimple(
8282
Preconditions.checkNotNull(type, "type must not be null");
8383
Preconditions.checkNotNull(duration, "duration must not be null");
8484

85-
return String.format(
86-
"Migration: %s [%s] v%d -> v%d (%dms)",
87-
fileName,
88-
type,
89-
fromVersion,
90-
toVersion,
91-
duration.toMillis()
92-
);
85+
return "Migration: " + fileName + " [" + type + "] v" + fromVersion
86+
+ " -> v" + toVersion + " (" + duration.toMillis() + "ms)";
9387
}
9488
}

‎aether-datafixers-spring-boot-starter/src/main/java/de/splatgames/aether/datafixers/spring/actuator/DataFixerHealthIndicator.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ public Health health() {
177177

178178
final Health.Builder builder = Health.up();
179179
builder.withDetail("totalDomains", fixers.size());
180+
boolean allHealthy = true;
180181

181182
for (final Map.Entry<String, AetherDataFixer> entry : fixers.entrySet()) {
182183
final String domain = entry.getKey();
@@ -188,14 +189,18 @@ public Health health() {
188189
builder.withDetail(domain + ".status", "UP");
189190
builder.withDetail(domain + ".currentVersion", currentVersion);
190191
} catch (final Exception e) {
191-
return Health.down()
192-
.withDetail(domain + ".status", "DOWN")
193-
.withDetail(domain + ".error", e.getMessage())
194-
.withDetail("totalDomains", fixers.size())
195-
.build();
192+
allHealthy = false;
193+
builder.withDetail(domain + ".status", "DOWN");
194+
builder.withDetail(domain + ".error", e.getMessage());
196195
}
197196
}
198197

198+
if (!allHealthy) {
199+
return Health.down()
200+
.withDetails(builder.build().getDetails())
201+
.build();
202+
}
203+
199204
return builder.build();
200205
}
201206
}

‎aether-datafixers-spring-boot-starter/src/main/java/de/splatgames/aether/datafixers/spring/metrics/MigrationMetrics.java‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@
2323
package de.splatgames.aether.datafixers.spring.metrics;
2424

2525
import com.google.common.base.Preconditions;
26+
import de.splatgames.aether.datafixers.api.exception.DataFixerException;
27+
import de.splatgames.aether.datafixers.api.exception.DecodeException;
28+
import de.splatgames.aether.datafixers.api.exception.EncodeException;
29+
import de.splatgames.aether.datafixers.api.exception.FixException;
30+
import de.splatgames.aether.datafixers.api.exception.RegistryException;
2631
import io.micrometer.core.instrument.Counter;
2732
import io.micrometer.core.instrument.DistributionSummary;
2833
import io.micrometer.core.instrument.MeterRegistry;
@@ -324,8 +329,38 @@ public void recordFailure(
324329
final int span = Math.abs(toVersion - fromVersion);
325330
getOrCreateVersionSpan(domain).record(span);
326331

327-
// Record failure count with error type
328-
getOrCreateFailureCounter(domain, error.getClass().getSimpleName()).increment();
332+
// Record failure count with classified error type (bounded cardinality)
333+
getOrCreateFailureCounter(domain, classifyError(error)).increment();
334+
}
335+
336+
/**
337+
* Classifies an error into a bounded set of categories for use as a metric tag.
338+
*
339+
* <p>This prevents unbounded cardinality in Micrometer metrics, which can cause
340+
* memory leaks when many distinct exception types are thrown. All errors are
341+
* mapped to one of a fixed set of known categories.</p>
342+
*
343+
* @param error the throwable to classify, must not be {@code null}
344+
* @return a bounded category string for use as a metric tag
345+
*/
346+
@NotNull
347+
private static String classifyError(@NotNull final Throwable error) {
348+
if (error instanceof FixException) {
349+
return "fix_error";
350+
}
351+
if (error instanceof DecodeException) {
352+
return "decode_error";
353+
}
354+
if (error instanceof EncodeException) {
355+
return "encode_error";
356+
}
357+
if (error instanceof RegistryException) {
358+
return "registry_error";
359+
}
360+
if (error instanceof DataFixerException) {
361+
return "datafixer_error";
362+
}
363+
return "unknown_error";
329364
}
330365

331366
/**

‎aether-datafixers-spring-boot-starter/src/test/java/de/splatgames/aether/datafixers/spring/metrics/MigrationMetricsTest.java‎

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -185,39 +185,46 @@ void rejectsNullDuration() {
185185
class FailureRecording {
186186

187187
@Test
188-
@DisplayName("records failure counter with error type")
189-
void recordsFailureCounterWithErrorType() {
188+
@DisplayName("records failure counter with classified error type")
189+
void recordsFailureCounterWithClassifiedErrorType() {
190190
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50),
191191
new RuntimeException("Test"));
192192

193193
Counter counter = registry.find("aether.datafixers.migrations.failure")
194194
.tag("domain", "game")
195-
.tag("error_type", "RuntimeException")
195+
.tag("error_type", "unknown_error")
196196
.counter();
197197

198198
assertThat(counter).isNotNull();
199199
assertThat(counter.count()).isEqualTo(1.0);
200200
}
201201

202202
@Test
203-
@DisplayName("differentiates failure counters by error type")
204-
void differentiatesFailureCountersByErrorType() {
203+
@DisplayName("classifies errors into bounded categories")
204+
void classifiesErrorsIntoBoundedCategories() {
205205
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50),
206-
new RuntimeException("Test"));
206+
new de.splatgames.aether.datafixers.api.exception.FixException("Test"));
207+
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50),
208+
new de.splatgames.aether.datafixers.api.exception.DecodeException("Test"));
207209
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50),
208-
new IllegalStateException("Test"));
210+
new RuntimeException("Test"));
209211

210-
Counter runtimeCounter = registry.find("aether.datafixers.migrations.failure")
212+
Counter fixCounter = registry.find("aether.datafixers.migrations.failure")
211213
.tag("domain", "game")
212-
.tag("error_type", "RuntimeException")
214+
.tag("error_type", "fix_error")
213215
.counter();
214-
Counter illegalStateCounter = registry.find("aether.datafixers.migrations.failure")
216+
Counter decodeCounter = registry.find("aether.datafixers.migrations.failure")
215217
.tag("domain", "game")
216-
.tag("error_type", "IllegalStateException")
218+
.tag("error_type", "decode_error")
219+
.counter();
220+
Counter unknownCounter = registry.find("aether.datafixers.migrations.failure")
221+
.tag("domain", "game")
222+
.tag("error_type", "unknown_error")
217223
.counter();
218224

219-
assertThat(runtimeCounter.count()).isEqualTo(1.0);
220-
assertThat(illegalStateCounter.count()).isEqualTo(1.0);
225+
assertThat(fixCounter.count()).isEqualTo(1.0);
226+
assertThat(decodeCounter.count()).isEqualTo(1.0);
227+
assertThat(unknownCounter.count()).isEqualTo(1.0);
221228
}
222229

223230
@Test
@@ -359,8 +366,8 @@ void handlesSpecialCharactersInDomainName() {
359366
}
360367

361368
@Test
362-
@DisplayName("handles custom exception classes")
363-
void handlesCustomExceptionClasses() {
369+
@DisplayName("classifies custom exception as unknown_error")
370+
void classifiesCustomExceptionAsUnknownError() {
364371
class CustomMigrationException extends RuntimeException {
365372
CustomMigrationException(String msg) {
366373
super(msg);
@@ -371,23 +378,21 @@ class CustomMigrationException extends RuntimeException {
371378
new CustomMigrationException("Test"));
372379

373380
Counter counter = registry.find("aether.datafixers.migrations.failure")
374-
.tag("error_type", "CustomMigrationException")
381+
.tag("error_type", "unknown_error")
375382
.counter();
376383

377384
assertThat(counter.count()).isEqualTo(1.0);
378385
}
379386

380387
@Test
381-
@DisplayName("handles anonymous exception classes")
382-
void handlesAnonymousExceptionClasses() {
383-
Exception anonymousException = new RuntimeException("Test") {
384-
};
385-
386-
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50), anonymousException);
388+
@DisplayName("classifies DataFixerException subtypes correctly")
389+
void classifiesDataFixerExceptionSubtypes() {
390+
metrics.recordFailure("game", 100, 200, Duration.ofMillis(50),
391+
new de.splatgames.aether.datafixers.api.exception.FixException("Test"));
387392

388-
// Anonymous classes have empty simple name, so it uses the base class
389393
Counter counter = registry.find("aether.datafixers.migrations.failure")
390394
.tag("domain", "game")
395+
.tag("error_type", "fix_error")
391396
.counter();
392397

393398
assertThat(counter.count()).isEqualTo(1.0);
@@ -456,7 +461,7 @@ void handlesConcurrentFailureRecordings() throws Exception {
456461

457462
Counter counter = registry.find("aether.datafixers.migrations.failure")
458463
.tag("domain", "game")
459-
.tag("error_type", "RuntimeException")
464+
.tag("error_type", "unknown_error")
460465
.counter();
461466

462467
assertThat(counter.count()).isEqualTo(threadCount * recordsPerThread);

0 commit comments

Comments
 (0)