[Flow] improve report related types
ClosedPublic
Actions

Authored by kamil on Nov 15 2023, 4:07 AM.

Details

Reviewers

ashoat
bartek

Commits

rCOMMb9ee04633890: [Flow] improve report related types

Summary

D7916 introduced id field on the client side to identify reports and avoid comparing the entire blob each time - this is different from keyserver id assigned by the backend database.

It was properly added for ClientThreadInconsistencyReportCreationRequest and ClientEntryInconsistencyReportCreationRequest which are only ClientReportCreationRequest.

By mistake, it was also added for ErrorReportCreationRequest, MediaMissionReportCreationRequest and UserInconsistencyReportCreationRequest types which are used as both ClientReportCreationRequest and ReportCreationRequest.

That caused diverging types, invalid validators (keyserver validators were implemented without the id field) and a lot of confusion pointed out by @ashoat here:

This shouldn't cause any issues for older clients, the id field is extracted in the current code version, and also was in the past (D7910).

The main issue is that validators were not reflected in types, this diff should fix this.

Test Plan

FLow.
Make sure validators are properly defined (without id field).
report-store-reducer.test.js and report-store-ops.test.js.
Try sending some reports and make sure it succeeded..

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 15 2023, 4:07 AM

kamil held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptNov 15 2023, 4:07 AM

kamil added 1 blocking reviewer(s): ashoat.Nov 15 2023, 4:21 AM

kamil added inline comments.

lib/utils/reports-service.js
20–32 ↗	(On Diff #33253)	adding this explicitly, to make it more clear

kamil published this revision for review.Nov 15 2023, 4:22 AM

Harbormaster completed remote builds in B24142: Diff 33253.Nov 15 2023, 4:26 AM

Seems reasonable to me. I haven't audited all places on the client where reports are constructed – hoping you went through all of those cases carefully

native/input/input-state-container.react.js
157 ↗	(On Diff #33253)	Looks like your IDE accidentally added this

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

remove blank line

In D9893#288366, @ashoat wrote:

Seems reasonable to me. I haven't audited all places on the client where reports are constructed – hoping you went through all of those cases carefully

Yes, I made sure that ReportCreationRequest types are used only on keyserver, and on the client we use those with Client* prefix

Harbormaster completed remote builds in B24143: Diff 33254.Nov 15 2023, 5:17 AM

Closed by commit rCOMMb9ee04633890: [Flow] improve report related types (authored by kamil). · Explain WhyNov 15 2023, 5:21 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMMb9ee04633890: [Flow] improve report related types.

kamil mentioned this in D9863: [Flow202][keyserver][skip-ci] [14/x] Address type errors with report-responders.js.Nov 15 2023, 5:24 AM