Fail gracefully when data sources are missing or malformed#7645
Open
Vest wants to merge 2 commits into
Open
Conversation
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.
Vest
commented
Jun 26, 2026
| 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); |
Contributor
Author
There was a problem hiding this comment.
@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...
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
.pcgwhose campaigns weren't installed:SizeFacet.calcRacialSizeInt:118— when noSizeAdjustmentis loaded withISDEFAULTSIZE:YES, the facet dereferences anulldeep in the constructor's facet chain (20+ frames).NoSuchElementExceptioninLstObjectFileLoader.loadLstFile:284— when a.lstisn't valid UTF-8,LstFileLoader.readFromURIcorrectly logs SEVERE and returnsOptional.empty(), but the caller's blind.get()aborts the entire source load.dataset.getCampaigns()inCharacterManager.openPcInternal:173— when the source load aborted (per CODE-2789 #2), the PCGenFrame lambda still callsopenCharacterwith a nullDataSetFacade, and the dereference happens outside the existingtry/catch.Fix
Three NPE → actionable error replacements (commit 1):
PlayerCharacter.<init>fails fast withIllegalStateExceptionwhengetDefaultSizeAdjustment()returns null. The constructor logs a SEVERE diagnostic with concrete guidance on where size definitions live (*__sizes.lstin the gamemode's Core Rules data source, e.g.data/pathfinder/.../core_essentials/ce__sizes.lst), then throws. The throw is caught byCharacterManager's existing handler and surfaced as a localized dialog. No more 23-frame stack trace.LstObjectFileLoader.loadLstFilenow respects theOptionalcontract: ifreadFromURIreturns empty (because it logged and skipped a non-UTF-8 / IO-failed file), the loader continues with the next file instead of aborting.CharacterManager.openPcInternalandcreateNewCharacternull-guard theirDataSetFacadeparameter before dereferencing it, logging and showing a localized "data sources failed to load - see earlier errors" dialog.One diagnostic upgrade (commit 2):
Globals.getCampaignKeyednow tells the user what failed (no.pccdeclaresCAMPAIGN:<key>) and where to install it (the vendor data dir or homebrew data dir paths — printed concretely).Before / After
Before, loading a
.pcgwhose campaigns weren't installed produced:After, the same
.pcgproduces: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 returnsnullfor misses.LstObjectFileLoader— only the empty-Optional branch changes (previously threw, now skips file; the file was already skipped upstream inreadFromURI).CharacterManager— only thedataset == nullbranch is new.PlayerCharacter— only fires whengetDefaultSizeAdjustment()returnsnull, 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
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.ISDEFAULTSIZEsemantics changed.getDefaultSizeAdjustment()still deref unguarded (SizeFacet.racialSizeInt:98,UnarmedDamageFacet.getUDamForRace:116,PCClass:770,ObjectKey.BASESIZE/SIZE.getDefault,FormulaKey.SIZE.getDefault). The fail-fast inPlayerCharacter.<init>makes them unreachable from a valid PC, but tightening them is a reasonable follow-up.