diff --git a/README.md b/README.md index b5af078..0374206 100644 --- a/README.md +++ b/README.md @@ -219,6 +219,15 @@ diagnostics.reportUnknownBlock("mod:strange_block"); // true the first time, f pre-26.1 layout, and `openRegionCount()` how many files are open right now. `close()` flushes every one of them and is what `ownsLoader(true)` calls for you. +The version guard and the unknown-entry fallback above are both policies, not fixed behaviour: +`ChunkVersionPolicy` decides whether a chunk is readable at all, `UnknownEntryPolicy` decides what an +unknown block or biome becomes, and `falco-anvil` ships a default for each — `DefaultChunkVersionPolicy` +is the 21w43a guard, `DefaultUnknownEntryPolicy` is the air/plains substitution — discovered from the +classpath via `ServiceLoader` unless the builder's `versionPolicy()`/`unknownEntryPolicy()` slots are +used instead. The guard can be removed: `versionPolicy(null)` turns the check off, and nothing stands +in for it — a loader with no guard reads a pre-21w43a world as air again, with no error and no log +line. + ### falco-light — block and sky light Three entry points, in order of how much they do: diff --git a/docs/superpowers/plans/2026-08-04-anvil-extension-points.md b/docs/superpowers/plans/2026-08-04-anvil-extension-points.md new file mode 100644 index 0000000..7c87a8d --- /dev/null +++ b/docs/superpowers/plans/2026-08-04-anvil-extension-points.md @@ -0,0 +1,829 @@ +# Anvil extension points — implementation plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Two hard-wired policies in `falco-anvil` — which worlds are readable, and what an unknown +palette entry becomes — become services a caller can replace or discover. + +**Architecture:** One shared resolution helper implements the discovery rules once. Two service +interfaces use it: `ChunkVersionPolicy` (the guard from #45, moved rather than rewritten) and +`UnknownEntryPolicy` (consulted where the two palette resolvers substitute today). Each has a +built-in implementation, an explicit builder slot, and a `discover…()` slot. + +**Tech Stack:** Java 25, Gradle, `java.util.ServiceLoader` (plain classpath, no JPMS), Adventure NBT, +JUnit 5. + +**Spec:** `docs/superpowers/specs/2026-08-04-anvil-extension-points-design.md` + +**Base:** `main` **after #45 is merged**. This plan moves the body of `requireReadableVersion`, which +only exists on that branch. Do not start before the merge. + +## Global Constraints + +- All three interfaces live in `net.onelitefeather.falco.anvil`. No new module. +- **No `module-info.java`** is added. Plain classpath services. +- `checkApiCompatibility` runs: every signature change is additive. **`PaletteEntryResolver.toId` and `toEntry` keep their exact signatures** — see the decision below. +- Javadoc under `-Werror`, `@param`/`@return`/`@throws` complete, `@since 1.2.0` on new members, `@version` of every changed type raised one minor. +- Builders are immutable: a new field means the constructor, `build()`, and **every** existing setter. +- Test names read as sentences. Tests are package-private, plain JUnit assertions. +- Conventional Commits, lower case. +- No timing figure anywhere. +- Check `uptime` before any test run and record it. Counts come from the JUnit XML, not the console. + +## Two decisions this plan makes that the spec left open + +Both are recorded here because the spec's "Open for the plan" section names them, and because an +implementer would otherwise have to invent them. + +**1. `ChunkVersionPolicy` does not count and does not log.** The spec's sketch was +`check(CompoundBinaryTag, int)`, but the body being moved reads three instance fields of the loader: +`minimumDataVersion`, `diagnostics` and `regionDirectory`. Passing all three into a service would put +the loader's infrastructure into a public contract. Instead the policy **only decides and throws**; +the loader catches, counts and logs, deriving the reported version from the compound it already has. +This keeps the interface free of `AnvilDiagnostics` and keeps every diagnostic in one place. + +**2. `UnknownEntryPolicy` throws an *unchecked* fault.** `PaletteEntryResolver.toId` is +`int toId(String, CompoundBinaryTag)` with no `throws` clause, and it is published API on a 1.0.0 +artefact. Adding a checked exception to it would break every implementor. `AnvilChunkException` +already exists as `non-sealed class … extends RuntimeException implements AnvilFault`, so a policy +that refuses throws that, and no published signature changes. + +## File Structure + +| File | Responsibility | Change | +| --- | --- | --- | +| `…/anvil/ServiceResolution.java` | The discovery rules, once | Create (package-private) | +| `…/anvil/ChunkVersionPolicy.java` | The readable-world contract | Create | +| `…/anvil/DefaultChunkVersionPolicy.java` | #45's body, moved | Create | +| `…/anvil/UnknownEntryPolicy.java` | The unknown-entry contract | Create | +| `…/anvil/DefaultUnknownEntryPolicy.java` | Air and plains, as today | Create | +| `…/anvil/FalcoAnvilLoader.java` | Loader and builder | Modify: two field pairs, four builder slots, `requireReadableVersion` becomes a call | +| `…/anvil/BlockPaletteResolver.java` | Block palette | Modify: substitution goes through the policy | +| `…/anvil/BiomePaletteResolver.java` | Biome palette | Modify: same | +| `falco-anvil/src/main/resources/META-INF/services/…ChunkVersionPolicy` | `falco-anvil`'s own registration | Create | +| Five test classes | | Create/modify per task | + +--- + +### Task 1: The resolution rules, once + +**Files:** +- Create: `falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ServiceResolution.java` +- Test: `falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ServiceResolutionTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `static @Nullable T ServiceResolution.discover(Class service)` — returns the single + provider, `null` if none, throws `IllegalStateException` naming all providers if more than one; + `static @Nullable T ServiceResolution.choose(Class service, @Nullable T explicit, boolean discover)` + — throws `IllegalStateException` if both an explicit instance and `discover` are given, otherwise + returns the explicit one, the discovered one, or `null`. + +- [ ] **Step 1: Write the failing tests** + +The tests need services to find. Register two dummies through the **test** resources so the real +module is unaffected: + +`falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$Dummy` +containing two lines, the two nested implementation class names. + +```java +class ServiceResolutionTest { + + interface Dummy { String name(); } + + public static final class FirstDummy implements Dummy { + public FirstDummy() { } + @Override public String name() { return "first"; } + } + + public static final class SecondDummy implements Dummy { + public SecondDummy() { } + @Override public String name() { return "second"; } + } + + interface Absent { } + + @Test + void testAServiceWithNoProviderResolvesToNothing() { + assertNull(ServiceResolution.discover(Absent.class)); + } + + @Test + void testTwoProvidersAreRefusedAndBothAreNamed() { + IllegalStateException failure = + assertThrows(IllegalStateException.class, () -> ServiceResolution.discover(Dummy.class)); + + assertTrue(failure.getMessage().contains("FirstDummy"), failure.getMessage()); + assertTrue(failure.getMessage().contains("SecondDummy"), failure.getMessage()); + } + + @Test + void testAnExplicitInstanceAndDiscoveryTogetherAreRefused() { + Dummy explicit = () -> "explicit"; + + assertThrows(IllegalStateException.class, + () -> ServiceResolution.choose(Dummy.class, explicit, true)); + } + + @Test + void testAnExplicitInstanceIsUsedWithoutTouchingTheClasspath() { + Dummy explicit = () -> "explicit"; + + assertEquals("explicit", ServiceResolution.choose(Dummy.class, explicit, false).name()); + } + + @Test + void testNeitherExplicitNorDiscoveredResolvesToNothing() { + assertNull(ServiceResolution.choose(Dummy.class, null, false)); + } +} +``` + +The fourth case matters more than it looks: it proves an explicit instance short-circuits before +`ServiceLoader` runs. Without that, `Dummy`'s two providers would make it throw. + +- [ ] **Step 2: Run them and watch them fail** + +Run: `./gradlew :falco-anvil:test --tests "*ServiceResolutionTest*"` +Expected: compilation failure — `ServiceResolution` does not exist. + +- [ ] **Step 3: Implement it** + +```java +final class ServiceResolution { + + private ServiceResolution() { + } + + /** + * Finds the single provider of the given service on the classpath. + * + * @param service the service interface + * @param the service type + * @return the provider, or null if the classpath carries none + * @throws IllegalStateException if more than one provider is registered + */ + static @Nullable T discover(Class service) { + List providers = new ArrayList<>(); + ServiceLoader.load(service).forEach(providers::add); + + if (providers.isEmpty()) { + return null; + } + if (providers.size() > 1) { + // Naming them is the whole value of this branch: "several providers" sends the reader + // to the classpath, the two class names send them to the jar that should not be there. + throw new IllegalStateException( + "Several providers of " + service.getName() + " are registered and none can be chosen for you: " + + providers.stream().map(provider -> provider.getClass().getName()).sorted().toList() + + ". Set one explicitly on the builder instead." + ); + } + return providers.getFirst(); + } + + /** + * Chooses between an explicitly configured instance and classpath discovery. + * + * @param service the service interface + * @param explicit the instance the caller configured, or null + * @param discover whether the caller asked for discovery + * @param the service type + * @return the chosen provider, or null if the caller asked for neither + * @throws IllegalStateException if the caller asked for both, or if discovery is ambiguous + */ + static @Nullable T choose(Class service, @Nullable T explicit, boolean discover) { + if (explicit != null && discover) { + throw new IllegalStateException( + "An explicit " + service.getSimpleName() + " and discovery were both configured. " + + "Choose one: the explicit instance, or the classpath." + ); + } + if (explicit != null) { + return explicit; + } + return discover ? discover(service) : null; + } +} +``` + +- [ ] **Step 4: Run them and watch them pass** + +Run: `./gradlew :falco-anvil:test --tests "*ServiceResolutionTest*"` +Expected: PASS, five cases. + +- [ ] **Step 5: Gegenprobe** + +Change `providers.size() > 1` to `providers.size() > 2`. +`testTwoProvidersAreRefusedAndBothAreNamed` must go red and the other four stay green. Revert, verify +`git status` is clean. + +- [ ] **Step 6: Commit** + +```bash +git add falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ServiceResolution.java \ + falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ServiceResolutionTest.java \ + falco-anvil/src/test/resources/META-INF/services/ +git commit -m "feat(anvil): resolve a service once, and refuse to guess between two" +``` + +--- + +### Task 2: The version policy + +**Files:** +- Create: `…/anvil/ChunkVersionPolicy.java`, `…/anvil/DefaultChunkVersionPolicy.java` +- Create: `falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.ChunkVersionPolicy` +- Modify: `…/anvil/FalcoAnvilLoader.java` — the guard call, one field, two builder slots +- Test: `…/anvil/ChunkVersionPolicyTest.java`, and the existing `FalcoAnvilLoaderIntegrationTest` + +**Interfaces:** +- Consumes: `ServiceResolution.choose` from Task 1. +- Produces: `ChunkVersionPolicy` with + `void check(CompoundBinaryTag data, int minimumDataVersion) throws ChunkDataException`; + `DefaultChunkVersionPolicy` implementing it; builder slots + `versionPolicy(ChunkVersionPolicy)` and `discoverVersionPolicy()`. + +- [ ] **Step 1: Write the failing tests** + +In a new `ChunkVersionPolicyTest`, against the default policy directly — no loader, no environment: + +```java +@Test +void testTheDefaultPolicyRefusesALevelLayout() { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .build(); + + ChunkDataException failure = assertThrows(ChunkDataException.class, + () -> new DefaultChunkVersionPolicy().check(legacy, 2844)); + assertEquals(ChunkDataException.Reason.UNSUPPORTED_CHUNK_VERSION, failure.reason()); +} + +@Test +void testTheDefaultPolicyAcceptsAChunkWithoutAStoredVersion() throws Exception { + CompoundBinaryTag toolWritten = CompoundBinaryTag.builder() + .put("sections", ListBinaryTag.empty()) + .build(); + + new DefaultChunkVersionPolicy().check(toolWritten, 2844); +} + +@Test +void testTheDefaultPolicyRefusesAMistypedVersion() { + CompoundBinaryTag broken = CompoundBinaryTag.builder() + .putString("DataVersion", "not-a-number") + .put("sections", ListBinaryTag.empty()) + .build(); + + assertThrows(ChunkDataException.class, () -> new DefaultChunkVersionPolicy().check(broken, 2844)); +} +``` + +In `FalcoAnvilLoaderIntegrationTest`, that the loader honours a replacement — the fixture helper +`writeRawChunk` is already there: + +```java +@Test +void testAPolicyThatAllowsEverythingLetsALegacyChunkThrough(Env env) throws Exception { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .putString("Status", "minecraft:full") + .build(); + writeRawChunk(11, 11, legacy); + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy((data, minimum) -> { }) + .build(this.worldRoot, OVERWORLD)) { + Instance instance = env.createEmptyInstance(loader); + + assertNotNull(loader.loadChunk(instance, 11, 11)); + } +} + +@Test +void testWithoutAnyPolicyALegacyChunkIsNotChecked(Env env) throws Exception { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .putString("Status", "minecraft:full") + .build(); + writeRawChunk(12, 12, legacy); + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy(null) + .build(this.worldRoot, OVERWORLD)) { + Instance instance = env.createEmptyInstance(loader); + + assertNotNull(loader.loadChunk(instance, 12, 12)); + } +} +``` + +**The second case is the one the spec asks for in executable form.** It documents the cost of the +chosen default: with no policy, a pre-21w43a chunk loads, and it loads as air. Its Javadoc says so. + +Check what `versionPolicy(null)` should mean before writing it — if the builder rejects null, express +"no policy" the way the implementation actually offers it, and say so in the report. + +- [ ] **Step 2: Run them and watch them fail** + +Run: `./gradlew :falco-anvil:test --tests "*ChunkVersionPolicyTest*" --tests "*FalcoAnvilLoaderIntegrationTest*"` +Expected: compilation failure — the type and the slots do not exist. + +- [ ] **Step 3: Create the interface** + +```java +/** + * Decides whether a chunk is one this loader can read. + *

+ * A policy only decides. It does not count and it does not log: the loader catches the failure, + * records it in its {@link AnvilDiagnostics} and writes the log line, so every diagnostic of a load + * stays in one place and this contract stays free of the loader's infrastructure. + *

