[Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders.js
ClosedPublic
Actions

Authored by ashoat on Nov 13 2023, 3:18 PM.

Details

Reviewers

bartek
kamil

Commits

rCOMM21e960b9d732: [Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders.

Summary

This one ended up being much more involved that I initially hoped. The main changes here are:

Probably reflect that the id field should only be required on the client. It should be optional on the keyserver, which needs to continue supporting older clients.
Update the validators to only consider fields included by supported client versions.

NOTE: CI will fail on this diff. I considered the possibility of fixing Flow errors BEFORE upgrading Flow, but it wasn't possible... in some cases, the fixes to support the new version of Flow caused errors in the old version. I could have hidden these type errors with $FlowFixMe lines and then later revert those, but that seemed like too much busy work.

Depends on D9862

Test Plan

Confirm the Flow errors go away

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Nov 13 2023, 3:18 PM

Herald added a subscriber: tomek. · View Herald TranscriptNov 13 2023, 3:18 PM

ashoat added a child revision: D9864: [Flow202][keyserver][skip-ci] [15/x] Fix Flow issues with types being underconstrained.Nov 13 2023, 3:19 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 13 2023, 3:29 PM

Harbormaster failed remote builds in B24080: Diff 33179!

Would appreciate a close review by @kamil

keyserver/src/responders/report-responders.js
40–43 ↗	(On Diff #33179)	I had to add this to satisfy Flow, but the runtime refinement is not something that's possible today, so I skipped that part
83 ↗	(On Diff #33179)	@kamil added the `id` field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver? A bit worried since I'm guessing @kamil tested this, so I must be missing something
93 ↗	(On Diff #33179)	@kamil added the `id` field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver? A bit worried since I'm guessing @kamil tested this, so I must be missing something
110 ↗	(On Diff #33179)	@kamil added the `id` field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver? A bit worried since I'm guessing @kamil tested this, so I must be missing something
112–118 ↗	(On Diff #33179)	The changes to `type`, `platformDetails`, `deviceType`, `codeVersion`, and `stateVersion` occurred in 2018 in this commit. We no longer support clients from 2018. As such, I'm updating the validators to no longer have to worry about that. This lets me skip creating a separate Flow type for this.
120 ↗	(On Diff #33179)	@kamil added the `id` field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver? A bit worried since I'm guessing @kamil tested this, so I must be missing something
128 ↗	(On Diff #33179)	The requirement to support strings was first introduced in April of 2020, and was subsequently deprecated in May of 2020. We no longer support clients from then (our current minimum supported client is from Jan 2022)
141 ↗	(On Diff #33179)	@kamil added the `id` field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver? A bit worried since I'm guessing @kamil tested this, so I must be missing something
163–184 ↗	(On Diff #33179)	After updating `reportCreationRequestInputValidator` above, we can use it directly here
lib/types/report-types.js
109–136 ↗	(On Diff #33179)	Same question to @kamil as above – it's not clear to me where `id` gets added in the stack, and I figured it should be added here too

ashoat requested review of this revision.Nov 14 2023, 6:17 AM

kamil mentioned this in D9893: [Flow] improve report related types.Nov 15 2023, 4:07 AM

kamil mentioned this in rCOMMb9ee04633890: [Flow] improve report related types.Nov 15 2023, 5:21 AM

@kamil added the id field in D7909 / D7916, but did not update these validators. Am I getting it wrong here – won't these params be passed to the keyserver?

A bit worried since I'm guessing @kamil tested this, so I must be missing something

Yes, id should never be passed to keyserver. Fixed and described in D9893 (which is similar to this diff but wanted to write up what was going on).

I'll request changes to put it back in your queue to remove id from keyserver types validators and rebase on D9893, but the rest of the code looks good.

lib/types/report-types.js
109–136 ↗	(On Diff #33179)	`id` should be removed here, as described here: D9893

This revision now requires changes to proceed.Nov 15 2023, 5:24 AM

Rebase and address @kamil's feedback

ashoat added inline comments.Nov 15 2023, 6:52 AM

keyserver/src/responders/report-responders.js
40–43	I had to add this to satisfy Flow, but the runtime refinement is not something that's possible today, so I skipped that part
112–118	The changes to `type`, `platformDetails`, `deviceType`, `codeVersion`, and `stateVersion` occurred in 2018 in this commit. We no longer support clients from 2018. As such, I'm updating the validators to no longer have to worry about that. This lets me skip creating a separate Flow type for this.
128	The requirement to support strings was first introduced in April of 2020, and was subsequently deprecated in May of 2020. We no longer support clients from then (our current minimum supported client is from Jan 2022)
164–185	After updating `reportCreationRequestInputValidator` above, we can use it directly here

ashoat added inline comments.Nov 15 2023, 6:54 AM

lib/shared/state-sync/users-state-sync-spec.js
25	Not sure, but I think these changes were missed by @kamil. Let me know if I'm wrong!

Harbormaster failed remote builds in B24150: Diff 33264!Nov 15 2023, 7:01 AM

kamil accepted this revision.Nov 15 2023, 10:27 AM

kamil added inline comments.

lib/shared/state-sync/users-state-sync-spec.js
25	Maybe it's some minor mistake while resolving conflict changes? I see it on the newest master link

This revision is now accepted and ready to land.Nov 15 2023, 10:27 AM

ashoat added inline comments.Nov 15 2023, 12:17 PM

lib/shared/state-sync/users-state-sync-spec.js
25	Actually the line you linked corresponds to line 40 here. But you're right that it's a rebase issue. What happened here is that I changed this file in D9709, but Git didn't detect any conflicts while rebasing. In fact I should go back and update D9709 to use the updated type.

ashoat added inline comments.Nov 15 2023, 12:18 PM

lib/shared/state-sync/users-state-sync-spec.js
25	Also D9706 needs to be updated

Rebase after addressing users-state-sync-spec.js changes in earlier diffs in the stack (where it's relevant)

Harbormaster failed remote builds in B24180: Diff 33298!Nov 15 2023, 12:41 PM

ashoat retitled this revision from [Flow202][keyserver] [14/x] Address type errors with report-responders.js to [Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders.js.Nov 19 2023, 5:07 PM

This revision was landed with ongoing or failed builds.Nov 27 2023, 3:27 PM

Closed by commit rCOMM21e960b9d732: [Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders. (authored by ashoat). · Explain Why

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMM21e960b9d732: [Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders..