Details

Reviewers

kamil
inka
tomek
ashoat

Commits

rCOMM1371c52cec71: [keyserver] Introduce convertObject function

Summary

Introdices convertObject function that implements the visitor pattern and uses it to implement sanitizeInput. It will used in the later diffs for implementing the id conversion function.

sanitizeInput should still redact passwords where it previously redacted them but it will now redact them in more places, e.g. lists, non-string optionals. Previously added tests succeed, and I have also added tests fot the new cases.

Test Plan

Run yarn jest

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Apr 18 2023, 8:33 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptApr 18 2023, 8:33 AM

michal added a parent revision: D7487: [keyserver] Add password sanitization tests.Apr 18 2023, 8:34 AM

Harbormaster completed remote builds in B18455: Diff 25273.Apr 18 2023, 8:51 AM

michal requested review of this revision.Apr 18 2023, 8:51 AM

ashoat mentioned this in D7487: [keyserver] Add password sanitization tests.Apr 18 2023, 9:29 AM

ashoat added inline comments.Apr 18 2023, 9:33 AM

keyserver/src/utils/validation-utils.js
22–26 ↗	(On Diff #25273)	Types looks significantly improved!! Wondering if this should be typed with a generic similar to how you typed `sanitizeInput`?
147 ↗	(On Diff #25273)	Why is the `: any` necessary here?
159–160 ↗	(On Diff #25273)	Nit: I think it would be better to do `return (conversionFunction(input): any);` here... `$FlowExpectedError` is meant for unit tests (eg. like this). There is also `$FlowFixMe`, but all of these are basically aliases for `any` Separately – can you talk through what you tried to avoid an `any` type here?

Looks great! (requesting changes because addressing the comments might change things significantly)

keyserver/src/utils/validation-utils.js
201–203 ↗	(On Diff #25273)	Why do we have to use lodash?

This revision now requires changes to proceed.Apr 21 2023, 1:52 AM

Changed mixed to generics, removed lodash _map. Tried to minimize usages of any by using a assertWithValidator function that's a no-op but changes the the data's type after validating it.

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25273)	I'm actually not sure, without it, I'm getting: Cannot instantiate TType because I is not a valid argument of TStructProps I suspect this fixes it because `: any` doesn't allow for the `empty` type. But I've tried to make a minimal repro with but I couldn't get it to fail with `empty` so I'm not sure.

Harbormaster completed remote builds in B18620: Diff 25510.Apr 21 2023, 4:05 AM

ashoat added inline comments.Apr 21 2023, 6:56 AM

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25510)	I think we should try to figure this out. I'll try to make some time to take a look at this later today
lib/utils/validation-utils.js
96 ↗	(On Diff #25510)	I think this is a good usage of `any`. We're doing runtime type validation on a type we don't understand; after validating, we have to assert statically that it is of the type that is passed to us. Furthermore, we use `any` in a "safe" way that won't affect other types, by immediately casting the result to `T`

michal added a child revision: D7564: [lib] Move `messageTypes` enum to a separate file.Apr 21 2023, 7:57 AM

ashoat added inline comments.Apr 21 2023, 11:33 AM

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25510)	I looked at this closer. I agree there's no way to avoid using `any` here... the core issue is that every type in the `TType` union needs to support its type parameter, but in the `TInterface` case we use `$ObjMap` on the type parameter, which only works on objects. It's possible this will work with the upcoming conditional type supporting coming to Flow. I did try this with my `$If` hack, but I wasn't able to get it working quickly. Instead, I figure it would be better to "constrain" our use of `any`. The best way to use `any` is to cast something to `any` and then immediately cast it to something else, so that the `any` type doesn't cross any boundaries and affect something we don't want it to affect. I think this would be slightly better: https://gist.github.com/Ashoat/45893e511ab9e860e351d08307ec3ef7
159–161 ↗	(On Diff #25510)	Are `TValidator` and `validator` the same? If so, it's confusing that you use both of them here. Should you use `typesToConvert.includes(validator)` and then use `validator` here?

tomek accepted this revision.Apr 25 2023, 2:58 AM

tomek added a reviewer: ashoat.

See my previous review

This revision now requires changes to proceed.Apr 25 2023, 4:21 AM

Introduce changes from the diff in the gist

keyserver/src/utils/validation-utils.js
159–161 ↗	(On Diff #25510)	`validator` is `TType<I>` and `TValidator` is `TType<T>` which allows the `assertWithValidator` to change the type of the `input`. I could change it to just cast `const recastValidator: TType<T> = (validator: any);` if you think that's better.