+ * + * @author TheMeinerLP + * @version 1.0.0 + * @since 1.2.0 + */ +@ApiStatus.Experimental +public interface ChunkVersionPolicy { + + /** + * Checks the given chunk data and throws if the loader cannot read it. + * + * @param data the root compound of the chunk + * @param minimumDataVersion the lowest data version the loader was configured to accept + * @throws ChunkDataException if the chunk cannot be read + */ + void check(CompoundBinaryTag data, int minimumDataVersion) throws ChunkDataException; +} +``` + +- [ ] **Step 4: Move #45's body into the default** + +`DefaultChunkVersionPolicy` takes the body of `requireReadableVersion` **unchanged in its decision +logic**: `versionMissing` from `data.get(DATA_VERSION_KEY) == null`, `versionMistyped`, +`legacyChunkLayout` from `!(… instanceof ListBinaryTag) && optionalCompound(Level) != null`, the same +early return, the same three message branches. What it drops is the diagnostics call and the log +line, which move to the loader per the decision above. + +- [ ] **Step 5: Register it and wire the loader** + +`falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.ChunkVersionPolicy` +holds one line: `net.onelitefeather.falco.anvil.DefaultChunkVersionPolicy`. + +In `FalcoAnvilLoader`: one field `@Nullable ChunkVersionPolicy versionPolicy`, resolved in the +constructor through `ServiceResolution.choose(...)`. Two builder slots, threaded through **every** +setter. At the seam, `requireReadableVersion(data)` becomes: + +```java + if (this.versionPolicy != null) { + checkVersion(data); + } +``` + +where `checkVersion` calls the policy, catches `ChunkDataException`, does the counting and logging +#45 did, and rethrows. + +Extend the existing startup log line with the resolved policy's class name or `none`. + +- [ ] **Step 6: Run them and watch them pass** + +Run: `./gradlew :falco-anvil:test` +Expected: PASS. **Every case #45 added must still pass** — they now exercise the default policy +through the loader instead of a private method, which is the point. + +- [ ] **Step 7: Gegenprobe** + +Two defects, one at a time, each reverted: + +1. Make `checkVersion` swallow the exception instead of rethrowing. Every #45 refusal case must go + red. +2. Delete the `META-INF/services` line. `testAPreRootLayoutChunkIsRefusedInsteadOfReadAsAir` must go + red, because nothing is registered and nothing checks — **and that is the documented cost of the + chosen default, reproduced on purpose.** Note in the report which cases went red; that list is + what the acceptance quotes. + +- [ ] **Step 8: Commit** + +```bash +git add falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ \ + falco-anvil/src/main/resources/META-INF/services/ \ + falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ +git commit -m "feat(anvil): make the version guard a service the caller can replace" +``` + +--- + +### Task 3: The unknown-entry policy + +**Files:** +- Create: `…/anvil/UnknownEntryPolicy.java`, `…/anvil/DefaultUnknownEntryPolicy.java` +- Modify: `…/anvil/BlockPaletteResolver.java`, `…/anvil/BiomePaletteResolver.java`, `…/anvil/FalcoAnvilLoader.java` +- Test: `…/anvil/UnknownEntryPolicyTest.java` + +**Interfaces:** +- Consumes: `ServiceResolution.choose` from Task 1. +- Produces: `UnknownEntryPolicy` with + `int onUnknownBlock(String name, @Nullable CompoundBinaryTag properties)` and + `int onUnknownBiome(String name)`, both unchecked; + `DefaultUnknownEntryPolicy`; builder slots `unknownEntryPolicy(...)` and `discoverUnknownEntryPolicy()`. + +- [ ] **Step 1: Write the failing tests** + +```java +@Test +void testTheDefaultPolicyReplacesAnUnknownBlockWithAir() { + assertEquals(Block.AIR.stateId(), new DefaultUnknownEntryPolicy().onUnknownBlock("falco:nope", null)); +} + +@Test +void testARefusingPolicyFailsTheChunkInsteadOfSubstituting() { + UnknownEntryPolicy refusing = new UnknownEntryPolicy() { + @Override public int onUnknownBlock(String name, CompoundBinaryTag properties) { + throw new AnvilChunkException("The block " + name + " has no mapping"); + } + @Override public int onUnknownBiome(String name) { + throw new AnvilChunkException("The biome " + name + " has no mapping"); + } + }; + + AnvilChunkException failure = assertThrows(AnvilChunkException.class, + () -> new BlockPaletteResolver(new AnvilDiagnostics(), refusing).toId("falco:nope", null)); + assertTrue(failure.getMessage().contains("falco:nope"), failure.getMessage()); +} + +@Test +void testTheResolverStillCountsWhenThePolicySubstitutes() { + AnvilDiagnostics diagnostics = new AnvilDiagnostics(); + + new BlockPaletteResolver(diagnostics, new DefaultUnknownEntryPolicy()).toId("falco:nope", null); + + assertEquals(1, diagnostics.unknownBlockCount()); +} +``` + +Check `AnvilChunkException`'s public constructors before writing case two and use one that exists. +The third case pins that counting stays in the resolver and does not move into the policy — losing it +would make a substituting run silent. + +- [ ] **Step 2: Run them and watch them fail** + +Run: `./gradlew :falco-anvil:test --tests "*UnknownEntryPolicyTest*"` +Expected: compilation failure — the type and the two-argument resolver constructor do not exist. + +- [ ] **Step 3: Create the interface and the default** + +```java +/** + * Decides what becomes of a palette entry the running server does not know. + *

+ * Returning an id substitutes it; throwing {@link AnvilChunkException} fails the chunk. Substituting + * is right for a server that wants a world to stay loadable and wrong for a tool that converts one, + * which is why this is a policy and not a constant. + *

+ * + * @author TheMeinerLP + * @version 1.0.0 + * @since 1.2.0 + */ +@ApiStatus.Experimental +public interface UnknownEntryPolicy { + + /** + * Decides what an unknown block becomes. + * + * @param name the block name stored in the palette + * @param properties the stored properties, or null if the entry carries none + * @return the state id to use instead + * @throws AnvilChunkException if the chunk should fail rather than carry a substitute + */ + int onUnknownBlock(String name, @Nullable CompoundBinaryTag properties); + + /** + * Decides what an unknown biome becomes. + * + * @param name the biome name stored in the palette + * @return the id to use instead + * @throws AnvilChunkException if the chunk should fail rather than carry a substitute + */ + int onUnknownBiome(String name); +} +``` + +`DefaultUnknownEntryPolicy` returns `Block.AIR.stateId()` and the plains id, exactly the values the +two resolvers hard-code today. + +- [ ] **Step 4: Route the resolvers through it** + +`BlockPaletteResolver` gains a second constructor argument. **Keep the one-argument constructor**, +delegating to the new one with the default policy — it is published API and removing it would break +`checkApiCompatibility`. In `toId`, the branch that today does + +```java + if (this.diagnostics.reportUnknownBlock(name)) { + LOGGER.warn("The block '{}' is unknown and is replaced with air, …", name); + } + return Block.AIR.stateId(); +``` + +keeps the reporting and the log **and** ends with `return this.policy.onUnknownBlock(name, properties);`. +The log wording must stop claiming "is replaced with air" unconditionally, because with a refusing +policy it is not. `BiomePaletteResolver` gets the same treatment. + +Add the two builder slots to `FalcoAnvilLoader`, threaded through every setter, and pass the resolved +policy into the resolvers the loader constructs itself. + +- [ ] **Step 5: Run them and watch them pass** + +Run: `./gradlew :falco-anvil:test` +Expected: PASS, including every existing resolver test unchanged. + +- [ ] **Step 6: Gegenprobe** + +Remove the `reportUnknownBlock` call while keeping the policy call. +`testTheResolverStillCountsWhenThePolicySubstitutes` must go red alone. Revert. + +- [ ] **Step 7: Commit** + +```bash +git add falco-anvil/src/ +git commit -m "feat(anvil): let a caller decide what an unknown palette entry becomes" +``` + +--- + +### Task 4: Acceptance + +- [ ] **Step 1: Check the load, then run every module** + +`uptime` before and after, both into the report. Then: + +```bash +./gradlew :falco-anvil:test :falco-light:test :falco-instance:test \ + :falco-demo:test :falco-benchmarks:test :falco-archunit:test --rerun-tasks +``` + +Counts from the JUnit XML. No count may fall against the baseline recorded at the merge of #45. + +- [ ] **Step 2: Build with javadoc and the API check** + +```bash +./gradlew build -x test --rerun-tasks +``` + +Javadoc genuinely executed for all published modules, zero warnings, `checkApiCompatibility` +executed. **If japicmp flags anything, stop and record it verbatim** — the plan's claim is that every +change is additive, and a flag means that claim is wrong. + +- [ ] **Step 3: Attack the gate** + +Re-inject Task 2's Gegenprobe #2 — delete the `META-INF/services` registration — and confirm the full +suite catches it, not just one class. This is also the executable record of what the optional guard +costs. Revert; `git status --short` empty. + +- [ ] **Step 4: Write the result into the plan and commit** + +Append `## Result`: cases added per task, which injected defect each caught, module counts from the +XML, both load figures, and explicitly what this does **not** change — no module, no JPMS, no +published signature, and no behaviour at all when nothing is registered except that the guard is now +losable. + +--- + +### Task 5: Documentation + +- [ ] **Step 1: README** + +One paragraph under the `falco-anvil` material: the loader has replaceable policies, the guard is one +of them, and it can be removed. Do not add a section. + +- [ ] **Step 2: The wiki page** + +Extend `Anvil-Chunk-Loader` in `/mnt/projects/oss/onelitefeather/Falco.wiki` — the page already has a +`### The version floor` subsection from #45, which is where this belongs. Cover all three services, +the three resolution rules, and **the consequence of removing the guard, in the same words the spec +uses.** No new page, so the sidebar is untouched. + +- [ ] **Step 3: Commit both, separately** + +The wiki is its own repository. Do not push either. + +--- + +## Self-Review + +**Spec coverage.** `ChunkVersionPolicy` → Task 2; `UnknownEntryPolicy` → Task 3; the three resolution +rules → Task 1, exercised again in 2 and 3; `falco-anvil`'s own registration → Task 2 Step 5; the +startup line → Task 2 Step 5; the Gegenprobe the spec explicitly asks for → Task 2 Step 7 case 2 and +Task 4 Step 3; documentation → Task 5. `ChunkMigrator` is out of scope and belongs to the migration +plan. + +**Placeholders.** None. Two steps carry a verify-before-you-write instruction rather than an +assumption — `versionPolicy(null)` in Task 2 Step 1 and `AnvilChunkException`'s constructors in Task 3 +Step 1 — because both depend on code this plan does not quote in full, and guessing them would put a +wrong call into a test. + +**Type consistency.** `ServiceResolution.discover`/`choose` are named identically in Task 1's +Interfaces block, its code, and the consumers in Tasks 2 and 3. `ChunkVersionPolicy.check` takes +`(CompoundBinaryTag, int)` everywhere. `UnknownEntryPolicy` has exactly the two methods named in the +spec, both unchecked. `@since 1.2.0` throughout, because #45 already took the module to 1.1.0. + +## Result + +Acceptance run against `a7f7b574` (branch tip), worktree +`/mnt/projects/oss/onelitefeather/Falco-worktrees/anvil-extension-points`, base `1bdd0cca` (#45, +merged into `origin/main`). The branch was already rebased onto `origin/main` and the 19 `@since` tags +redated to `2.1.0` before this measurement. + +This section supersedes an earlier version of itself. That earlier pass measured `1664dcdd` and found +`falco-archunit` red — 4 of `ForeignCouplingTest`'s cases failed, because the three implementation +tasks added `ChunkVersionPolicy`/`DefaultChunkVersionPolicy`/`UnknownEntryPolicy`/ +`DefaultUnknownEntryPolicy` to `net.onelitefeather.falco.anvil` without ever running +`:falco-archunit:test` against them. That finding was reported, not silently fixed, and is preserved +below as "The archunit defect, and how it was actually closed" — it is the most instructive part of +this task and stays in the history rather than being measured away. Three commits landed since that +measurement (`fe569f5b`, `49854313`, `a7f7b574`) and are folded into the numbers below, which are a full +re-measurement, not a patch on the old ones. + +### `@since`/`@version` check (re-verified against the current tip) + +`grep -rn "@since 1.2.0" falco-anvil/src` returns nothing. 17 `@since 2.1.0` tags in +`falco-anvil/src/main`, 2 more in the two new test classes (`ChunkVersionPolicyTest`, +`UnknownEntryPolicyTest`) — 19 total, unaffected by the three commits since the last measurement (none +of them touched an `@since` tag). `@version` tags remain untouched, as they were before. + +### Cases added per task (falco-anvil, measured from the JUnit XML) + +`falco-anvil` now carries 255 cases (was 230 at `1bdd0cca`, +25). 24 new `@Test` methods are +identifiable by name via `git diff 1bdd0cca..HEAD -- falco-anvil/src/test`: + +- **Task 1** (`ServiceResolution`) — `ServiceResolutionTest.java` (new file): 7 — the original five + resolution-rule cases plus `testAForeignProviderWinsOverTheShippedDefault` and + `testTwoForeignProvidersAreStillRefusedEvenWithAShippedDefaultRegistered`, added during Task 2's + review fix round. +- **Task 2** (`ChunkVersionPolicy`) — 7: `ChunkVersionPolicyTest` (new file, 3 cases) + + `FalcoAnvilLoaderIntegrationTest` (+2: `testAPolicyThatAllowsEverythingLetsALegacyChunkThrough`, + `testWithoutAnyPolicyALegacyChunkIsNotChecked`) + `FalcoAnvilLoaderBuilderTest` (+2: + `testAnExplicitVersionPolicySurvivesEveryOtherSetter`, + `testDiscoverVersionPolicySurvivesEveryOtherSetterAfterClearingAnExplicitOne`). +- **Task 3** (`UnknownEntryPolicy`) — 10: `UnknownEntryPolicyTest` (8 cases — the 3 original block + cases, 3 biome mirrors added in Task 3's own review fix round, and 2 more — + `testAnUnusableSubstituteBlockFailsTheChunkAndNamesBothNames`, + `testAnUnusableSubstituteBiomeFailsTheChunkAndNamesBothNames` — added by the interface rework below) + + `FalcoAnvilLoaderBuilderTest` (+2: `testAnExplicitUnknownEntryPolicySurvivesEveryOtherSetter`, + `testDiscoverUnknownEntryPolicySurvivesEveryOtherSetterAfterClearingAnExplicitOne`). + +Named total: 24. The measured module delta is +25 (230 → 255) — the same one-test gap this section +flagged at the previous measurement persists (traced there to the *baseline* figure carried over from +the prior acceptance report, not to anything this branch added or removed since), plus the interface +rework's own net +2 (`testTheDefaultPolicyReplacesAnUnknownBlockWithAir` and +`testTheDefaultPolicyReplacesAnUnknownBiomeWithPlains` were rewritten in place, not added — only the +two "unusable substitute" cases are new). Still a "more than accounted for," not "fewer," gap. + +### The archunit defect, and how it was actually closed + +**Found by this acceptance, reported rather than silently repaired, then fixed by the coordinator in +two separate, targeted commits — not by loosening the rules to fit the code.** + +At the previous measurement (`1664dcdd`), `net.onelitefeather.falco.architecture.ForeignCouplingTest` +failed 4 cases: `anvilCoreKnowsNoMinestom`, `blockRegistryOnlyInAdapters`, +`dynamicRegistryOnlyInBiomeResolver`, `byteLayerKnowsNoNbt`. The four new policy classes touched +Minestom and Kyori-NBT types that pre-existing allow-list regexes in that test did not name, because +none of the three implementation tasks ran `:falco-archunit:test` — it lives outside `falco-anvil` and +outside each task's own file list. + +Two different fixes landed, for two different reasons: + +1. **`fe569f5b` — `fix(anvil): let the unknown-entry policy name a substitute instead of resolving + one`.** This is a real interface change, not a workaround: `UnknownEntryPolicy.onUnknownBlock`/ + `onUnknownBiome` now return a palette **name** (`String`, e.g. `"minecraft:air"`) instead of an + **id** (`int`). `DefaultUnknownEntryPolicy` dropped its `Supplier>` field and + the double-checked-locking lazy resolution entirely — it now returns the literals + `"minecraft:air"`/`"minecraft:plains"` and touches neither Minestom nor a registry. The resolver + that already holds the registry (it just used it to look the original, unknown name up) resolves the + substitute name itself, and fails the chunk — without asking the policy a second time — if that name + is itself unknown, using `IllegalStateException` rather than `AnvilChunkException`: only + `FalcoAnvilLoader` is allowed to construct `AnvilChunkException` (an `exactlyOneTranslationPoint` + rule this same archunit suite enforces elsewhere), and a resolver is not that class. This closed + three of the four rules — `anvilCoreKnowsNoMinestom`, `blockRegistryOnlyInAdapters`, + `dynamicRegistryOnlyInBiomeResolver` — by removing the dependency the rules objected to, not by + widening any allow-list. `ANVIL_MINESTOM_BOUNDARY` (`FalcoAnvilLoader|BlockPaletteResolver| + BiomePaletteResolver`) is unchanged from before this whole plan. + Two new tests, `testAnUnusableSubstituteBlockFailsTheChunkAndNamesBothNames` and + `testAnUnusableSubstituteBiomeFailsTheChunkAndNamesBothNames`, assert the resolver throws + `IllegalStateException` naming both the original and the unusable substitute name, and that the + policy is asked for a substitute exactly once, not twice. +2. **`49854313` — `test(archunit): widen byteLayerKnowsNoNbt for the anvil policy classes`.** The + fourth rule stayed red on its own merits: `ChunkVersionPolicy`/`DefaultChunkVersionPolicy` and + `UnknownEntryPolicy`/`DefaultUnknownEntryPolicy` carry `CompoundBinaryTag` in their contracts by + design (deciding chunk readability, and identifying an unknown block/biome, both need the NBT data), + one layer above `RegionFile`'s pure-byte guarantee that this rule actually protects — there was no + dependency to remove here, unlike the Minestom case. `ANVIL_NBT_LAYER`, the hand-maintained name + list this rule checks against, had its own Javadoc corrected: it previously claimed to be a + self-maintaining complement, which is why nobody had extended it when the four policy classes were + first added. All four are now in the list, and the Javadoc says plainly that it is hand-maintained + and that a class added to this layer later has to be added here too — naming the four policy classes + as the example of what happens when that step is skipped. A Gegenprobe was run for this fix: + removing one class from the list turned the rule red again, and the failure message named exactly + that class. + +### `./gradlew :falco-anvil:test :falco-light:test :falco-instance:test :falco-demo:test :falco-benchmarks:test :falco-archunit:test --rerun-tasks` + +`BUILD SUCCESSFUL`. Module counts (from JUnit XML under `build/test-results/test/`, `` +elements counted directly and cross-checked against each file's `testsuite` summary attributes — the +two agreed on every file): + +| Module | Baseline at `1bdd0cca` (#45) | Count now | Delta | +| --- | --- | --- | --- | +| falco-anvil | 230 | 255 | +25 | +| falco-light | 223 | 223 | 0 | +| falco-instance | 259 | 259 | 0 | +| falco-demo | 167 | 167 | 0 | +| falco-benchmarks | 42 (1 skipped) | 42 (1 skipped) | 0 | +| falco-archunit | 47 | **47** | **0 — the regression from the previous measurement is closed** | + +No count fell. `falco-archunit` is back to 47/47 green, for the reasons above, not because the rules +were softened. + +### `./gradlew build -x test --rerun-tasks` + +`BUILD SUCCESSFUL`. `javadoc` genuinely executed (no `UP-TO-DATE`; full `--rerun-tasks` output grepped +case-insensitively for "warning": zero matches) for the four modules that carry a javadoc task — +falco-anvil, falco-light, falco-instance, falco-demo. `checkApiCompatibility` genuinely executed for +the three configured modules — falco-anvil, falco-light, falco-instance. All three reports, verbatim: + +``` +Comparing binary compatibility of falco-anvil-1.0.0.jar against falco-anvil-1.0.0.jar +No changes. +``` + +(same text for `falco-light-1.0.0.jar` and `falco-instance-1.0.0.jar`.) japicmp raised nothing — +`onlyBinaryIncompatibleModified` is `true` for this task, so purely-additive surface, including +`UnknownEntryPolicy`'s changed return type (`int` → `String`, a real signature change, but to a type +that was never in a published jar — it was introduced and reworked entirely within this unreleased +branch), does not appear here by design. No exception entry was needed in +`gradle/api-breaks.properties` and none was added. + +### Gate attacks (re-verified against the current tip, not re-run) + +Both gate attacks from the prior measurement still apply; per instruction they were not re-executed, +only checked that the test names they cite still exist at the current tip. All do, unchanged: + +**Attack 1 — delete the `ChunkVersionPolicy` service registration.** Confirmed present: +`FalcoAnvilLoaderIntegrationTest.testAPreRootLayoutChunkIsRefusedInsteadOfReadAsAir`, +`testAChunkBelowTheFloorIsRefused`, `testASectionsKeyStoredAsTheWrongTypeWithLevelIsRefused`, +`testAChunkWithADataVersionStoredAsTheWrongTypeIsRefused`, +`testAChunkWithANegativeDataVersionIsRefused`, and +`FalcoAnvilLoaderBuilderTest.testDiscoverVersionPolicySurvivesEveryOtherSetterAfterClearingAnExplicitOne` +— all six exist at the current tip with the same names. Removing the registration made these six fail +(253 tests completed, 6 failed, at the time of that run) — five rejection cases plus the one +pass-through test whose discovery fallback resolves to `null` instead of a `DefaultChunkVersionPolicy` +instance with nothing registered. This remains the executable record of what the optional guard costs: +with no provider on the classpath, every chunk that would have been refused loads instead, unchecked. +Reverted; `git status --short` was empty afterward. + +**Attack 2 — drop the two new policy fields from one builder setter.** Confirmed present: +`FalcoAnvilLoaderBuilderTest.testAnExplicitVersionPolicySurvivesEveryOtherSetter` and +`testAnExplicitUnknownEntryPolicySurvivesEveryOtherSetter` — both exist unchanged. Replacing the +trailing four constructor arguments in `Builder.openRegionLimit(int)` with `null, true, null, true` +made exactly these two go red (20 tests completed, 2 failed), the other 18 unaffected. Reverted; +`git status --short` empty afterward, full `falco-anvil` module re-run 253/253 green at that time (now +255/255 at the current tip, per the module run above). + +### Machine load + +`uptime` before the module run (15:42:09): `load average: 6.84, 7.27, 7.09` +`uptime` after the module run (15:44:29): `load average: 17.86, 13.84, 9.74` + +The machine was measurably busier by the end (consistent with earlier task reports on this same branch +noting a shared, non-idle machine), but every Gradle run in this measurement completed normally with +consistent, repeatable pass/fail outcomes. No timing figure is produced or quoted anywhere in this +section. + +### What this work does not change + +- No new module. Everything lives in `net.onelitefeather.falco.anvil`, across the six existing + published modules. +- No `module-info.java`, no JPMS — plain classpath `ServiceLoader`, as the plan specified. +- No published signature removed or changed — `checkApiCompatibility` confirms this literally ("No + changes.") for all three configured modules; `UnknownEntryPolicy`'s `int` → `String` return-type + change is a real signature change but to a type with no published jar to break. +- No behaviour change for a caller who registers nothing and calls the builder as before: + `discoverVersionPolicy`/`discoverUnknownEntryPolicy` default to `true`, so `builder()` and the two + public constructors resolve `DefaultChunkVersionPolicy`/`DefaultUnknownEntryPolicy` exactly as #45's + guard and the two resolvers' hard-coded fallbacks always did — **except that the guard is now + losable**: deleting one `META-INF/services` line (Attack 1, above) turns the same "default" + configuration into "no check at all," which was not possible before this plan. That loss is a + deliberate project decision, documented in Task 2's own report and re-confirmed by both measurements + of this section, not an oversight. + +### Status + +**DONE.** All four acceptance steps ran to completion on the current tip; every module's test count +held or grew, including `falco-archunit`'s return to 47/47; javadoc and `checkApiCompatibility` are +clean; both gate attacks' test names were confirmed still valid and their previously-recorded results +remain accurate. The one open item from the prior measurement — the `falco-archunit` regression — is +closed, by a real interface fix in one case and a corrected, extended allow-list in the other, both +recorded above rather than left as an unexplained diff. diff --git a/docs/superpowers/specs/2026-08-04-anvil-extension-points-design.md b/docs/superpowers/specs/2026-08-04-anvil-extension-points-design.md new file mode 100644 index 0000000..d37dd91 --- /dev/null +++ b/docs/superpowers/specs/2026-08-04-anvil-extension-points-design.md @@ -0,0 +1,146 @@ +# Extension points for the Anvil loader + +Design of 2026-08-04. `falco-anvil` turns three hard-wired decisions into services a caller can +replace: the version guard, the fallback for an unknown palette entry, and — already specified +separately — chunk migration. All three use the platform `ServiceLoader`. + +Sits between [#45](https://github.com/OneLiteFeatherNET/Falco/pull/45), which built the guard, and +`falco-migration`, which needs the third of them. `2026-08-04-falco-migration-design.md` specifies +`ChunkMigrator` and is not repeated here. + +## Why + +Two behaviours in the loader are decisions the project made for its own use and then fixed in code: + +1. **Which worlds are readable.** #45 refuses anything below `DEFAULT_MINIMUM_DATA_VERSION` or + carrying a `Level` compound. That is right for a server that only wants worlds it can read, and + wrong for a tool that wants to inspect one it cannot. +2. **What an unknown block or biome becomes.** `BlockPaletteResolver` substitutes air, and + `BiomePaletteResolver` substitutes plains, both counting the substitution in `AnvilDiagnostics`. + For a server that keeps a world loadable this is the reasonable last resort. For anything that + converts or audits a world it is the wrong reaction, because a substitution that is counted is + still a substitution written back on the next save. + +Neither is wrong. Both are policy, and policy that only one consumer can choose is not policy. + +`PaletteEntryResolver` already proves the shape: an interface in `falco-anvil`, handed in through the +builder. What it does not offer is discovery — a consumer must know the type exists and wire it. The +services below add that, and keep the builder slot as the explicit route. + +## Decisions + +| Question | Decision | +| --- | --- | +| Mechanism | Plain classpath `ServiceLoader`. No `module-info.java` exists in this project and none is added | +| Guard default | **None. No provider means no version check** | +| Guard shipped by `falco-anvil` | Yes, as a registered provider of its own | +| Fallback default | The current behaviour: air and plains, counted | +| Explicit route | Kept. A builder slot overrides discovery for every service | +| API compatibility | Additive only. No existing signature changes | + +**The guard is fully optional, and that is the project owner's decision, taken with its consequence +stated.** The consequence: a loader with no guard provider reads a pre-21w43a world as air again, +with no error and no log line — the defect #45 exists to close. This spec does not soften that. What +it does is make the state visible rather than silent: + +- `falco-anvil` registers its own guard in its `META-INF/services`, so a normal dependency on the + module has the guard. Losing it takes an exclusion somebody writes, not a classpath that happens to + be empty. +- The loader's existing startup line — which already reports the chosen region layout, and exists + because "without this line the choice between the two layouts happens invisibly" — gains the guard + it resolved, or the word `none`. + +## The services + +### `ChunkVersionPolicy` + +```java +public interface ChunkVersionPolicy { + + void check(CompoundBinaryTag data, int minimumDataVersion) throws ChunkDataException; +} +``` + +Called at the seam where `requireReadableVersion` is called today. The built-in implementation is the +body #45 wrote, moved rather than rewritten: layout first, version second, one `Reason`, the +diagnostics counter. + +A policy that wants to allow everything implements an empty body. A policy that wants a different +floor reads its own configuration. The interface deliberately takes the whole compound, not a version +number, because the layout check does not rest on a version at all. + +### `UnknownEntryPolicy` + +```java +public interface UnknownEntryPolicy { + + String onUnknownBlock(String name, @Nullable CompoundBinaryTag properties); + + String onUnknownBiome(String name); +} +``` + +Consulted by `BlockPaletteResolver` and `BiomePaletteResolver` where they substitute today. Returning +a name substitutes it; throwing `AnvilChunkException` fails the chunk. Built as `String` rather than +`int` as this section originally specified: the id belongs to the registry lookup, and the project's +architecture rule keeps that lookup in exactly one adapter — the resolver that already owns it for the +entry it could not otherwise decode — not duplicated into every implementation of this interface. The +built-in implementation returns `"minecraft:air"` and `"minecraft:plains"` and keeps the existing +counting, so behaviour without a provider is byte-for-byte what it is now. + +This is the seam `falco-migration` needs: a converter installs a policy that throws, because on the +upgrade path an unmappable block means the mapping data is incomplete and must be seen. + +### Resolution rules, shared by all three services + +Applied uniformly to every service here: + +- **The shipped default steps aside for a foreign provider.** `falco-anvil` registers its own + implementations, so without this rule a third party taking the documented route would *always* + produce two providers and always hit the refusal below. Discovery could never return anything but + the default, and the extension point would be decoration. A named default is therefore removed from + the candidate set before the candidates are counted. **Added after the first implementation review + found the point unusable as this document originally specified it.** +- **More than one *foreign* provider throws**, naming them — the default is not among the names, + because by then it is not a candidate. Silent selection between two foreign providers is how a + world gets read under a policy nobody chose. +- **The builder slot and discovery are exclusive.** Setting both is a configuration error. +- **A builder slot always wins over the classpath** when only it is used — that is what "explicit" + means, and it short-circuits before the class path is consulted at all. +- **Discovery loads with the service's own class loader**, not the thread context loader. Under + CloudNet, extension or plugin class loaders the context loader may not see the `falco-anvil` jar; + discovery would then find nothing, resolve to no policy, and put the pre-21w43a air chunk back — + silently, which is the failure mode this whole line of work exists to end. The contract and its + shipped provider live in the same module, so that module's loader is the one to ask. + +Discovery happens once, when the loader is built, not per chunk. + +## What this does not do + +- **No behaviour change with no provider present**, for the fallback. Air and plains, counted, exactly + as today. +- **A behaviour change for the guard**, and it is the point of the change: the guard becomes losable. + See the consequence above. +- **No new module.** All three interfaces live in `falco-anvil`. +- **No JPMS.** Plain classpath services. +- **`ChunkMigrator` is not specified here** — see the migration design. + +## Evidence + +- One case per service that the built-in provider is used when nothing is registered, and that + behaviour matches the current tree. For the fallback this is a regression guard on all of #45's + and the existing resolver tests; for the guard it is the assertion that `falco-anvil`'s own + registration is found. +- One case per service that a registered provider replaces the built-in one. +- One case that two providers throw, and one that builder slot plus discovery throws. +- **The Gegenprobe that matters:** remove `falco-anvil`'s own guard registration and assert that a + pre-21w43a world loads as air again. That test documents the cost of the chosen default in + executable form, so nobody has to take this document's word for it. + +## Open for the plan + +- Whether `ChunkVersionPolicy` and `UnknownEntryPolicy` are one service or two. They are written as + two here because they answer unrelated questions and a consumer will usually want one, not both. +- Where the built-in providers live: a package of their own, or beside the interfaces. +- Whether `AnvilDiagnostics` gains a counter for "policy replaced", so a run says it did not use the + defaults. diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BiomePaletteResolver.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BiomePaletteResolver.java index 78c95f1..8a35996 100644 --- a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BiomePaletteResolver.java +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BiomePaletteResolver.java @@ -16,9 +16,10 @@ * The {@link BiomePaletteResolver} class translates between the biome entries of the Anvil format * and the biome ids of the server registry. *

