Fix handling of empty builds.json file #3733
Merged
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
If your
%LOCALAPPDATA%\CKAN\builds.json
file is empty (zero bytes), CKAN throws an exception at startup:Originally reported by Discord user Krhank, and probably confirmed by Discord user Netuno.
Cause
KspBuildMap.TrySetBuildMap
is supposed to returntrue
if it successfully sets the build map andfalse
if it fails, so the calling code can fall back to other sources, and it assumes thatJsonConvert.DeserializeObject
will throw an exception if there are any problems parsing the input:CKAN/Core/Games/KerbalSpaceProgram/GameVersionProviders/KspBuildMap.cs
Lines 101 to 114 in fa6f9d6
However, if
%LOCALAPPDATA%\CKAN\builds.json
is empty, thenbuildMapJson
will be the empty string, which parses tonull
without throwing an exception!KspBuildMap.TrySetBuildMap
then returnstrue
even though_jBuilds
isnull
. This preventsKspBuildMap
from falling back to other sources like the embedded build map, and any subsequent attempt to access the build map will throw null reference exceptions. (We've had this break things before in other places where we parse JSON.)(Caching of this data in a local file was new in #3624, but the parsing logic had this flaw before that.)
Changes
Now
KspBuildMap.TrySetBuildMap
only returns true if_jBuilds
is non-null. This way a null value will be detected as a failure, allowing fallback sources to be checked.We also now only save the file to disk if the parse succeeds, so empty files should be much less likely.