#169 Fixes for dimensions in legacy loaders

[tpwalke2]

No description provided.

[Copilot]

Pull request overview

This PR hardens legacy sign-marker migration/loading, focusing on dimension key normalization and making the V1/V2 → V3 migration path more robust to partially malformed legacy data so servers don’t end up with duplicated markers or dropped entries.

Changes:

• Expanded V1 dimension normalization to handle canonical resource paths (the_nether/the_end) and namespaced shorthand (minecraft:nether/minecraft:end).
• Added explicit handling for “missing version/data envelope” inputs in the versioned loader to avoid relying on incidental null behavior.
• Switched legacy (V1) and V2 conversion paths to per-entry try/catch so one bad entry doesn’t abort the whole file; added unit tests for these cases.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/VersionedFileSignEntryLoader.java	Adds explicit envelope validation and per-entry safe conversion for V2 files.
src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/Version1SignEntryLoader.java	Improves dimension normalization and skips malformed legacy entries instead of aborting.
src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/VersionedFileSignEntryLoaderTest.java	Adds tests for missing envelope handling and skipping malformed V2 entries.
src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/Version1SignEntryLoaderTest.java	Adds tests for additional dimension normalization cases and malformed-entry skipping.
.scratch/codebase-review-followups/issues/05-harden-legacy-migration-loaders.md	Marks the tracked hardening work as resolved and documents what changed.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/Version1SignEntryLoader.java:59

• In the catch block, entry.key() can throw another NPE if the JSON array contains a null element (or if entry.key() itself is null), which would prevent the loader from skipping malformed entries as intended.

    private static SignEntry loadEntry(SignEntryV2 entry, MarkerGroup[] markerGroups) {
        try {
            return Version3Converter.convertToV3(withNormalizedKey(entry), markerGroups);
        } catch (Exception e) {
            LOGGER.error("Failed to load v1 sign entry, skipping: {}", entry.key(), e);
            return null;
        }

src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/persistence/loaders/VersionedFileSignEntryLoader.java:74

• In the catch block, entry.key() can throw another NPE if the JSON array contains a null element (or if entry.key() itself is null), which would defeat the whole “skip bad entry” hardening and abort the load/migration.

    private static SignEntry convertEntrySafely(SignEntryV2 entry, MarkerGroup[] markerGroups) {
        try {
            return Version3Converter.convertToV3(entry, markerGroups);
        } catch (Exception e) {
            LOGGER.error("Failed to convert v2 sign entry, skipping: {}", entry.key(), e);
            return null;