- * A biome the registry does not know is replaced with the plains biome instead of failing, which - * follows the behaviour of the built-in loader. Every replaced name is reported once through the - * diagnostics. + * A biome the registry does not know is handed to an {@link UnknownEntryPolicy} instead of failing + * outright: the default substitutes plains, which follows the behaviour of the built-in loader, but + * a caller that converts or checks a world can configure a policy which refuses it instead. Every + * unknown name is reported once through the diagnostics, regardless of what the policy does with it. *

*

* The registry is resolved on the first use instead of in a static initializer or in the @@ -32,7 +33,7 @@ *

* * @author TheMeinerLP - * @version 1.0.0 + * @version 1.2.0 * @since 0.1.0 */ @ApiStatus.Experimental @@ -43,17 +44,31 @@ public final class BiomePaletteResolver implements PaletteEntryResolver { private static final String NAME_KEY = "Name"; private final AnvilDiagnostics diagnostics; + private final UnknownEntryPolicy policy; private final Supplier> registrySupplier; - private volatile @Nullable Registries resolved; + private volatile @Nullable DynamicRegistry resolvedRegistry; /** - * Creates a new resolver which uses the biome registry of the running server. + * Creates a new resolver which uses the biome registry of the running server and replaces an + * unknown biome following {@link DefaultUnknownEntryPolicy}. * * @param diagnostics the diagnostics which throttle the reports */ public BiomePaletteResolver(AnvilDiagnostics diagnostics) { - this(diagnostics, MinecraftServer::getBiomeRegistry); + this(diagnostics, new DefaultUnknownEntryPolicy(), MinecraftServer::getBiomeRegistry); + } + + /** + * Creates a new resolver which uses the biome registry of the running server and decides what + * an unknown biome becomes through the given policy. + * + * @param diagnostics the diagnostics which throttle the reports + * @param policy the policy consulted for a biome the registry does not know + * @since 2.1.0 + */ + public BiomePaletteResolver(AnvilDiagnostics diagnostics, UnknownEntryPolicy policy) { + this(diagnostics, policy, MinecraftServer::getBiomeRegistry); } /** @@ -68,62 +83,85 @@ public BiomePaletteResolver(AnvilDiagnostics diagnostics) { * @param registrySupplier the supplier which provides the registry of the known biomes */ public BiomePaletteResolver(AnvilDiagnostics diagnostics, Supplier> registrySupplier) { + this(diagnostics, new DefaultUnknownEntryPolicy(), registrySupplier); + } + + /** + * Creates a new resolver which uses the registry the given supplier provides and decides what an + * unknown biome becomes through the given policy. + *

+ * Package-private on purpose: nothing in this module needs both a custom policy and a custom + * registry at once, and there is no reason to promise that combination as public API before a + * caller actually needs it. It exists for this package's own tests, which use it to exercise + * {@link #toId(String, CompoundBinaryTag)} against a fake registry without a running server. + *

+ * + * @param diagnostics the diagnostics which throttle the reports + * @param policy the policy consulted for a biome the registry does not know + * @param registrySupplier the supplier which provides the registry of the known biomes + * @since 2.1.0 + */ + BiomePaletteResolver(AnvilDiagnostics diagnostics, UnknownEntryPolicy policy, + Supplier> registrySupplier) { this.diagnostics = diagnostics; + this.policy = policy; this.registrySupplier = registrySupplier; } /** * Returns the registry of this resolver and resolves it on the first call. * - * @return the registry and the id of the fallback biome + * @return the registry of the known biomes */ - private Registries registries() { - Registries current = this.resolved; + private DynamicRegistry registry() { + DynamicRegistry current = this.resolvedRegistry; if (current != null) { return current; } synchronized (this) { - Registries created = this.resolved; + DynamicRegistry created = this.resolvedRegistry; if (created == null) { - DynamicRegistry registry = this.registrySupplier.get(); - created = new Registries(registry, registry.getId(Biome.PLAINS)); - this.resolved = created; + created = this.registrySupplier.get(); + this.resolvedRegistry = created; } return created; } } - /** - * The {@link Registries} record holds the resolved registry together with the id of the biome - * which replaces an unknown one. - * - * @param registry the registry which holds the known biomes - * @param fallbackId the id of the biome which replaces an unknown one - * @author TheMeinerLP - * @version 1.0.0 - * @since 0.1.0 - */ - private record Registries(DynamicRegistry registry, int fallbackId) { - } - /** * {@inheritDoc} + * + * @throws AnvilChunkException if the configured policy refuses an unknown biome + * @throws IllegalStateException if the name the policy substitutes is itself unknown; not an + * {@link AnvilChunkException}, because {@code FalcoAnvilLoader} is + * the only place that constructs one, and it wraps this into one + * when a chunk is read through the loader */ @Override public int toId(String name, @Nullable CompoundBinaryTag properties) { - Registries registries = registries(); - int id = registries.registry().getId(RegistryKey.unsafeOf(name)); + DynamicRegistry registry = registry(); + int id = registry.getId(RegistryKey.unsafeOf(name)); if (id != -1) { return id; } if (this.diagnostics.reportUnknownBiome(name)) { - LOGGER.warn("The biome '{}' is unknown and is replaced with plains, further chunks with it are not reported", name); + LOGGER.warn("The biome '{}' is unknown, further chunks with it are not reported", name); + } + String substituteName = this.policy.onUnknownBiome(name); + int substituteId = registry.getId(RegistryKey.unsafeOf(substituteName)); + + // The policy is not consulted a second time for its own substitute, for the same reason + // BlockPaletteResolver does not: a second call could loop, and a substitute the registry + // does not know either is a failure that has to reach the caller, not carry on silently. + if (substituteId == -1) { + throw new IllegalStateException("The biome '" + name + "' is unknown and its substitute '" + + substituteName + "' is unknown too"); } - return registries.fallbackId(); + return substituteId; } /** @@ -131,7 +169,7 @@ public int toId(String name, @Nullable CompoundBinaryTag properties) { */ @Override public CompoundBinaryTag toEntry(int id) { - RegistryKey key = registries().registry().getKey(id); + RegistryKey key = registry().getKey(id); String name = key == null ? Biome.PLAINS.key().asString() : key.key().asString(); return CompoundBinaryTag.builder().putString(NAME_KEY, name).build(); } diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BlockPaletteResolver.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BlockPaletteResolver.java index ccf5705..d7aa498 100644 --- a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BlockPaletteResolver.java +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/BlockPaletteResolver.java @@ -16,10 +16,12 @@ * The {@link BlockPaletteResolver} class translates between the block entries of the Anvil format * and the block state ids of Minestom. *

- * An entry the server does not know is replaced with air instead of failing. A world can hold - * blocks of a mod or of a newer game version and rejecting the whole chunk over a single unknown - * block would lose far more data than it protects. Every replaced name is reported once through - * the diagnostics so the problem stays visible without flooding the log. + * An entry the server does not know is handed to an {@link UnknownEntryPolicy} instead of failing + * outright: the default substitutes air, which keeps a world holding blocks of a mod or of a newer + * game version loadable instead of losing a whole chunk over a single unknown block, but a caller + * that converts or checks a world can configure a policy which refuses it instead. Every unknown + * name is reported once through the diagnostics so the problem stays visible without flooding the + * log, regardless of what the policy does with it. *

* *

@@ -28,7 +30,7 @@ *

* * @author TheMeinerLP - * @version 1.0.0 + * @version 1.2.0 * @since 0.1.0 */ @ApiStatus.Experimental @@ -40,18 +42,39 @@ public final class BlockPaletteResolver implements PaletteEntryResolver { private static final String PROPERTIES_KEY = "Properties"; private final AnvilDiagnostics diagnostics; + private final UnknownEntryPolicy policy; /** - * Creates a new resolver which reports unknown blocks to the given diagnostics. + * Creates a new resolver which reports unknown blocks to the given diagnostics and replaces + * them following {@link DefaultUnknownEntryPolicy}. * * @param diagnostics the diagnostics which throttle the reports */ public BlockPaletteResolver(AnvilDiagnostics diagnostics) { + this(diagnostics, new DefaultUnknownEntryPolicy()); + } + + /** + * Creates a new resolver which reports unknown blocks to the given diagnostics and decides what + * they become through the given policy. + * + * @param diagnostics the diagnostics which throttle the reports + * @param policy the policy consulted for a block the server does not know + * @since 2.1.0 + */ + public BlockPaletteResolver(AnvilDiagnostics diagnostics, UnknownEntryPolicy policy) { this.diagnostics = diagnostics; + this.policy = policy; } /** * {@inheritDoc} + * + * @throws AnvilChunkException if the configured policy refuses an unknown block + * @throws IllegalStateException if the name the policy substitutes is itself unknown; not an + * {@link AnvilChunkException}, because {@code FalcoAnvilLoader} is + * the only place that constructs one, and it wraps this into one + * when a chunk is read through the loader */ @Override public int toId(String name, @Nullable CompoundBinaryTag properties) { @@ -59,9 +82,19 @@ public int toId(String name, @Nullable CompoundBinaryTag properties) { if (block == null) { if (this.diagnostics.reportUnknownBlock(name)) { - LOGGER.warn("The block '{}' is unknown and is replaced with air, further chunks with it are not reported", name); + LOGGER.warn("The block '{}' is unknown, further chunks with it are not reported", name); + } + String substituteName = this.policy.onUnknownBlock(name, properties); + Block substitute = Block.fromKey(substituteName); + + // The policy is not consulted a second time for its own substitute: doing so could loop + // if a policy ever named itself as the replacement, and a substitute the running server + // does not know either is exactly the kind of failure a chunk should not carry silently. + if (substitute == null) { + throw new IllegalStateException("The block '" + name + "' is unknown and its substitute '" + + substituteName + "' is unknown too"); } - return Block.AIR.stateId(); + return substitute.stateId(); } if (properties == null || properties.size() == 0) { return block.stateId(); diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ChunkVersionPolicy.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ChunkVersionPolicy.java new file mode 100644 index 0000000..2c36309 --- /dev/null +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ChunkVersionPolicy.java @@ -0,0 +1,36 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import org.jetbrains.annotations.ApiStatus; + +/** + * Decides whether a chunk is one this loader can read. + *

+ * A policy only decides. It does not count and it does not log: the loader catches the failure, + * records it in its {@link AnvilDiagnostics} and writes the log line, so every diagnostic of a load + * stays in one place and this contract stays free of the loader's infrastructure. + *

+ *

+ * Called from several threads at once. The policy is resolved once, when the loader is built, + * and every load after that consults the same instance — including every parallel load, since + * {@link FalcoAnvilLoader#supportsParallelLoading()} reports {@code true}. An implementation + * therefore has to be thread-safe on its own; the loader takes no lock around the call. + * {@link DefaultChunkVersionPolicy}, the shipped default, holds no state and needs none. + *

+ * + * @author TheMeinerLP + * @version 1.0.0 + * @since 2.1.0 + */ +@ApiStatus.Experimental +public interface ChunkVersionPolicy { + + /** + * Checks the given chunk data and throws if the loader cannot read it. + * + * @param data the root compound of the chunk + * @param minimumDataVersion the lowest data version the loader was configured to accept + * @throws ChunkDataException if the chunk cannot be read + */ + void check(CompoundBinaryTag data, int minimumDataVersion) throws ChunkDataException; +} diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultChunkVersionPolicy.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultChunkVersionPolicy.java new file mode 100644 index 0000000..e6cbae8 --- /dev/null +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultChunkVersionPolicy.java @@ -0,0 +1,88 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import net.kyori.adventure.nbt.ListBinaryTag; +import net.kyori.adventure.nbt.NumberBinaryTag; +import org.jetbrains.annotations.ApiStatus; + +/** + * The {@link ChunkVersionPolicy} a loader resolves when nothing else was configured: refuses the + * pre-1.18 {@code Level} layout and a stored {@code DataVersion} below the configured floor, and + * accepts a chunk that carries no {@code DataVersion} at all. + *

+ * This is the guard against a specific piece of data loss: before snapshot {@code 21w43a}, a + * chunk's block data lived under a {@code Level} compound instead of {@code sections} on the root. + * Reading such a chunk with a loader that only looks for {@code sections} on the root does not + * fail, it silently decodes to an empty section list — a chunk of air. This policy is what turns + * that silent data loss into a thrown exception. + *

+ *

+ * This type is experimental, like everything else in this package. + *

+ * + * @author TheMeinerLP + * @version 1.0.0 + * @since 2.1.0 + */ +@ApiStatus.Experimental +public final class DefaultChunkVersionPolicy implements ChunkVersionPolicy { + + private static final String SECTIONS_KEY = "sections"; + private static final String LEGACY_LEVEL_KEY = "Level"; + private static final String DATA_VERSION_KEY = "DataVersion"; + + /** + * Creates the policy. It holds no state of its own, so every instance behaves the same way. + */ + public DefaultChunkVersionPolicy() { + } + + /** + * Checks the given chunk data and throws if the loader cannot read it. + *

+ * The layout is checked before the version, because a version number is a claim about the data + * while the layout is the data: a chunk may carry no version at all, and one that carries a + * version may not hold what that version promises. A root compound without {@code sections} but + * with a {@code Level} compound is the pre-1.18 shape, which would otherwise decode to an empty + * section list and reach the caller as a chunk of air. + *

+ *

+ * A missing {@code DataVersion} is the one case that is not a rejection: a tool which writes + * {@code sections} on the root but never learned to stamp a version has to keep loading, or a + * whole category of externally-written world becomes unreadable. A key that is present but is not + * the number it claims to be, and a key that holds a negative number, are both a different + * situation from absent: something wrote a value there and it does not describe a version this + * loader can trust, so both are refused rather than waved through the same path as "nothing was + * ever written". + *

+ * + * @param data the root compound of the chunk + * @param minimumDataVersion the lowest data version the loader was configured to accept + * @throws ChunkDataException if the chunk cannot be read + */ + @Override + public void check(CompoundBinaryTag data, int minimumDataVersion) throws ChunkDataException { + boolean versionMissing = data.get(DATA_VERSION_KEY) == null; + // A stored value that is not a number falls back to the same -1 as an absent key, but the + // two are not the same failure: this flag is what lets the exception below say "not a + // number" instead of misreporting a value ("-1") that was never actually stored. + boolean versionMistyped = !versionMissing && !(data.get(DATA_VERSION_KEY) instanceof NumberBinaryTag); + int version = NbtReads.optionalInteger(data, DATA_VERSION_KEY, -1); + boolean legacyChunkLayout = !(data.get(SECTIONS_KEY) instanceof ListBinaryTag) + && NbtReads.optionalCompound(data, LEGACY_LEVEL_KEY) != null; + + if (!legacyChunkLayout && (versionMissing || version >= minimumDataVersion)) { + return; + } + + throw new ChunkDataException( + ChunkDataException.Reason.UNSUPPORTED_CHUNK_VERSION, + legacyChunkLayout + ? "The chunk stores its data under Level, which means a version before 1.18" + : versionMistyped + ? "The chunk does not store its DataVersion as a number" + : "The chunk stores data version " + version + + " but the loader accepts " + minimumDataVersion + " and above" + ); + } +} diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultUnknownEntryPolicy.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultUnknownEntryPolicy.java new file mode 100644 index 0000000..a7cc7ce --- /dev/null +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/DefaultUnknownEntryPolicy.java @@ -0,0 +1,67 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.Nullable; + +/** + * The {@link UnknownEntryPolicy} a resolver falls back to when nothing else was configured: + * substitutes air for an unknown block and plains for an unknown biome, exactly the values + * {@link BlockPaletteResolver} and {@link BiomePaletteResolver} hard-coded before this policy + * existed. + *

+ * Both names are plain string literals. This policy resolves nothing itself — {@link + * UnknownEntryPolicy} hands back a name, not an id, precisely so that the shipped default needs + * neither Minestom nor a registry to answer. The resolver that consults this policy already holds + * the registry it needs to turn {@code "minecraft:air"} or {@code "minecraft:plains"} into an id, + * because it just used that same registry to look the original, unknown name up. + *

+ *

+ * This type is experimental, like everything else in this package. + *

+ * + * @author TheMeinerLP + * @version 1.1.0 + * @since 2.1.0 + */ +@ApiStatus.Experimental +public final class DefaultUnknownEntryPolicy implements UnknownEntryPolicy { + + /** + * The name substituted for an unknown block. + */ + static final String AIR = "minecraft:air"; + + /** + * The name substituted for an unknown biome. + */ + static final String PLAINS = "minecraft:plains"; + + /** + * Creates the policy. It holds no state of its own, so every instance behaves the same way. + */ + public DefaultUnknownEntryPolicy() { + } + + /** + * {@inheritDoc} + *

+ * Always returns {@code "minecraft:air"}. + *

+ */ + @Override + public String onUnknownBlock(String name, @Nullable CompoundBinaryTag properties) { + return AIR; + } + + /** + * {@inheritDoc} + *

+ * Always returns {@code "minecraft:plains"}. + *

+ */ + @Override + public String onUnknownBiome(String name) { + return PLAINS; + } +} diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/FalcoAnvilLoader.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/FalcoAnvilLoader.java index 541482a..0339d19 100644 --- a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/FalcoAnvilLoader.java +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/FalcoAnvilLoader.java @@ -7,7 +7,6 @@ import net.kyori.adventure.nbt.ByteArrayBinaryTag; import net.kyori.adventure.nbt.CompoundBinaryTag; import net.kyori.adventure.nbt.ListBinaryTag; -import net.kyori.adventure.nbt.NumberBinaryTag; import net.kyori.adventure.nbt.StringBinaryTag; import net.minestom.server.MinecraftServer; import net.minestom.server.coordinate.CoordConversion; @@ -79,7 +78,7 @@ *

* * @author TheMeinerLP - * @version 1.1.0 + * @version 1.3.0 * @since 0.1.0 */ @ApiStatus.Experimental @@ -91,7 +90,6 @@ public final class FalcoAnvilLoader implements ChunkLoader, AutoCloseable { private static final BinaryTagIO.Writer TAG_WRITER = BinaryTagIO.writer(); private static final String SECTIONS_KEY = "sections"; - private static final String LEGACY_LEVEL_KEY = "Level"; private static final String DATA_VERSION_KEY = "DataVersion"; private static final String BLOCK_STATES_KEY = "block_states"; private static final String BIOMES_KEY = "biomes"; @@ -130,6 +128,44 @@ public final class FalcoAnvilLoader implements ChunkLoader, AutoCloseable { private final int dataVersion; private final int minimumDataVersion; + /** + * The policy consulted before a chunk is decoded, or null to skip that check entirely. + *

+ * Resolved once, in the constructor, through {@link ServiceResolution#choose(Class, Object, + * boolean, Class)}, naming {@link DefaultChunkVersionPolicy} as the shipped default — so a + * foreign {@link ChunkVersionPolicy} registered through {@code META-INF/services} is chosen over + * it, and only a second foreign provider is refused as ambiguous. A builder which never touches + * {@link Builder#versionPolicy(ChunkVersionPolicy)} or {@link Builder#discoverVersionPolicy()} + * discovers a policy through the classpath by default, which is what keeps every loader built + * the way earlier versions built one refusing the same chunks it always refused. Calling + * {@code versionPolicy(null)} is the only way to leave this field null, and + * {@link #checkVersion(CompoundBinaryTag)} treats that as "check nothing" rather than + * substituting the default itself. + *

+ * + * @since 2.1.0 + */ + private final @Nullable ChunkVersionPolicy versionPolicy; + + /** + * The policy consulted for a palette entry the running server does not know, resolved once in + * the constructor the same way {@link #versionPolicy} is: through {@link + * ServiceResolution#choose(Class, Object, boolean, Class)}, naming {@link + * DefaultUnknownEntryPolicy} as the shipped default. + *

+ * Unlike {@link #versionPolicy}, this field is never null. A builder which never touches + * {@link Builder#unknownEntryPolicy(UnknownEntryPolicy)} or + * {@link Builder#discoverUnknownEntryPolicy()} discovers a policy from the classpath by default, + * falling back to {@link DefaultUnknownEntryPolicy} when the classpath registers none — there is + * no "consult nothing" state for this decision the way {@code versionPolicy(null)} lets a caller + * skip the version check entirely, because an id is always required for the loader to keep + * decoding. + *

+ * + * @since 2.1.0 + */ + private final UnknownEntryPolicy unknownEntryPolicy; + /** * Where failures are reported, or null for the exception manager of the running server. *

@@ -158,6 +194,8 @@ public final class FalcoAnvilLoader implements ChunkLoader, AutoCloseable { * * @param worldRoot the root directory of the world * @param dimension the key of the dimension the loader reads and writes + * @throws IllegalStateException if the classpath registers more than one foreign + * {@link ChunkVersionPolicy} */ public FalcoAnvilLoader(Path worldRoot, Key dimension) { this(worldRoot, dimension, DEFAULT_OPEN_REGION_LIMIT); @@ -176,6 +214,8 @@ public FalcoAnvilLoader(Path worldRoot, Key dimension) { * @param dimension the key of the dimension the loader reads and writes * @param openRegionLimit the amount of region files the loader keeps open * @throws IllegalArgumentException if the limit is not positive + * @throws IllegalStateException if the classpath registers more than one foreign + * {@link ChunkVersionPolicy} */ public FalcoAnvilLoader(Path worldRoot, Key dimension, int openRegionLimit) { this(worldRoot, dimension, builder().openRegionLimit(openRegionLimit)); @@ -207,11 +247,38 @@ private FalcoAnvilLoader(Path worldRoot, Key dimension, Builder settings) { this.legacyLayout = resolved.legacyLayout(); this.dimensionLabel = dimension.asString(); this.diagnostics = effective; + // A caller who names both a policy and their own resolver has built a loader in which the + // policy can never be reached: the resolver a builder is handed is used exactly as given, + // never rebuilt around the configured policy, so unknownEntryPolicy() below would keep + // reporting a policy that the actual decoding path never consults. Refusing the combination + // holds this to the same standard ServiceResolution.choose already applies to explicit + // configuration versus discovery: two conflicting explicit decisions are refused outright, + // not silently reconciled by picking one of them. A resolver configured without touching + // either unknownEntryPolicy slot is unaffected, because unknownEntryPolicyConfigured stays + // false for a builder that never called unknownEntryPolicy(...) or + // discoverUnknownEntryPolicy() itself. + if (settings.unknownEntryPolicyConfigured && (settings.blockResolver != null || settings.biomeResolver != null)) { + throw new IllegalStateException( + "An UnknownEntryPolicy was configured together with a custom " + + (settings.blockResolver != null ? "blockResolver" : "biomeResolver") + + ". A resolver supplied through the builder is used exactly as given and never " + + "sees the configured policy, so it would silently keep its own fallback instead. " + + "Pass the policy into the resolver you build instead, e.g. " + + "new BlockPaletteResolver(diagnostics, policy), and stop configuring it on this " + + "builder." + ); + } + UnknownEntryPolicy resolvedUnknownEntryPolicy = ServiceResolution.choose( + UnknownEntryPolicy.class, settings.unknownEntryPolicy, settings.discoverUnknownEntryPolicy, + DefaultUnknownEntryPolicy.class); + this.unknownEntryPolicy = resolvedUnknownEntryPolicy == null + ? new DefaultUnknownEntryPolicy() + : resolvedUnknownEntryPolicy; this.blockResolver = settings.blockResolver == null - ? new BlockPaletteResolver(effective) + ? new BlockPaletteResolver(effective, this.unknownEntryPolicy) : settings.blockResolver; this.biomeResolver = settings.biomeResolver == null - ? new BiomePaletteResolver(effective) + ? new BiomePaletteResolver(effective, this.unknownEntryPolicy) : settings.biomeResolver; this.regions = new ConcurrentHashMap<>(); this.trackedChunks = new ConcurrentHashMap<>(); @@ -219,6 +286,9 @@ private FalcoAnvilLoader(Path worldRoot, Key dimension, Builder settings) { this.dataVersion = settings.dataVersion; this.minimumDataVersion = settings.minimumDataVersion; this.exceptionHandler = settings.exceptionHandler; + this.versionPolicy = ServiceResolution.choose( + ChunkVersionPolicy.class, settings.versionPolicy, settings.discoverVersionPolicy, + DefaultChunkVersionPolicy.class); this.closeLock = new ReentrantLock(); // Which directory was chosen, and how many region files are in it, is the first thing @@ -226,12 +296,13 @@ private FalcoAnvilLoader(Path worldRoot, Key dimension, Builder settings) { // two layouts happens invisibly, and a world whose files sit in the other one looks exactly // like a world which is empty. LOGGER.info( - "Opening the anvil loader for region={} layout={} exists={} regionFiles={} dim={}", + "Opening the anvil loader for region={} layout={} exists={} regionFiles={} dim={} versionPolicy={}", this.regionDirectory, this.legacyLayout ? "legacy /region" : "dimension /dimensions///region", Files.isDirectory(this.regionDirectory), describeRegionFileCount(this.regionDirectory), - this.dimensionLabel + this.dimensionLabel, + this.versionPolicy == null ? "none" : this.versionPolicy.getClass().getName() ); } @@ -258,8 +329,12 @@ private void reportException(Throwable exception) { * Returns a builder for a loader whose defaults are those of the constructors. *

* The builder reaches the values the constructors set for themselves — the compression level, - * the diagnostics, both palette resolvers, the save parallelism and the data version. The world - * directory and the dimension are not among them: they are required, so they sit in + * the diagnostics, both palette resolvers, the save parallelism and the data version. It also + * defaults to discovering a {@link ChunkVersionPolicy} and an {@link UnknownEntryPolicy} from + * the classpath, which is what keeps every loader built through a constructor rather than an + * explicit {@link Builder#versionPolicy(ChunkVersionPolicy)} or + * {@link Builder#unknownEntryPolicy(UnknownEntryPolicy)} call behaving the way it always did. + * The world directory and the dimension are not among them: they are required, so they sit in * {@link Builder#build(Path, Key)} rather than in a slot. *

* @@ -269,7 +344,7 @@ private void reportException(Throwable exception) { public static Builder builder() { return new Builder(DEFAULT_OPEN_REGION_LIMIT, ChunkCompression.DEFAULT_LEVEL, Math.max(Runtime.getRuntime().availableProcessors(), 2), MinecraftServer.DATA_VERSION, - DEFAULT_MINIMUM_DATA_VERSION, null, null, null, null); + DEFAULT_MINIMUM_DATA_VERSION, null, null, null, null, null, true, null, true, false); } /** @@ -298,7 +373,7 @@ public static Builder builder() { *

* * @author TheMeinerLP - * @version 1.1.0 + * @version 1.3.0 * @since 0.4.0 */ @ApiStatus.Experimental @@ -313,12 +388,22 @@ public static final class Builder { private final @Nullable PaletteEntryResolver blockResolver; private final @Nullable PaletteEntryResolver biomeResolver; private final @Nullable Consumer exceptionHandler; + private final @Nullable ChunkVersionPolicy versionPolicy; + private final boolean discoverVersionPolicy; + private final @Nullable UnknownEntryPolicy unknownEntryPolicy; + private final boolean discoverUnknownEntryPolicy; + private final boolean unknownEntryPolicyConfigured; private Builder(int openRegionLimit, int compressionLevel, int saveParallelism, int dataVersion, int minimumDataVersion, @Nullable AnvilDiagnostics diagnostics, @Nullable PaletteEntryResolver blockResolver, @Nullable PaletteEntryResolver biomeResolver, - @Nullable Consumer exceptionHandler) { + @Nullable Consumer exceptionHandler, + @Nullable ChunkVersionPolicy versionPolicy, + boolean discoverVersionPolicy, + @Nullable UnknownEntryPolicy unknownEntryPolicy, + boolean discoverUnknownEntryPolicy, + boolean unknownEntryPolicyConfigured) { this.openRegionLimit = openRegionLimit; this.compressionLevel = compressionLevel; this.saveParallelism = saveParallelism; @@ -328,6 +413,11 @@ private Builder(int openRegionLimit, int compressionLevel, int saveParallelism, this.blockResolver = blockResolver; this.biomeResolver = biomeResolver; this.exceptionHandler = exceptionHandler; + this.versionPolicy = versionPolicy; + this.discoverVersionPolicy = discoverVersionPolicy; + this.unknownEntryPolicy = unknownEntryPolicy; + this.discoverUnknownEntryPolicy = discoverUnknownEntryPolicy; + this.unknownEntryPolicyConfigured = unknownEntryPolicyConfigured; } /** @@ -354,7 +444,12 @@ public Builder openRegionLimit(int openRegionLimit) { this.diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -385,7 +480,12 @@ public Builder compressionLevel(int compressionLevel) { this.diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -412,7 +512,12 @@ public Builder saveParallelism(int saveParallelism) { this.diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -436,7 +541,12 @@ public Builder dataVersion(int dataVersion) { this.diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -466,7 +576,12 @@ public Builder minimumDataVersion(int minimumDataVersion) { this.diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -495,7 +610,12 @@ public Builder diagnostics(AnvilDiagnostics diagnostics) { diagnostics, this.blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -507,6 +627,14 @@ public Builder diagnostics(AnvilDiagnostics diagnostics) { * blocks although the resolver saw them. {@link PaletteEntryResolver} exposes no * diagnostics, so this cannot be checked in {@code build}. *

+ *

+ * Do not combine this with {@link #unknownEntryPolicy(UnknownEntryPolicy)} or + * {@link #discoverUnknownEntryPolicy()}. Whatever policy those slots resolve to is never + * handed to a resolver supplied here — {@link #build(Path, Key)} refuses the combination + * rather than build a loader whose configured policy is silently unreachable. Pass the + * policy into your own resolver instead, the same way the shipped resolvers accept one in + * their constructor. + *

* * @param blockResolver the resolver for block palette entries * @return a new builder with this value @@ -521,14 +649,21 @@ public Builder blockResolver(PaletteEntryResolver blockResolver) { this.diagnostics, blockResolver, this.biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** * Sets the resolver which turns biome palette entries into ids. *

* The same caveat as {@link #blockResolver(PaletteEntryResolver)}: a resolver of your own - * counts past the diagnostics of the loader. + * counts past the diagnostics of the loader, and the same restriction against combining it + * with {@link #unknownEntryPolicy(UnknownEntryPolicy)} or + * {@link #discoverUnknownEntryPolicy()} applies, for the same reason. *

* * @param biomeResolver the resolver for biome palette entries @@ -544,7 +679,12 @@ public Builder biomeResolver(PaletteEntryResolver biomeResolver) { this.diagnostics, this.blockResolver, biomeResolver, - this.exceptionHandler); + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); } /** @@ -576,7 +716,186 @@ public Builder exceptionHandler(Consumer exceptionHandler) { this.diagnostics, this.blockResolver, this.biomeResolver, - exceptionHandler); + exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); + } + + /** + * Sets the policy consulted before a chunk is decoded, or clears it so nothing is consulted. + *

+ * Calling this slot always turns discovery off, whatever value is passed: an explicit + * decision about the policy — including the explicit decision "none" — has to win over the + * classpath without {@link ServiceResolution#choose(Class, Object, boolean, Class)} seeing + * both set at once and refusing to guess between them. A builder that never calls this slot + * keeps discovering the default instead, which is what {@link #builder()} starts with. + *

+ *

+ * Passing {@code null} is not "use the default": it is "check nothing". A pre-{@code + * 21w43a} chunk that would otherwise be refused loads as a chunk of air instead, exactly as + * this loader read one before {@link ChunkVersionPolicy} existed. That cost is deliberate — + * see {@code testWithoutAnyPolicyALegacyChunkIsNotChecked} in the loader's integration + * tests for the case that documents it. + *

+ *

+ * An explicit instance passed here is shared by every loader this builder builds afterward, + * the same way an explicit {@link #diagnostics(AnvilDiagnostics)} instance is — and + * {@link ChunkVersionPolicy} is called from every one of those loaders' parallel loads at + * once, so it has to tolerate that sharing the way {@link AnvilDiagnostics} already does. + *

+ * + * @param versionPolicy the policy to consult before a chunk is decoded, or null to consult + * none + * @return a new builder with this value + * @since 2.1.0 + */ + @Contract(value = "_ -> new", pure = true) + public Builder versionPolicy(@Nullable ChunkVersionPolicy versionPolicy) { + return new Builder(this.openRegionLimit, + this.compressionLevel, + this.saveParallelism, + this.dataVersion, + this.minimumDataVersion, + this.diagnostics, + this.blockResolver, + this.biomeResolver, + this.exceptionHandler, + versionPolicy, + false, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); + } + + /** + * Asks the loader to discover its {@link ChunkVersionPolicy} from the classpath instead of + * using an explicit instance. + *

+ * This is what {@link #builder()} already defaults to, so calling it only matters after an + * earlier {@link #versionPolicy(ChunkVersionPolicy)} call on the same chain, to undo it. The + * shipped {@link DefaultChunkVersionPolicy} steps aside for a single registered foreign + * provider; a classpath that registers more than one foreign provider makes + * {@link Builder#build(Path, Key)} throw {@link IllegalStateException} rather than guess + * between them. + *

+ * + * @return a new builder with this value + * @since 2.1.0 + */ + @Contract(value = "-> new", pure = true) + public Builder discoverVersionPolicy() { + return new Builder(this.openRegionLimit, + this.compressionLevel, + this.saveParallelism, + this.dataVersion, + this.minimumDataVersion, + this.diagnostics, + this.blockResolver, + this.biomeResolver, + this.exceptionHandler, + null, + true, + this.unknownEntryPolicy, + this.discoverUnknownEntryPolicy, + this.unknownEntryPolicyConfigured); + } + + /** + * Sets the policy consulted for a palette entry the running server does not know, or clears + * it so the classpath default is used instead. + *

+ * Calling this slot always turns discovery off, whatever value is passed, for the same + * reason {@link #versionPolicy(ChunkVersionPolicy)} does: an explicit decision has to win + * over the classpath without {@link ServiceResolution#choose(Class, Object, boolean, Class)} + * seeing both set at once and refusing to guess between them. A builder that never calls + * this slot keeps discovering the default instead, which is what {@link #builder()} starts + * with. + *

+ *

+ * Unlike {@link #versionPolicy(ChunkVersionPolicy)}, passing {@code null} here is not "check + * nothing": {@link #build(Path, Key)} always resolves a usable policy, falling back to + * {@link DefaultUnknownEntryPolicy} when neither an explicit instance nor a foreign + * classpath provider is found, because a resolver always needs an id for the entry it could + * not otherwise decode. + *

+ * + *

+ * Do not combine this with {@link #blockResolver(PaletteEntryResolver)} or + * {@link #biomeResolver(PaletteEntryResolver)}. A resolver supplied through either of + * those slots is used exactly as given and is never rebuilt around this policy, so + * {@link #build(Path, Key)} refuses the combination instead of building a loader whose + * configured policy would never actually run. + *

+ *

+ * An explicit instance passed here is shared by every loader this builder builds afterward, + * the same way an explicit {@link #diagnostics(AnvilDiagnostics)} instance is — and + * {@link UnknownEntryPolicy} is called from every one of those loaders' parallel loads at + * once, so it has to tolerate that sharing the way {@link AnvilDiagnostics} already does. + *

+ * + * @param unknownEntryPolicy the policy to consult for an unknown palette entry, or null to + * fall back to the classpath default + * @return a new builder with this value + * @since 2.1.0 + */ + @Contract(value = "_ -> new", pure = true) + public Builder unknownEntryPolicy(@Nullable UnknownEntryPolicy unknownEntryPolicy) { + return new Builder(this.openRegionLimit, + this.compressionLevel, + this.saveParallelism, + this.dataVersion, + this.minimumDataVersion, + this.diagnostics, + this.blockResolver, + this.biomeResolver, + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + unknownEntryPolicy, + false, + true); + } + + /** + * Asks the loader to discover its {@link UnknownEntryPolicy} from the classpath instead of + * using an explicit instance. + *

+ * This is what {@link #builder()} already defaults to, so calling it only matters after an + * earlier {@link #unknownEntryPolicy(UnknownEntryPolicy)} call on the same chain, to undo it. + * The shipped {@link DefaultUnknownEntryPolicy} steps aside for a single registered foreign + * provider; a classpath that registers more than one foreign provider makes + * {@link Builder#build(Path, Key)} throw {@link IllegalStateException} rather than guess + * between them. + *

+ *

+ * Calling this together with {@link #blockResolver(PaletteEntryResolver)} or + * {@link #biomeResolver(PaletteEntryResolver)} is refused by {@link #build(Path, Key)} for + * the same reason {@link #unknownEntryPolicy(UnknownEntryPolicy)} is: a resolver supplied + * through either of those slots never sees whatever this discovers. + *

+ * + * @return a new builder with this value + * @since 2.1.0 + */ + @Contract(value = "-> new", pure = true) + public Builder discoverUnknownEntryPolicy() { + return new Builder(this.openRegionLimit, + this.compressionLevel, + this.saveParallelism, + this.dataVersion, + this.minimumDataVersion, + this.diagnostics, + this.blockResolver, + this.biomeResolver, + this.exceptionHandler, + this.versionPolicy, + this.discoverVersionPolicy, + null, + true, + true); } /** @@ -590,6 +909,16 @@ public Builder exceptionHandler(Consumer exceptionHandler) { * @param worldRoot the root directory of the world * @param dimension the key of the dimension the loader reads and writes * @return a new loader, independent of every other loader from this builder + * @throws IllegalStateException if this builder was left on classpath discovery and the + * classpath registers more than one foreign + * {@link ChunkVersionPolicy} or more than one foreign + * {@link UnknownEntryPolicy}, or if + * {@link #unknownEntryPolicy(UnknownEntryPolicy)} or + * {@link #discoverUnknownEntryPolicy()} was called on this + * builder together with {@link #blockResolver(PaletteEntryResolver)} + * or {@link #biomeResolver(PaletteEntryResolver)} — a custom + * resolver never sees the configured policy, so the combination + * would silently drop it instead of applying it */ @Contract(value = "_, _ -> new", pure = true) public FalcoAnvilLoader build(Path worldRoot, Key dimension) { @@ -706,7 +1035,9 @@ private record ResolvedRegionDirectory(Path directory, boolean legacyLayout) { } CompoundBinaryTag data = TAG_READER.read(new ByteArrayInputStream(raw.decompress()), BinaryTagIO.Compression.NONE); - requireReadableVersion(data); + if (this.versionPolicy != null) { + checkVersion(data); + } String status = chunkStatus(data); if (!isFullyGenerated(status)) { @@ -1068,6 +1399,36 @@ int minimumDataVersion() { return this.minimumDataVersion; } + /** + * Returns the policy this loader resolved, or null if it checks nothing. + *

+ * Package-private for the same reason as {@link #minimumDataVersion()}: this exists for the + * builder's own pass-through tests, not for a caller outside this package. + *

+ * + * @return the resolved policy, or null if the loader checks no chunk version at all + * @since 2.1.0 + */ + @Contract(pure = true) + @Nullable ChunkVersionPolicy versionPolicy() { + return this.versionPolicy; + } + + /** + * Returns the policy this loader resolved for an unknown palette entry. + *

+ * Package-private for the same reason as {@link #versionPolicy()}: this exists for the + * builder's own pass-through tests, not for a caller outside this package. + *

+ * + * @return the resolved policy, never null + * @since 2.1.0 + */ + @Contract(pure = true) + UnknownEntryPolicy unknownEntryPolicy() { + return this.unknownEntryPolicy; + } + /** * Closes every region file the loader opened and reports a summary of its work. *

@@ -1411,61 +1772,54 @@ public int openRegionCount() { } /** - * Refuses a chunk which comes from a version this loader cannot read. - *

- * The layout is checked before the version, because a version number is a claim about the data - * while the layout is the data: a chunk may carry no version at all, and one that carries a - * version may not hold what that version promises. A root compound without {@code sections} but - * with a {@code Level} compound is the pre-1.18 shape, which would otherwise decode to an empty - * section list and reach the caller as a chunk of air. - *

+ * Consults {@link #versionPolicy} about a chunk, and reports and counts a refusal before it + * reaches the caller. *

- * A missing {@code DataVersion} is the one case that is not a rejection: a tool which writes - * {@code sections} on the root but never learned to stamp a version has to keep loading, or a - * whole category of externally-written world becomes unreadable. A key that is present but is not - * the number it claims to be, and a key that holds a negative number, are both a different - * situation from absent: something wrote a value there and it does not describe a version this - * loader can trust, so both are refused rather than waved through the same path as "nothing was - * ever written". + * The policy only decides and throws; it does not know about {@link AnvilDiagnostics} or the + * logger, on purpose — see {@link ChunkVersionPolicy}. Counting and logging the refusal is + * therefore the loader's job, done here rather than duplicated at every call site, and done + * only when {@link #versionPolicy} is not null: the caller at {@link #loadChunk(Instance, int, + * int)} already guards the call for that reason. *

* * @param data the root compound of the chunk - * @throws ChunkDataException if the chunk cannot be read + * @throws ChunkDataException if the policy refuses the chunk */ - private void requireReadableVersion(CompoundBinaryTag data) throws ChunkDataException { - boolean versionMissing = data.get(DATA_VERSION_KEY) == null; - // A stored value that is not a number falls back to the same -1 as an absent key, but the - // two are not the same failure: this flag is what lets the exception below say "not a - // number" instead of misreporting a value ("-1") that was never actually stored. - boolean versionMistyped = !versionMissing && !(data.get(DATA_VERSION_KEY) instanceof NumberBinaryTag); - int version = NbtReads.optionalInteger(data, DATA_VERSION_KEY, -1); - String reported = versionMissing ? AnvilDiagnostics.UNKNOWN_DATA_VERSION : Integer.toString(version); - boolean legacyChunkLayout = !(data.get(SECTIONS_KEY) instanceof ListBinaryTag) - && NbtReads.optionalCompound(data, LEGACY_LEVEL_KEY) != null; - - if (!legacyChunkLayout && (versionMissing || version >= this.minimumDataVersion)) { - return; - } - - if (this.diagnostics.reportUnsupportedChunkVersion(reported)) { - LOGGER.warn( - "Refusing a chunk from data version {} in {}: {}", - reported, this.regionDirectory, - legacyChunkLayout - ? "the chunk data sits under Level, which this loader does not read" - : "the loader accepts " + this.minimumDataVersion + " and above" - ); + private void checkVersion(CompoundBinaryTag data) throws ChunkDataException { + try { + this.versionPolicy.check(data, this.minimumDataVersion); + } catch (ChunkDataException failure) { + String reported = reportedDataVersion(data); + + if (this.diagnostics.reportUnsupportedChunkVersion(reported)) { + LOGGER.warn( + "Refusing a chunk from data version {} in {}: {}", + reported, this.regionDirectory, failure.getMessage() + ); + } + throw failure; } + } - throw new ChunkDataException( - ChunkDataException.Reason.UNSUPPORTED_CHUNK_VERSION, - legacyChunkLayout - ? "The chunk stores its data under Level, which means a version before 1.18" - : versionMistyped - ? "The chunk does not store its DataVersion as a number" - : "The chunk stores data version " + version - + " but the loader accepts " + this.minimumDataVersion + " and above" - ); + /** + * Renders the stored {@code DataVersion} of a chunk the way the version breakdown of + * {@link AnvilDiagnostics} groups it: by the number it holds, or by + * {@link AnvilDiagnostics#UNKNOWN_DATA_VERSION} for a chunk which stores none. + *

+ * This mirrors only the presentation the guard used before it became a policy, not its + * decision: a key that is present but not a number renders as {@code "-1"} here exactly as it + * always did, because {@link NbtReads#optionalInteger(CompoundBinaryTag, String, int)} falls + * back to that default for a mistyped value the same way it does for a missing one. + *

+ * + * @param data the root compound of the chunk + * @return the data version to report, or {@link AnvilDiagnostics#UNKNOWN_DATA_VERSION} + */ + @Contract(pure = true) + private static String reportedDataVersion(CompoundBinaryTag data) { + return data.get(DATA_VERSION_KEY) == null + ? AnvilDiagnostics.UNKNOWN_DATA_VERSION + : Integer.toString(NbtReads.optionalInteger(data, DATA_VERSION_KEY, -1)); } /** diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/PaletteEntryResolver.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/PaletteEntryResolver.java index b78a4c3..11384fe 100644 --- a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/PaletteEntryResolver.java +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/PaletteEntryResolver.java @@ -20,7 +20,7 @@ *

* * @author TheMeinerLP - * @version 1.0.0 + * @version 1.1.0 * @since 0.1.0 */ @ApiStatus.Experimental @@ -29,15 +29,22 @@ public interface PaletteEntryResolver { /** * Resolves the id which belongs to the given palette entry. *

- * An implementation must not fail for an unknown name. A world can hold entries of a mod or of - * a newer game version and losing a whole chunk over a single unknown entry would destroy more - * data than it protects. An implementation is expected to return a replacement id instead and - * to report the name to the caller. + * An implementation is allowed to fail for an unknown name — unchecked, since this method + * declares no {@code throws} clause. {@link BlockPaletteResolver} and + * {@link BiomePaletteResolver} both delegate that decision to a caller-supplied + * {@link UnknownEntryPolicy} rather than deciding it themselves: the shipped default + * substitutes a replacement id, which keeps a world holding entries of a mod or of a newer game + * version loadable instead of losing a whole chunk over a single unknown entry, but a policy + * configured to refuse instead throws {@link AnvilChunkException} from here. A caller of + * {@code toId} therefore has to be ready for either outcome, depending on how the resolver it + * holds was configured. *

* * @param name the name of the palette entry * @param properties the properties of the palette entry or null if it carries none * @return the id which belongs to the entry + * @throws AnvilChunkException if the implementation was configured to refuse an unknown name + * instead of substituting one */ int toId(String name, @Nullable CompoundBinaryTag properties); diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ServiceResolution.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ServiceResolution.java new file mode 100644 index 0000000..0d66d88 --- /dev/null +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/ServiceResolution.java @@ -0,0 +1,144 @@ +package net.onelitefeather.falco.anvil; + +import org.jetbrains.annotations.Nullable; + +import java.util.ArrayList; +import java.util.List; +import java.util.ServiceLoader; + +/** + * Resolves the two decisions that currently sit hardcoded in the loader as classpath services: + * which worlds are readable, and what an unknown block becomes. This type builds the resolution + * rules once, so the extension points built on top of it do not each reinvent them. + *

+ * A caller either configures an instance explicitly, or asks for classpath discovery, but not both + * at once: {@link #choose(Class, Object, boolean)} refuses the combination outright rather than + * silently preferring one side. Discovery itself, in {@link #discover(Class)}, refuses to guess + * between more than one registered provider — except that a module's own shipped default, named + * through {@link #discover(Class, Class)}, always steps aside for a foreign one rather than + * counting as a second vote. Two foreign providers still refuse each other. + *

+ * + * @author TheMeinerLP + * @version 1.1.0 + * @since 1.0.0 + */ +final class ServiceResolution { + + private ServiceResolution() { + } + + /** + * Finds the single provider of the given service on the classpath. + * + * @param service the service interface + * @param the service type + * @return the provider, or null if the classpath carries none + * @throws IllegalStateException if more than one provider is registered + */ + static @Nullable T discover(Class service) { + return discover(service, null); + } + + /** + * Finds the single provider of the given service on the classpath, letting a module's own + * shipped default step aside for a foreign one instead of counting as a competing vote. + *

+ * This is not the "no silent choice between two providers" rule weakening: a module's own + * default is a known quantity, registered by the module itself, not a second undocumented + * opinion on the classpath. A caller who registers one foreign provider has stated an + * unambiguous intent, and that intent should not be refused just because the module also ships + * its own fallback. Two foreign providers are a different situation — neither of them + * is the known default, so the classpath genuinely holds two competing, undocumented opinions, + * and this still refuses to guess between them exactly as {@link #discover(Class)} does. + *

+ * + * @param service the service interface + * @param shippedDefault the class of the module's own default implementation, which yields to + * any other registered provider, or null if the service has no such + * default + * @param the service type + * @return the provider, or null if the classpath carries none + * @throws IllegalStateException if more than one provider other than {@code shippedDefault} is + * registered + * @since 2.1.0 + */ + static @Nullable T discover(Class service, @Nullable Class shippedDefault) { + List providers = new ArrayList<>(); + // The thread's context classloader is not a safe bet here: in a CloudNet, extension or + // plugin classloader environment it may not see this jar at all, which would make discovery + // silently resolve to nothing instead of finding the provider that is right there on this + // module's own classloader. The service interface and its providers live in the same + // module, so that module's classloader is what has to be asked, not whatever classloader + // happens to be current on the calling thread. + ServiceLoader.load(service, service.getClassLoader()).forEach(providers::add); + + if (providers.isEmpty()) { + return null; + } + + if (shippedDefault != null) { + List foreign = providers.stream() + .filter(provider -> !shippedDefault.equals(provider.getClass())) + .toList(); + + if (!foreign.isEmpty()) { + providers = foreign; + } + } + + if (providers.size() > 1) { + // Naming them is the whole value of this branch: "several providers" sends the reader + // to the classpath, the two class names send them to the jar that should not be there. + throw new IllegalStateException( + "Several providers of " + service.getName() + " are registered and none can be chosen for you: " + + providers.stream().map(provider -> provider.getClass().getName()).sorted().toList() + + ". Set one explicitly on the builder instead." + ); + } + return providers.getFirst(); + } + + /** + * Chooses between an explicitly configured instance and classpath discovery. + * + * @param service the service interface + * @param explicit the instance the caller configured, or null + * @param discover whether the caller asked for discovery + * @param the service type + * @return the chosen provider, or null if the caller asked for neither + * @throws IllegalStateException if the caller asked for both, or if discovery is ambiguous + */ + static @Nullable T choose(Class service, @Nullable T explicit, boolean discover) { + return choose(service, explicit, discover, null); + } + + /** + * Chooses between an explicitly configured instance and classpath discovery, letting discovery + * treat a module's own shipped default as stepping aside for a foreign provider. + * + * @param service the service interface + * @param explicit the instance the caller configured, or null + * @param discover whether the caller asked for discovery + * @param shippedDefault the class of the module's own default implementation, passed on to + * {@link #discover(Class, Class)}, or null if the service has no such + * default + * @param the service type + * @return the chosen provider, or null if the caller asked for neither + * @throws IllegalStateException if the caller asked for both, or if discovery is ambiguous + * @since 2.1.0 + */ + static @Nullable T choose(Class service, @Nullable T explicit, boolean discover, + @Nullable Class shippedDefault) { + if (explicit != null && discover) { + throw new IllegalStateException( + "An explicit " + service.getSimpleName() + " and discovery were both configured. " + + "Choose one: the explicit instance, or the classpath." + ); + } + if (explicit != null) { + return explicit; + } + return discover ? discover(service, shippedDefault) : null; + } +} diff --git a/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/UnknownEntryPolicy.java b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/UnknownEntryPolicy.java new file mode 100644 index 0000000..b36ec4e --- /dev/null +++ b/falco-anvil/src/main/java/net/onelitefeather/falco/anvil/UnknownEntryPolicy.java @@ -0,0 +1,63 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.Nullable; + +/** + * Decides what becomes of a palette entry the running server does not know. + *

+ * Returning a name substitutes it; throwing {@link AnvilChunkException} fails the chunk. + * Substituting is right for a server that wants a world to stay loadable and wrong for a tool that + * converts one, which is why this is a policy and not a constant. + *

+ *

+ * A policy names a replacement; it does not resolve one. It returns a palette name such as + * {@code "minecraft:stone"}, not an id — the resolver that consults it owns the registry lookup + * that turns a name into an id, the same registry it already needed to look the original, unknown + * name up in the first place. That split keeps this interface, and any implementation of it, free of + * a dependency on Minestom or any registry: naming {@code "minecraft:air"} takes no more than a + * string literal. + *

+ *

+ * A policy only decides. It does not count and it does not log: the resolver which consults it keeps + * reporting the name to its {@link AnvilDiagnostics} and writing the log line regardless of what the + * policy does with the entry, so a substituting run stays as visible as a refusing one. Nor does a + * policy get asked twice: if the name it returns is itself unknown, the resolver fails the chunk + * instead of consulting the policy again, which would risk a loop. + *

+ *

+ * Called from several threads at once. The policy is resolved once, when the loader is built, + * and both {@link BlockPaletteResolver} and {@link BiomePaletteResolver} keep consulting that same + * instance for as long as the loader lives — including every parallel load, since + * {@link FalcoAnvilLoader#supportsParallelLoading()} reports {@code true}. An implementation + * therefore has to be thread-safe on its own; neither resolver takes a lock around the call. + * {@link DefaultUnknownEntryPolicy}, the shipped default, holds no state and needs none. + *

+ * + * @author TheMeinerLP + * @version 1.1.0 + * @since 2.1.0 + */ +@ApiStatus.Experimental +public interface UnknownEntryPolicy { + + /** + * Decides what an unknown block becomes. + * + * @param name the block name stored in the palette + * @param properties the stored properties, or null if the entry carries none + * @return the name of the block to use instead + * @throws AnvilChunkException if the chunk should fail rather than carry a substitute + */ + String onUnknownBlock(String name, @Nullable CompoundBinaryTag properties); + + /** + * Decides what an unknown biome becomes. + * + * @param name the biome name stored in the palette + * @return the name of the biome to use instead + * @throws AnvilChunkException if the chunk should fail rather than carry a substitute + */ + String onUnknownBiome(String name); +} diff --git a/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.ChunkVersionPolicy b/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.ChunkVersionPolicy new file mode 100644 index 0000000..67b2e89 --- /dev/null +++ b/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.ChunkVersionPolicy @@ -0,0 +1 @@ +net.onelitefeather.falco.anvil.DefaultChunkVersionPolicy diff --git a/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.UnknownEntryPolicy b/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.UnknownEntryPolicy new file mode 100644 index 0000000..1d14ce4 --- /dev/null +++ b/falco-anvil/src/main/resources/META-INF/services/net.onelitefeather.falco.anvil.UnknownEntryPolicy @@ -0,0 +1 @@ +net.onelitefeather.falco.anvil.DefaultUnknownEntryPolicy diff --git a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ChunkVersionPolicyTest.java b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ChunkVersionPolicyTest.java new file mode 100644 index 0000000..588710a --- /dev/null +++ b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ChunkVersionPolicyTest.java @@ -0,0 +1,56 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import net.kyori.adventure.nbt.ListBinaryTag; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests {@link DefaultChunkVersionPolicy} directly, against chunk data built by hand rather than + * through the loader. What used to be the loader's private {@code requireReadableVersion} guard now + * lives here, unchanged in its decision logic and reachable without a running Minestom environment. + * + * @author TheMeinerLP + * @version 1.0.0 + * @since 2.1.0 + */ +class ChunkVersionPolicyTest { + + @Test + void testTheDefaultPolicyRefusesALevelLayout() { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .build(); + + ChunkDataException failure = assertThrows(ChunkDataException.class, + () -> new DefaultChunkVersionPolicy().check(legacy, 2844)); + assertEquals(ChunkDataException.Reason.UNSUPPORTED_CHUNK_VERSION, failure.reason()); + } + + @Test + void testTheDefaultPolicyAcceptsAChunkWithoutAStoredVersion() throws Exception { + CompoundBinaryTag toolWritten = CompoundBinaryTag.builder() + .put("sections", ListBinaryTag.empty()) + .build(); + + new DefaultChunkVersionPolicy().check(toolWritten, 2844); + } + + @Test + void testTheDefaultPolicyRefusesAMistypedVersion() { + CompoundBinaryTag broken = CompoundBinaryTag.builder() + .putString("DataVersion", "not-a-number") + .put("sections", ListBinaryTag.empty()) + .build(); + + ChunkDataException failure = assertThrows(ChunkDataException.class, + () -> new DefaultChunkVersionPolicy().check(broken, 2844)); + // Pins the distinction the message is for, not its exact wording: a mistyped DataVersion is + // not the same failure as a DataVersion that is simply too old, and the loader's log line now + // relays this message instead of recomputing that distinction itself. + assertTrue(failure.getMessage().contains("number"), failure.getMessage()); + } +} diff --git a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderBuilderTest.java b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderBuilderTest.java index e18eca6..fd56a70 100644 --- a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderBuilderTest.java +++ b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderBuilderTest.java @@ -20,10 +20,12 @@ import java.util.concurrent.CopyOnWriteArrayList; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * Pins down the builder of the loader. @@ -319,6 +321,120 @@ void testTheMinimumDataVersionSurvivesEveryOtherSetter(@TempDir Path worldRoot) } } + @Test + void testAnExplicitVersionPolicySurvivesEveryOtherSetter(@TempDir Path worldRoot) throws Exception { + ChunkVersionPolicy policy = (data, minimum) -> { + }; + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy(policy) + .openRegionLimit(4) + .compressionLevel(3) + .saveParallelism(2) + .build(worldRoot, OVERWORLD)) { + + assertSame(policy, loader.versionPolicy()); + } + } + + @Test + void testDiscoverVersionPolicySurvivesEveryOtherSetterAfterClearingAnExplicitOne(@TempDir Path worldRoot) throws Exception { + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy((data, minimum) -> { + }) + .discoverVersionPolicy() + .openRegionLimit(4) + .compressionLevel(3) + .saveParallelism(2) + .build(worldRoot, OVERWORLD)) { + + assertInstanceOf(DefaultChunkVersionPolicy.class, loader.versionPolicy()); + } + } + + @Test + void testAnExplicitUnknownEntryPolicySurvivesEveryOtherSetter(@TempDir Path worldRoot) throws Exception { + UnknownEntryPolicy policy = new DefaultUnknownEntryPolicy(); + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .unknownEntryPolicy(policy) + .openRegionLimit(4) + .compressionLevel(3) + .saveParallelism(2) + .build(worldRoot, OVERWORLD)) { + + assertSame(policy, loader.unknownEntryPolicy()); + } + } + + @Test + void testDiscoverUnknownEntryPolicySurvivesEveryOtherSetterAfterClearingAnExplicitOne(@TempDir Path worldRoot) throws Exception { + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .unknownEntryPolicy(new DefaultUnknownEntryPolicy()) + .discoverUnknownEntryPolicy() + .openRegionLimit(4) + .compressionLevel(3) + .saveParallelism(2) + .build(worldRoot, OVERWORLD)) { + + assertInstanceOf(DefaultUnknownEntryPolicy.class, loader.unknownEntryPolicy()); + } + } + + /** + * The combination this class exists to refuse: an explicit {@link UnknownEntryPolicy} and a + * custom resolver together. A resolver supplied through {@link Builder#blockResolver} is used + * exactly as given, so the configured policy would never actually be consulted while + * {@link FalcoAnvilLoader#unknownEntryPolicy()} kept reporting it as active — the exact trap a + * caller who names both slots would otherwise fall into silently. + */ + @Test + void testCombiningAnExplicitUnknownEntryPolicyWithACustomBlockResolverIsRefused() { + FalcoAnvilLoader.Builder builder = FalcoAnvilLoader.builder() + .unknownEntryPolicy(new DefaultUnknownEntryPolicy()) + .blockResolver(new RefusingResolver()); + + IllegalStateException failure = assertThrows(IllegalStateException.class, + () -> builder.build(this.worldRoot, OVERWORLD)); + assertTrue(failure.getMessage().contains("UnknownEntryPolicy"), failure.getMessage()); + assertTrue(failure.getMessage().contains("blockResolver"), failure.getMessage()); + } + + /** + * The mirror of the block resolver case, and the other slot the guard has to consider: an + * explicit request for classpath discovery combined with a custom biome resolver is refused for + * the same reason a resolved instance is — a resolver supplied through + * {@link Builder#biomeResolver} never sees whatever discovery would have found. + */ + @Test + void testCombiningDiscoverUnknownEntryPolicyWithACustomBiomeResolverIsRefused() { + FalcoAnvilLoader.Builder builder = FalcoAnvilLoader.builder() + .discoverUnknownEntryPolicy() + .biomeResolver(new RefusingResolver()); + + IllegalStateException failure = assertThrows(IllegalStateException.class, + () -> builder.build(this.worldRoot, OVERWORLD)); + assertTrue(failure.getMessage().contains("UnknownEntryPolicy"), failure.getMessage()); + assertTrue(failure.getMessage().contains("biomeResolver"), failure.getMessage()); + } + + /** + * The existing case the guard must not break: a custom resolver on its own, without either + * {@link Builder#unknownEntryPolicy} or {@link Builder#discoverUnknownEntryPolicy} ever being + * called, still builds. {@link #testAGivenBlockResolverIsTheOneTheLoaderDecodesWith(Env)} already + * exercises this shape end to end; this test pins the construction itself so the guard's + * "configured" flag, not merely a null check, is what the refusal above keys on. + */ + @Test + void testACustomBlockResolverWithoutAnyPolicyConfigurationStillBuilds() throws Exception { + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .blockResolver(new RefusingResolver()) + .build(this.worldRoot, OVERWORLD)) { + + assertNotNull(loader); + } + } + @Test void testTheBuilderCanBeReusedAfterASlotChanged() throws Exception { FalcoAnvilLoader.Builder builder = FalcoAnvilLoader.builder().openRegionLimit(8); diff --git a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderIntegrationTest.java b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderIntegrationTest.java index 6a3d097..cfeeb29 100644 --- a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderIntegrationTest.java +++ b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/FalcoAnvilLoaderIntegrationTest.java @@ -784,6 +784,53 @@ void testASectionsKeyStoredAsTheWrongTypeWithLevelIsRefused(Env env) throws Exce } } + @Test + void testAPolicyThatAllowsEverythingLetsALegacyChunkThrough(Env env) throws Exception { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .putString("Status", "minecraft:full") + .build(); + writeRawChunk(11, 11, legacy); + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy((data, minimum) -> { }) + .build(this.worldRoot, OVERWORLD)) { + Instance instance = env.createEmptyInstance(loader); + + assertNotNull(loader.loadChunk(instance, 11, 11)); + } + } + + /** + * Documents the cost of the loader's default: without any {@link ChunkVersionPolicy}, nothing + * checks a chunk's data version at all, so a pre-{@code 21w43a} chunk which stores its data + * under {@code Level} is not refused. It loads exactly as it did before this loader ever gained + * a version guard — as a chunk of air, because {@code sections} is absent from the root. This is + * a deliberate choice of the project, not a regression: a caller has to opt out of the check + * explicitly, with {@code versionPolicy(null)}, to reach this behaviour. + */ + @Test + void testWithoutAnyPolicyALegacyChunkIsNotChecked(Env env) throws Exception { + CompoundBinaryTag legacy = CompoundBinaryTag.builder() + .put("Level", CompoundBinaryTag.builder().put("Sections", ListBinaryTag.empty()).build()) + .putString("Status", "minecraft:full") + .build(); + writeRawChunk(12, 12, legacy); + + try (FalcoAnvilLoader loader = FalcoAnvilLoader.builder() + .versionPolicy(null) + .build(this.worldRoot, OVERWORLD)) { + Instance instance = env.createEmptyInstance(loader); + Chunk chunk = loader.loadChunk(instance, 12, 12); + + assertNotNull(chunk); + // The claim of this test's own javadoc: not merely that a chunk comes back, but that it + // is the chunk of air the legacy Level layout decodes to when sections is absent and no + // policy caught it. + assertEquals(Block.AIR, blockAt(chunk, 0, 40, 0)); + } + } + @Test void testUnloadingAForeignChunkIsIgnored(Env env) throws Exception { Instance instance = env.createEmptyInstance(loader()); diff --git a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ServiceResolutionTest.java b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ServiceResolutionTest.java new file mode 100644 index 0000000..8b1784f --- /dev/null +++ b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/ServiceResolutionTest.java @@ -0,0 +1,176 @@ +package net.onelitefeather.falco.anvil; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins down the resolution rules {@link ServiceResolution} offers to the two extension points built + * on top of it: how a service is discovered on the classpath, and how an explicitly configured + * instance relates to that discovery. + *

+ * The dummy providers this test resolves are registered through the test resources, under + * {@code META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$Dummy} and two more + * files named after {@link DefaultAware} and {@link DefaultAwareWithTwoForeign} below, so the main + * module registers nothing extra. + *

+ * + * @author TheMeinerLP + * @version 1.1.0 + * @since 1.0.0 + */ +class ServiceResolutionTest { + + interface Dummy { + String name(); + } + + public static final class FirstDummy implements Dummy { + public FirstDummy() { + } + + @Override + public String name() { + return "first"; + } + } + + public static final class SecondDummy implements Dummy { + public SecondDummy() { + } + + @Override + public String name() { + return "second"; + } + } + + interface Absent { + } + + /** + * A service with a "shipped default" registered next to a single foreign provider, for the two + * {@link ServiceResolution#discover(Class, Class)} cases below. + */ + interface DefaultAware { + String name(); + } + + public static final class ShippedDefault implements DefaultAware { + public ShippedDefault() { + } + + @Override + public String name() { + return "shipped"; + } + } + + public static final class ForeignProvider implements DefaultAware { + public ForeignProvider() { + } + + @Override + public String name() { + return "foreign"; + } + } + + /** + * A separate service, registered with a shipped default and two foreign providers, so + * that "the default steps aside" and "two foreign providers still refuse each other" can be + * pinned down independently of one another. + */ + interface DefaultAwareWithTwoForeign { + String name(); + } + + public static final class AnotherShippedDefault implements DefaultAwareWithTwoForeign { + public AnotherShippedDefault() { + } + + @Override + public String name() { + return "shipped"; + } + } + + public static final class FirstForeignProvider implements DefaultAwareWithTwoForeign { + public FirstForeignProvider() { + } + + @Override + public String name() { + return "first-foreign"; + } + } + + public static final class SecondForeignProvider implements DefaultAwareWithTwoForeign { + public SecondForeignProvider() { + } + + @Override + public String name() { + return "second-foreign"; + } + } + + @Test + void testAServiceWithNoProviderResolvesToNothing() { + assertNull(ServiceResolution.discover(Absent.class)); + } + + @Test + void testTwoProvidersAreRefusedAndBothAreNamed() { + IllegalStateException failure = + assertThrows(IllegalStateException.class, () -> ServiceResolution.discover(Dummy.class)); + + assertTrue(failure.getMessage().contains("FirstDummy"), failure.getMessage()); + assertTrue(failure.getMessage().contains("SecondDummy"), failure.getMessage()); + } + + @Test + void testAnExplicitInstanceAndDiscoveryTogetherAreRefused() { + Dummy explicit = () -> "explicit"; + + assertThrows(IllegalStateException.class, + () -> ServiceResolution.choose(Dummy.class, explicit, true)); + } + + @Test + void testAnExplicitInstanceIsUsedWithoutTouchingTheClasspath() { + Dummy explicit = () -> "explicit"; + + assertEquals("explicit", ServiceResolution.choose(Dummy.class, explicit, false).name()); + } + + @Test + void testNeitherExplicitNorDiscoveredResolvesToNothing() { + assertNull(ServiceResolution.choose(Dummy.class, null, false)); + } + + @Test + void testAForeignProviderWinsOverTheShippedDefault() { + DefaultAware resolved = ServiceResolution.discover(DefaultAware.class, ShippedDefault.class); + + assertEquals("foreign", resolved.name()); + } + + @Test + void testTwoForeignProvidersAreStillRefusedEvenWithAShippedDefaultRegistered() { + IllegalStateException failure = assertThrows(IllegalStateException.class, + () -> ServiceResolution.discover(DefaultAwareWithTwoForeign.class, AnotherShippedDefault.class)); + + assertTrue(failure.getMessage().contains("FirstForeignProvider"), failure.getMessage()); + assertTrue(failure.getMessage().contains("SecondForeignProvider"), failure.getMessage()); + // The other half of the rule the spec states: the shipped default stepped aside because a + // foreign provider was found, so it is not one of the competing opinions the message lists. + // Naming it here too would send the reader chasing a class that was never actually a + // candidate. + assertFalse(failure.getMessage().contains("AnotherShippedDefault"), failure.getMessage()); + } +} diff --git a/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/UnknownEntryPolicyTest.java b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/UnknownEntryPolicyTest.java new file mode 100644 index 0000000..36538a0 --- /dev/null +++ b/falco-anvil/src/test/java/net/onelitefeather/falco/anvil/UnknownEntryPolicyTest.java @@ -0,0 +1,160 @@ +package net.onelitefeather.falco.anvil; + +import net.kyori.adventure.nbt.CompoundBinaryTag; +import net.minestom.server.world.biome.Biome; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests {@link DefaultUnknownEntryPolicy} directly and pins how {@link BlockPaletteResolver} and + * {@link BiomePaletteResolver} each use whichever {@link UnknownEntryPolicy} they were built with. + *

+ * The counting cases are the ones that matter most: counting an unknown entry is the resolver's job, + * not the policy's, and it has to keep happening even when the policy substitutes instead of + * throwing. Losing it would make a substituting run silent again, which is exactly what this whole + * extension point exists to prevent. + *

+ *

+ * {@link UnknownEntryPolicy} hands back a name, not an id — the resolver owns the registry lookup + * that turns a name into one, using the same registry it already needed for the original, unknown + * name. That is what the "unusable substitute" cases pin: the resolver has to fail if the name a + * policy substitutes is itself unresolvable, and it must not ask the policy a second time to find + * out, which could loop. The failure surfaces as {@link IllegalStateException} rather than {@link + * AnvilChunkException} directly, because {@code FalcoAnvilLoader} is the only class that constructs + * the latter — it wraps this into one when a chunk is read through the loader. + *

+ *

+ * The biome cases use {@link Biome#createDefaultRegistry()} rather than the biome registry of a + * running server, so this class needs no Minestom test environment: {@link BiomePaletteResolver}'s + * package-private three-argument constructor exists for exactly this, to inject both a policy and a + * registry supplier without starting a server. + *

+ * + * @author TheMeinerLP + * @version 2.0.0 + * @since 2.1.0 + */ +class UnknownEntryPolicyTest { + + @Test + void testTheDefaultPolicyReplacesAnUnknownBlockWithAir() { + assertEquals("minecraft:air", new DefaultUnknownEntryPolicy().onUnknownBlock("falco:nope", null)); + } + + @Test + void testTheDefaultPolicyReplacesAnUnknownBiomeWithPlains() { + assertEquals("minecraft:plains", new DefaultUnknownEntryPolicy().onUnknownBiome("falco:nope")); + } + + @Test + void testARefusingPolicyFailsTheChunkInsteadOfSubstituting() { + UnknownEntryPolicy refusing = new UnknownEntryPolicy() { + @Override + public String onUnknownBlock(String name, CompoundBinaryTag properties) { + throw new AnvilChunkException("The block " + name + " has no mapping"); + } + + @Override + public String onUnknownBiome(String name) { + throw new AnvilChunkException("The biome " + name + " has no mapping"); + } + }; + + AnvilChunkException failure = assertThrows(AnvilChunkException.class, + () -> new BlockPaletteResolver(new AnvilDiagnostics(), refusing).toId("falco:nope", null)); + assertTrue(failure.getMessage().contains("falco:nope"), failure.getMessage()); + } + + @Test + void testARefusingPolicyFailsTheChunkInsteadOfSubstitutingForABiome() { + UnknownEntryPolicy refusing = new UnknownEntryPolicy() { + @Override + public String onUnknownBlock(String name, CompoundBinaryTag properties) { + throw new AnvilChunkException("The block " + name + " has no mapping"); + } + + @Override + public String onUnknownBiome(String name) { + throw new AnvilChunkException("The biome " + name + " has no mapping"); + } + }; + + AnvilChunkException failure = assertThrows(AnvilChunkException.class, + () -> new BiomePaletteResolver(new AnvilDiagnostics(), refusing, Biome::createDefaultRegistry) + .toId("falco:nope", null)); + assertTrue(failure.getMessage().contains("falco:nope"), failure.getMessage()); + } + + @Test + void testTheResolverStillCountsWhenThePolicySubstitutes() { + AnvilDiagnostics diagnostics = new AnvilDiagnostics(); + + new BlockPaletteResolver(diagnostics, new DefaultUnknownEntryPolicy()).toId("falco:nope", null); + + assertEquals(1, diagnostics.unknownBlockCount()); + } + + @Test + void testTheResolverStillCountsWhenThePolicySubstitutesForABiome() { + AnvilDiagnostics diagnostics = new AnvilDiagnostics(); + + new BiomePaletteResolver(diagnostics, new DefaultUnknownEntryPolicy(), Biome::createDefaultRegistry) + .toId("falco:nope", null); + + assertEquals(1, diagnostics.unknownBiomeCount()); + } + + @Test + void testAnUnusableSubstituteBlockFailsTheChunkAndNamesBothNames() { + AtomicInteger calls = new AtomicInteger(); + UnknownEntryPolicy nonsense = new UnknownEntryPolicy() { + @Override + public String onUnknownBlock(String name, CompoundBinaryTag properties) { + calls.incrementAndGet(); + return "falco:still-nope"; + } + + @Override + public String onUnknownBiome(String name) { + throw new AssertionError("not exercised by this test"); + } + }; + + IllegalStateException failure = assertThrows(IllegalStateException.class, + () -> new BlockPaletteResolver(new AnvilDiagnostics(), nonsense).toId("falco:nope", null)); + + assertTrue(failure.getMessage().contains("falco:nope"), failure.getMessage()); + assertTrue(failure.getMessage().contains("falco:still-nope"), failure.getMessage()); + assertEquals(1, calls.get(), "the policy is not asked a second time for its own substitute"); + } + + @Test + void testAnUnusableSubstituteBiomeFailsTheChunkAndNamesBothNames() { + AtomicInteger calls = new AtomicInteger(); + UnknownEntryPolicy nonsense = new UnknownEntryPolicy() { + @Override + public String onUnknownBlock(String name, CompoundBinaryTag properties) { + throw new AssertionError("not exercised by this test"); + } + + @Override + public String onUnknownBiome(String name) { + calls.incrementAndGet(); + return "falco:still-nope"; + } + }; + + IllegalStateException failure = assertThrows(IllegalStateException.class, + () -> new BiomePaletteResolver(new AnvilDiagnostics(), nonsense, Biome::createDefaultRegistry) + .toId("falco:nope", null)); + + assertTrue(failure.getMessage().contains("falco:nope"), failure.getMessage()); + assertTrue(failure.getMessage().contains("falco:still-nope"), failure.getMessage()); + assertEquals(1, calls.get(), "the policy is not asked a second time for its own substitute"); + } +} diff --git a/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAware b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAware new file mode 100644 index 0000000..2f4677d --- /dev/null +++ b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAware @@ -0,0 +1,2 @@ +net.onelitefeather.falco.anvil.ServiceResolutionTest$ShippedDefault +net.onelitefeather.falco.anvil.ServiceResolutionTest$ForeignProvider diff --git a/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAwareWithTwoForeign b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAwareWithTwoForeign new file mode 100644 index 0000000..e6193dc --- /dev/null +++ b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$DefaultAwareWithTwoForeign @@ -0,0 +1,3 @@ +net.onelitefeather.falco.anvil.ServiceResolutionTest$AnotherShippedDefault +net.onelitefeather.falco.anvil.ServiceResolutionTest$FirstForeignProvider +net.onelitefeather.falco.anvil.ServiceResolutionTest$SecondForeignProvider diff --git a/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$Dummy b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$Dummy new file mode 100644 index 0000000..a9c2788 --- /dev/null +++ b/falco-anvil/src/test/resources/META-INF/services/net.onelitefeather.falco.anvil.ServiceResolutionTest$Dummy @@ -0,0 +1,2 @@ +net.onelitefeather.falco.anvil.ServiceResolutionTest$FirstDummy +net.onelitefeather.falco.anvil.ServiceResolutionTest$SecondDummy diff --git a/falco-archunit/src/test/java/net/onelitefeather/falco/architecture/ForeignCouplingTest.java b/falco-archunit/src/test/java/net/onelitefeather/falco/architecture/ForeignCouplingTest.java index e7ca831..9430626 100644 --- a/falco-archunit/src/test/java/net/onelitefeather/falco/architecture/ForeignCouplingTest.java +++ b/falco-archunit/src/test/java/net/onelitefeather/falco/architecture/ForeignCouplingTest.java @@ -54,7 +54,9 @@ class ForeignCouplingTest { private static final String ANVIL_NBT_LAYER = "net\\.onelitefeather\\.falco\\.anvil\\." + "(FalcoAnvilLoader|SectionCodec|NbtReads|PaletteEntryResolver" - + "|BlockPaletteResolver|BiomePaletteResolver)(\\$.*)?"; + + "|BlockPaletteResolver|BiomePaletteResolver" + + "|ChunkVersionPolicy|DefaultChunkVersionPolicy" + + "|UnknownEntryPolicy|DefaultUnknownEntryPolicy)(\\$.*)?"; private static final String ANVIL_FILE_BOUNDARY = "net\\.onelitefeather\\.falco\\.anvil\\.(RegionFile|FalcoAnvilLoader)(\\$.*)?"; @@ -251,9 +253,14 @@ class ForeignCouplingTest { * import down here is not untidy, it is the door through which the loss of predictability under * concurrency comes back. * - *

Phrased as a complement rather than a hand-maintained allow list, so it covers - * {@code PaletteData}, {@code AnvilChunkException} and nested types such as - * {@code RegionFile$RawChunk} without maintenance when a class is added. + *

{@code ANVIL_NBT_LAYER} is a hand-maintained list, not a complement: it names the ten + * classes of the NBT layer as of this writing — {@code FalcoAnvilLoader}, + * {@code SectionCodec}, {@code NbtReads}, {@code PaletteEntryResolver}, + * {@code BlockPaletteResolver}, {@code BiomePaletteResolver}, {@code ChunkVersionPolicy}, + * {@code DefaultChunkVersionPolicy}, {@code UnknownEntryPolicy} and + * {@code DefaultUnknownEntryPolicy} — and a class added to that layer later has to be + * added here too, the way the four policy classes were not when they were first written. + * Nested types need no separate entry; they come along through the {@code (\$.*)?} suffix. */ @ArchTest static final ArchRule byteLayerKnowsNoNbt = noClasses()