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

Validate data['id'] field early in dev #238

Closed
wants to merge 4 commits into from

Conversation

LucDeCaf
Copy link
Contributor

@LucDeCaf LucDeCaf commented Feb 2, 2025

Implements change proposed in #137.

Copy link
Contributor

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

In my opinion, we should add explicit casts to all our JSON-deserializing constructors (and also enable strict casts to check this).
dart2js with -O3 or -O4 can omit implicit casts such as the ones in our fromRow methods, which leads to unsound code when the type in the JSON value is not what we expect (it leads to num values creeping into String fields, possibly breaking type safety everywhere).
The general advice is to not compile with -O3 without carefully checking for implicit casts, but:

  1. We compile our web workers with -O4.
  2. All Flutter web apps are compiled with -O4 (a very dangerous decision by Flutter, but we have to deal with that).

So ideally, CrudEntry.fromRow should look like this:

final data = jsonDecode(row['data'] as String);
return CrudEntry(
  row['id'] as int,
  UpdateType.fromJsonChecked(data['op'] as String),
  data['type'] as String,
  data['id'] as String,
  row['tx_id'] a String,
  data['data'],
);

There are a few methods affected by this which should all be fixed. That will be a bigger change, so you can leave that to us if you don't want to patch them all :D

@LucDeCaf LucDeCaf closed this Feb 3, 2025
@LucDeCaf LucDeCaf deleted the feat/validate-id-field branch February 3, 2025 07:58
@LucDeCaf
Copy link
Contributor Author

LucDeCaf commented Feb 3, 2025

@simolus3 Thank you for the insight! I'm renaming the branch to reflect the new changes so expect a new PR soon ;)

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