Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODEBASE: Recheck all usages of typecasting with JSON.parse #1775

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

catloversg
Copy link
Contributor

While we search and fix lint errors, we (kind of) agree that we should not typecast the result of JSON.parse (e.g., JSON.parse(saveString, Reviver) as Something. This PR searches all references of JSON.parse and checks if they are typecasting in a questionable way. My guideline:

  • It's fine to typecast to unknown.
  • It's fine to typecast after or immediately before the validation code.
  • Compromises:
    • If invalid data comes from a third-party tool (RFA client).
    • If proper validation requires an overcomplicated and risky refactor (the god object PlayerObject).

@catloversg
Copy link
Contributor Author

In the latest commit, I refactored loadStaneksGift. There is a behavior change:

  • Old: If saveString is an empty string, we load default data; otherwise, we parse saveString and pretend that everything is okay. If there is an error, the caller (loadGame) will handle it with a try-catch.
  • New: Always pass saveString to JSON.parse in a try-catch. Validate the result. If it's invalid, we load default data, then show a dialog.

Rationale:

  • BN13 was added in 793d9b3 - v0.54.0 - 26 Sep 2021. saveString can only be invalid (empty string or invalid data) if the save file is older than v0.54.0 or it's a badly edited one.
  • Proper validation.
  • When the save string is invalid, instead of silently reset data, we show a dialog to warn the player. This is not a problem, unless they try to load an ancient/invalid save file.

From now on, I'll use this code for other "loader" functions if I need to change them (I'm testing another PR that changes loadAllGangs):

let somethingData: unknown;
try {
  somethingData = JSON.parse(saveString, Reviver);
} catch (error) {
  console.error(error);
}
if (!validate(somethingData)) {
  console.error("Invalid somethingData:", saveString);
  somethingData = DefaultData;
  setTimeout(() => {
    dialogBoxCreate("Cannot load data of something. Something is reset.");
  }, 1000);
  return;
}
something = somethingData;

After #1774 is merged, with proper "loader" functions, we can gradually deal with legacy code in loadGame in a safe way.

Copy link
Collaborator

@d0sboots d0sboots left a comment

Choose a reason for hiding this comment

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

Note to self: Have to test this on the old saves too

src/CotMG/Helper.tsx Show resolved Hide resolved
src/SaveObject.ts Outdated Show resolved Hide resolved
test/jest/FullSave.test.ts Outdated Show resolved Hide resolved
@catloversg
Copy link
Contributor Author

Unrelated question:
We have src\utils\helpers\typeAssertion.ts and src\utils\TypeAssertion.ts. Can I merge them later in another PR (src\utils\helpers\typeAssertion.ts -> src\utils\TypeAssertion.ts)?

@d0sboots
Copy link
Collaborator

Unrelated question: We have src\utils\helpers\typeAssertion.ts and src\utils\TypeAssertion.ts. Can I merge them later in another PR (src\utils\helpers\typeAssertion.ts -> src\utils\TypeAssertion.ts)?

Please do, that's just silly. Unless there are circular dependency issues, but those should be solved by breaking individual functions out, not having two of the same file.

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.

2 participants