Skip to content

Fail gracefully when data sources are missing or malformed#7645

Open
Vest wants to merge 2 commits into
PCGen:masterfrom
Vest:fix-graceful-data-load-errors
Open

Fail gracefully when data sources are missing or malformed#7645
Vest wants to merge 2 commits into
PCGen:masterfrom
Vest:fix-graceful-data-load-errors

Conversation

@Vest

@Vest Vest commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Problem

Loading a character against a partially-loaded or malformed data set produces obscure NPEs deep in facet wiring. Three real-world failure modes I hit while loading an existing .pcg whose campaigns weren't installed:

  1. NPE in SizeFacet.calcRacialSizeInt:118 — when no SizeAdjustment is loaded with ISDEFAULTSIZE:YES, the facet dereferences a null deep in the constructor's facet chain (20+ frames).
  2. NoSuchElementException in LstObjectFileLoader.loadLstFile:284 — when a .lst isn't valid UTF-8, LstFileLoader.readFromURI correctly logs SEVERE and returns Optional.empty(), but the caller's blind .get() aborts the entire source load.
  3. NPE on dataset.getCampaigns() in CharacterManager.openPcInternal:173 — when the source load aborted (per CODE-2789 #2), the PCGenFrame lambda still calls openCharacter with a null DataSetFacade, and the dereference happens outside the existing try/catch.

Fix

Three NPE → actionable error replacements (commit 1):

  • PlayerCharacter.<init> fails fast with IllegalStateException when getDefaultSizeAdjustment() returns null. The constructor logs a SEVERE diagnostic with concrete guidance on where size definitions live (*__sizes.lst in the gamemode's Core Rules data source, e.g. data/pathfinder/.../core_essentials/ce__sizes.lst), then throws. The throw is caught by CharacterManager's existing handler and surfaced as a localized dialog. No more 23-frame stack trace.
  • LstObjectFileLoader.loadLstFile now respects the Optional contract: if readFromURI returns empty (because it logged and skipped a non-UTF-8 / IO-failed file), the loader continues with the next file instead of aborting.
  • CharacterManager.openPcInternal and createNewCharacter null-guard their DataSetFacade parameter before dereferencing it, logging and showing a localized "data sources failed to load - see earlier errors" dialog.

One diagnostic upgrade (commit 2):

  • Globals.getCampaignKeyed now tells the user what failed (no .pcc declares CAMPAIGN:<key>) and where to install it (the vendor data dir or homebrew data dir paths — printed concretely).

Before / After

Before, loading a .pcg whose campaigns weren't installed produced:

SEVERE  Globals:130  Could not find campaign: Dungeons & Dragons - Core Books
... (×14, no further information)
SEVERE  CharacterManager:208 Unable to load character /path/to/Alejandro.pcg
java.lang.NullPointerException: Cannot invoke "pcgen.core.SizeAdjustment.get(...)"
because the return value of "...getDefaultSizeAdjustment()" is null
    at pcgen.cdom.facet.model.SizeFacet.calcRacialSizeInt(SizeFacet.java:118)
    ... 22 more frames ...

After, the same .pcg produces:

SEVERE  Globals:130  Could not find campaign: Dungeons & Dragons - Core Books - no
    loaded .pcc file declares CAMPAIGN:Dungeons & Dragons - Core Books. If this
    is third-party (non-OGL) data, install the .pcc into the vendor data dir
    (/Users/.../vendordata) or homebrew data dir (/Users/.../homebrewdata).
SEVERE  PlayerCharacter:552  Game mode '35e' has no default size: no SizeAdjustment
    was loaded with ISDEFAULTSIZE:YES. Sizes are typically defined in a *__sizes.lst
    file inside the gamemode's Core Rules data source (e.g.
    data/pathfinder/.../core_essentials/ce__sizes.lst). Exactly one size in that
    file must be flagged ISDEFAULTSIZE:YES.

Dialog: "Game mode '35e' has no default size. Check earlier log errors for the missing data source."

After installing the missing campaigns, the same character loads successfully.

Risk

Low. None of the four files have behavior changes on the happy path:

  • Globals.getCampaignKeyed — log-message-only change, still returns null for misses.
  • LstObjectFileLoader — only the empty-Optional branch changes (previously threw, now skips file; the file was already skipped upstream in readFromURI).
  • CharacterManager — only the dataset == null branch is new.
  • PlayerCharacter — only fires when getDefaultSizeAdjustment() returns null, which is itself a "broken-data" state the engine cannot proceed against.

./gradlew :test --tests "pcgen.cdom.facet.model.SizeFacetTest" passes (happy-path coverage of the modified file).

Notes

  • The fail-fast at PlayerCharacter.<init> is the primary fix. The other three changes are defense-in-depth + diagnostic polish that surfaced while tracking down the same end-user symptom.
  • No new behavior on existing tests; no ISDEFAULTSIZE semantics changed.
  • Five other call sites of getDefaultSizeAdjustment() still deref unguarded (SizeFacet.racialSizeInt:98, UnarmedDamageFacet.getUDamForRace:116, PCClass:770, ObjectKey.BASESIZE/SIZE.getDefault, FormulaKey.SIZE.getDefault). The fail-fast in PlayerCharacter.<init> makes them unreachable from a valid PC, but tightening them is a reasonable follow-up.

Vest added 2 commits June 27, 2026 00:30
Three NPE crashes when loading a character against a partially-loaded
or malformed data set are replaced with clear, actionable errors:

LstObjectFileLoader.loadLstFile no longer calls .get() blindly on the
Optional returned by LstFileLoader.readFromURI. readFromURI already
logs and returns Optional.empty() for non-fatal IO/decode errors (e.g.
a non-UTF-8 .lst), but the caller's blind .get() turned that into a
NoSuchElementException that aborted the entire source load. The file
is now skipped, the upstream log line remains, and loading continues.

CharacterManager.openPcInternal and createNewCharacter now null-check
the DataSetFacade parameter before dereferencing it. When the source
load aborts, callers in PCGenFrame previously passed the unpopulated
ref through, producing an NPE on dataset.getCampaigns() outside the
existing catch block. The methods now log and surface a localised
dialog instead.

PlayerCharacter's Collection<Campaign> constructor now fails fast when
SizeUtilities.getDefaultSizeAdjustment() returns null. The previous
behaviour was to NPE 20+ frames deep in the SizeFacet wiring while
loading the unselected race; users got a stack trace with no hint at
the actual cause. The fix logs the missing-default-size diagnostic
once at SEVERE (with concrete guidance on where size definitions
live) and throws IllegalStateException, caught and shown as a dialog
by CharacterManager.
Globals.getCampaignKeyed previously logged just the missing campaign
key, leaving users with no hint that the campaign needed to be
installed somewhere or where that 'somewhere' was. The message now
names the lookup mechanism (looking for a .pcc file declaring
CAMPAIGN:<key>) and points at the vendor and homebrew data dirs as
the standard drop-in locations for third-party (non-OGL) data.
aString = LstFileLoader.readFromURI(uri).get();
// readFromURI returns Optional.empty() when it logs a non-fatal IO/decode error
// (e.g. a non-UTF-8 .lst). Skip the file rather than aborting the whole load.
Optional<String> read = LstFileLoader.readFromURI(uri);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@karianna, I gave a chance to @Nylanfs to load files that are non-UTF-8.
Previously, the loading failed, and the logs wrote that a non-UTF-8 file was detected (from BahamutDragon/pcgen repo).
Now we load the campaign, but it can be barely playable, because not all files were in a proper codepage.

But if you prefer us to be stricter (I like it, actually), I can revert this part. I don't want to have a situation, where PCGen has to assume a lot, when a human can correct an error...

@Vest

Vest commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@Nylanfs, your character should work with the nightly PCGen, but @karianna must approve this PR first.
p.s. please fix Bahamut's repo as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant