Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c30a10f
Fix Fixes.fixTypeEverywhere() silently returning input unchanged (#117)
Splatcrafter Mar 21, 2026
d6565fc
Fix Schema.buildTypes() parent type inheritance and thread safety (#1…
Splatcrafter Mar 21, 2026
74808e0
Implement `equals`, `hashCode`, and `toString` in `TaggedDynamic` cla…
Splatcrafter Mar 21, 2026
8d77269
Fix `Typed.encodeAndGet` to return `DataResult` and handle missing pa…
Splatcrafter Mar 21, 2026
479cc70
Validate non-negative index in `Finder.index` method (#122)
Splatcrafter Mar 21, 2026
c7c8362
Remove unused `compose` method from reversed `Iso` implementation (#123)
Splatcrafter Mar 21, 2026
28e12b3
Update `RandomFailuresChaosIT` to handle `FixException` instead of `C…
Splatcrafter Mar 21, 2026
3d16a70
Add unit tests for negative index rejection in `Finder.index()` and `…
Splatcrafter Mar 21, 2026
d4dbc9e
Improve robustness of dynamic key extraction in `Rules` by adding nul…
Splatcrafter Mar 21, 2026
df9343a
Initial plan
Copilot Mar 21, 2026
214b5a9
docs: update getAt @return Javadoc to reflect error-based behavior
Copilot Mar 21, 2026
47db958
Merge pull request #224 from aether-framework/copilot/sub-pr-223
Splatcrafter Mar 21, 2026
89883f0
Initial plan
Copilot Mar 21, 2026
734a3bd
Initial plan
Copilot Mar 21, 2026
a66c154
Update aether-datafixers-api/src/main/java/de/splatgames/aether/dataf…
Splatcrafter Mar 21, 2026
c3b0e11
Merge pull request #226 from aether-framework/copilot/sub-pr-223-anot…
Splatcrafter Mar 21, 2026
4512c3d
Revert "[WIP] [WIP] Address feedback on removing unused methods"
Splatcrafter Mar 21, 2026
7bf14ca
Fix Finder.index() Javadoc: negative index throws IAE at construction…
Copilot Mar 21, 2026
2d0851a
Merge pull request #227 from aether-framework/revert-226-copilot/sub-…
Splatcrafter Mar 21, 2026
06feff8
Merge pull request #225 from aether-framework/copilot/sub-pr-223-again
Splatcrafter Mar 21, 2026
56f8352
Simplify `fixTypeEverywhere` by directly returning `Optional` from `D…
Splatcrafter Mar 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,68 @@ public TypeReference type() {
public Dynamic<?> value() {
return this.value;
}

/**
* Indicates whether some other object is "equal to" this one.
*
* <p>Two {@code TaggedDynamic} instances are considered equal if they have the same type reference and the same
* dynamic value.
* This means that both the type and the data must match for two tagged dynamics to be considered equal.</p>
*
* <h4>Example</h4>
* <pre>{@code
* TaggedDynamic tagged1 = new TaggedDynamic(typeRef, dynamicValue);
* TaggedDynamic tagged2 = new TaggedDynamic(typeRef, dynamicValue);
*
* // tagged1 and tagged2 are equal because they have the same type and value
* boolean isEqual = tagged1.equals(tagged2); // true
* }</pre>
*
* @param obj the reference object with which to compare
* @return {@code true} if this object is the same as the obj argument; {@code false} otherwise
*/
@Override
public boolean equals(final Object obj) {
if (this == obj) {
return true;
}
if (obj == null || getClass() != obj.getClass()) {
return false;
}

final TaggedDynamic that = (TaggedDynamic) obj;
return this.type.equals(that.type) && this.value.equals(that.value);
}

/**
* Returns a hash code value for the object.
*
* <p>The hash code is computed based on both the type reference and the dynamic value. This ensures that equal
* {@code TaggedDynamic} instances will have the same hash code, which is important for correct behavior in
* hash-based collections.</p>
*
* @return a hash code value for this object
*/
@Override
public int hashCode() {
int result = this.type.hashCode();
result = 31 * result + this.value.hashCode();
return result;
}

/**
* Returns a string representation of the object.
*
* <p>The string representation includes the type reference and the dynamic value, providing a human-readable
* summary of the tagged dynamic's contents. This can be useful for debugging and logging purposes.</p>
*
* @return a string representation of the object
*/
@Override
public String toString() {
return "TaggedDynamic{" +
"type=" + this.type +
", value=" + this.value +
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private Fixes() {
* @param rewrite the function that transforms the typed value; must not be {@code null}
* @return a type rewrite rule that applies the transformation to matching types; never {@code null}
* @throws NullPointerException if any parameter is {@code null}
* @see #fixTypeEverywhere(String, Type, Function)
* @see #fixTypeEverywhere(String, DynamicOps, Type, Function)
*/
@NotNull
public static TypeRewriteRule fixTypeEverywhereTyped(@NotNull final String name,
Expand Down Expand Up @@ -156,22 +156,44 @@ public String toString() {
/**
* Creates a rule that transforms the dynamic representation of a type.
*
* @param name the fix name
* @param type the type to transform
* @param rewrite the dynamic transformation function
* @return a type rewrite rule
* <p>This method creates a {@link TypeRewriteRule} that matches values of the specified
* type and transforms them by encoding to {@link Dynamic}, applying the rewrite function, and decoding back. The
* rule only matches types with the same {@link de.splatgames.aether.datafixers.api.TypeReference} as the target
* type.</p>
*
* <h4>Example</h4>
* <pre>{@code
* TypeRewriteRule rule = Fixes.fixTypeEverywhere(
* "addTimestamp",
* GsonOps.INSTANCE,
* playerType,
* dynamic -> dynamic.set("timestamp", dynamic.createLong(0L))
* );
* }</pre>
*
* @param <T> the dynamic type
* @param name the fix name; must not be {@code null}
* @param ops the dynamic ops for encoding/decoding; must not be {@code null}
* @param type the type to transform; must not be {@code null}
* @param rewrite the dynamic transformation function; must not be {@code null}
* @return a type rewrite rule; never {@code null}
* @throws NullPointerException if any parameter is {@code null}
* @see #fixTypeEverywhereTyped(String, Type, Type, Function)
*/
@NotNull
public static TypeRewriteRule fixTypeEverywhere(@NotNull final String name,
@NotNull final Type<?> type,
@NotNull final Function<Dynamic<?>, Dynamic<?>> rewrite) {
public static <T> TypeRewriteRule fixTypeEverywhere(@NotNull final String name,
@NotNull final DynamicOps<T> ops,
@NotNull final Type<?> type,
@NotNull final Function<Dynamic<?>, Dynamic<?>> rewrite) {
Preconditions.checkNotNull(name, "name must not be null");
Preconditions.checkNotNull(ops, "ops must not be null");
Preconditions.checkNotNull(type, "type must not be null");
Preconditions.checkNotNull(rewrite, "rewrite must not be null");

return new TypeRewriteRule() {
@NotNull
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
public Optional<Typed<?>> rewrite(@NotNull final Type<?> inputType,
@NotNull final Typed<?> input) {
Preconditions.checkNotNull(inputType, "inputType must not be null");
Expand All @@ -180,9 +202,11 @@ public Optional<Typed<?>> rewrite(@NotNull final Type<?> inputType,
return Optional.empty();
}

// We need to get DynamicOps from somewhere - this requires the caller to provide it
// For now, we'll use a simplified approach
return Optional.of(input);
final DataResult<Dynamic<T>> encodeResult = input.encode(ops);
return encodeResult.flatMap(dynamic -> {
final Dynamic<?> transformed = rewrite.apply(dynamic);
return ((Type) type).read(transformed);
}).map(newValue -> new Typed<>((Type) type, newValue)).result();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,10 @@ public Dynamic<?> set(@NotNull final Dynamic<?> root,
* Creates a finder that navigates to an element by index in a list/array structure.
*
* <p>The returned finder extracts and modifies the element at the specified
* index position. If the index is out of bounds (negative or beyond the list size), {@link #get} returns
* {@code null} and {@link #set} returns the root unchanged.</p>
* index position. A negative {@code index} is rejected immediately by throwing
* {@link IllegalArgumentException} at construction time. If the index is non-negative
* but beyond the list size, {@link #get} returns {@code null} and {@link #set} returns
* the root unchanged.</p>
*
* <h4>Example</h4>
* <pre>{@code
Expand All @@ -195,16 +197,21 @@ public Dynamic<?> set(@NotNull final Dynamic<?> root,
* Dynamic<?> updated = firstScoreFinder.set(data, data.createInt(100));
* // updated: {"scores": [100, 92, 78]}
*
* // Out of bounds access
* // Out-of-range (positive) index — get returns null, set returns root unchanged
* Finder<?> outOfBounds = scoresFinder.then(Finder.index(10));
* Dynamic<?> missing = outOfBounds.get(data); // null
*
* // Negative index — throws IllegalArgumentException immediately
* Finder.index(-1); // throws IllegalArgumentException
* }</pre>
*
* @param index the zero-based index of the element to focus on
* @param index the zero-based index of the element to focus on; must not be negative
* @throws IllegalArgumentException if {@code index} is negative
* @return a finder that navigates to the element at the specified index, never {@code null}
*/
@NotNull
static Finder<Object> index(final int index) {
Preconditions.checkArgument(index >= 0, "index must not be negative, got: %s", index);
return new Finder<>() {
Comment on lines 211 to 215

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc for Finder.index(...) says negative indices cause get() to return null, but the method now throws IllegalArgumentException at construction time for index < 0. Please update the Javadoc to reflect the new contract (and clarify that only out-of-range positive indices return null/leave root unchanged).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

@NotNull
@Override
Expand Down Expand Up @@ -232,7 +239,7 @@ public String id() {
return root;
}
final var list = listResult.result().orElseThrow().toList();
if (index < 0 || index >= list.size()) {
if (index >= list.size()) {
return root;
}
final List<Dynamic<Object>> newList = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,13 +384,6 @@ public A modify(@NotNull final B source,
Preconditions.checkNotNull(modifier, "modifier must not be null");
return from(modifier.apply(to(source)));
}

@NotNull
@Override
public <C, D> Optic<B, A, C, D> compose(@NotNull final Optic<T, S, C, D> other) {
Preconditions.checkNotNull(other, "other must not be null");
throw new UnsupportedOperationException("Compose on reversed iso");
}
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1568,9 +1568,13 @@ public static <T> TypeRewriteRule flattenField(@NotNull final DynamicOps<T> ops,
Dynamic<T> result = typedDynamic.remove(fieldName);
final var entries = entriesResult.result().orElse(java.util.stream.Stream.empty()).toList();
for (final var entry : entries) {
final String key = entry.first().asString().result().orElse(null);
final Dynamic<?> keyDynamic = entry.first();
final Dynamic<T> value = entry.second();
if (keyDynamic == null || value == null) {
continue;
}
final String key = keyDynamic.asString().result().orElse(null);
if (key != null) {
final Dynamic<T> value = entry.second();
result = result.set(key, value);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@
public class Schema {
private final DataVersion version;
private final Schema parent;
private TypeRegistry types;
private volatile TypeRegistry types;
private TypeRegistry buildingTypes;

/**
* Creates a new schema for the specified version with the given types.
Expand Down Expand Up @@ -161,10 +162,17 @@ public Schema parent() {
*/
@NotNull
public TypeRegistry types() {
if (this.types == null) {
this.types = this.buildTypes();
TypeRegistry result = this.types;
if (result == null) {
synchronized (this) {
result = this.types;
if (result == null) {
result = this.buildTypes();
this.types = result;
}
}
}
return this.types;
return result;
}

/**
Expand All @@ -178,18 +186,22 @@ public TypeRegistry types() {
@NotNull
private TypeRegistry buildTypes() {
final TypeRegistry registry = this.createTypeRegistry();
this.types = registry;
this.buildingTypes = registry;

// Inherit types from parent if present
if (this.parent != null) {
// Copy types from parent
this.parent.types();
// Parent types are already registered in parent's registry
// For now, we don't copy - subclass must re-register all types it needs
final TypeRegistry parentTypes = this.parent.types();
for (final TypeReference ref : parentTypes.references()) {
final Type<?> parentType = parentTypes.get(ref);
if (parentType != null) {
registry.register(parentType);
}
}
}

// Let subclass register types
this.registerTypes();
this.buildingTypes = null;

Comment on lines 187 to 205

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildTypes() sets buildingTypes but only clears it on the happy path. If inheriting parent types or registerTypes() throws, buildingTypes will remain non-null and the Schema instance is left in an inconsistent state (later registerType(...) calls may unexpectedly succeed). Wrap the body in try/finally to ensure buildingTypes is always cleared.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

return registry;
}
Expand Down Expand Up @@ -232,8 +244,9 @@ protected void registerTypes() {
*/
protected final void registerType(@NotNull final Type<?> type) {
Preconditions.checkNotNull(type, "type must not be null");
Preconditions.checkState(this.types != null, "Cannot register types before types() is called");
this.types.register(type);
final TypeRegistry registry = this.buildingTypes;
Preconditions.checkState(registry != null, "Cannot register types outside of registerTypes()");
registry.register(type);
}

/**
Expand Down Expand Up @@ -267,13 +280,14 @@ protected final void registerType(@NotNull final TypeReference reference,
@NotNull final TypeTemplate template) {
Preconditions.checkNotNull(reference, "reference must not be null");
Preconditions.checkNotNull(template, "template must not be null");
Preconditions.checkState(this.types != null, "Cannot register types before types() is called");
final TypeRegistry registry = this.buildingTypes;
Preconditions.checkState(registry != null, "Cannot register types outside of registerTypes()");

// Apply the template with an empty family to get the concrete type
final Type<?> templateType = template.apply(TypeFamily.empty());

// Wrap the template type with the reference
this.types.register(new TemplateBasedType<>(reference, templateType));
registry.register(new TemplateBasedType<>(reference, templateType));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ public <T> DataResult<Typed<A>> updateDynamic(@NotNull final DynamicOps<T> ops,
* @param ops the dynamic operations for encoding, must not be {@code null}
* @param finder the finder that locates the desired sub-value, must not be {@code null}
* @param <T> the underlying data format type
* @return a {@link DataResult} containing the found dynamic value or {@code null} if not found, never {@code null}
* @return a {@link DataResult} containing the found dynamic value on success, or an error result if the path
* was not found; never {@code null}
* @throws NullPointerException if {@code ops} or {@code finder} is {@code null}
* @see #updateAt(DynamicOps, Finder, Function)
*/
Expand All @@ -393,12 +394,12 @@ public <T> DataResult<Dynamic<T>> getAt(@NotNull final DynamicOps<T> ops,
@NotNull final Finder<?> finder) {
Preconditions.checkNotNull(ops, "ops must not be null");
Preconditions.checkNotNull(finder, "finder must not be null");
return encode(ops).map(dynamic -> {
return encode(ops).flatMap(dynamic -> {
final Dynamic<?> found = finder.get(dynamic);
if (found == null) {
return null;
return DataResult.error("Path not found: " + finder);
}
return found.convert(ops);
return DataResult.success(found.convert(ops));
Comment on lines +397 to +402

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAt(...) now returns DataResult.error("Path not found: ...") when the finder doesn't locate a path. The method Javadoc still states the result may contain null when not found; please update the @return documentation to match the new error-based behavior.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot commit to this branch to apply changes based on this feedback

});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.Map;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

/**
* Unit tests for {@link Finder}.
Expand Down Expand Up @@ -167,6 +168,14 @@ void idReturnsDescriptiveIdentifier() {

assertThat(finder.id()).isEqualTo("index[3]");
}

@Test
@DisplayName("index() rejects negative index")
void indexRejectsNegativeIndex() {
assertThatThrownBy(() -> Finder.index(-1))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("must not be negative");
}
}

@Nested
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,26 @@ void reverseAddsReverseToId() {
assertThat(unwrapIso.reverse().id()).isEqualTo("wrapper.unwrap.reverse");
}

@Test
@DisplayName("reverse() supports compose()")
void reverseSupportsCompose() {
// reversed: String -> Wrapper<String>
final Iso<String, String, Wrapper<String>, Wrapper<String>> reversed = unwrapIso.reverse();

// another iso: Wrapper<String> -> String (extract value and uppercase)
final Iso<Wrapper<String>, Wrapper<String>, String, String> extractUpper = Iso.of(
"wrapper.upper",
w -> w.value().toUpperCase(),
s -> new Wrapper<>(s.toLowerCase())
);

// compose: String -> Wrapper<String> -> String (uppercased)
final Iso<String, String, String, String> composed = reversed.compose(extractUpper);

assertThat(composed.to("hello")).isEqualTo("HELLO");
assertThat(composed.from("HELLO")).isEqualTo("hello");
}

@Test
@DisplayName("double reverse returns to original")
void doubleReverseReturnsToOriginal() {
Expand Down
Loading
Loading