Page MenuHomePhabricator
Differential D7488

[keyserver] Introduce convertObject function
ClosedPublic
Actions

Authored by michal on Apr 18 2023, 8:33 AM.
Tags
None
Referenced Files
F5226217: D7488.id25812.diff
Sat, Apr 5, 3:56 AM
Unknown Object (File)
Sun, Mar 30, 9:04 PM
Unknown Object (File)
Thu, Mar 27, 3:09 AM
Unknown Object (File)
Tue, Mar 25, 9:44 AM
Unknown Object (File)
Mon, Mar 17, 1:59 PM
Unknown Object (File)
Mon, Mar 17, 1:59 PM
Unknown Object (File)
Mon, Mar 17, 1:59 PM
Unknown Object (File)
Mon, Mar 17, 1:59 PM
View All Files
Subscribers
ashoat
atul

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

Types looks significantly improved!!

Wondering if this should be typed with a generic similar to how you typed sanitizeInput?

147

Why is the : any necessary here?

159–160

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?

tomek requested changes to this revision.Apr 21 2023, 1:52 AM

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

keyserver/src/utils/validation-utils.js
201–203

Why do we have to use lodash?

This revision now requires changes to proceed.Apr 21 2023, 1:52 AM
michal updated this revision to Diff 25510.Apr 21 2023, 3:48 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

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.
ashoat requested changes to this revision.Apr 25 2023, 4:21 AM

See my previous review

This revision now requires changes to proceed.Apr 25 2023, 4:21 AM
michal updated this revision to Diff 25679.Apr 25 2023, 7:52 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.

Harbormaster completed remote builds in B18741: Diff 25679.Apr 25 2023, 8:09 AM
ashoat accepted this revision.Apr 25 2023, 10:21 AM
ashoat added inline comments.
keyserver/src/utils/validation-utils.js
159–161 ↗(On Diff #25510)

Thanks for explaining. I think the best thing would be to add a code comment explaining what we're doing here

This revision is now accepted and ready to land.Apr 25 2023, 10:21 AM
michal updated this revision to Diff 25812.Apr 27 2023, 4:14 AM

Add a comment

Harbormaster completed remote builds in B18843: Diff 25812.Apr 27 2023, 4:32 AM
michal updated this revision to Diff 26053.May 4 2023, 3:04 AM

Rebase

Harbormaster completed remote builds in B19022: Diff 26053.May 4 2023, 3:21 AM
Closed by commit rCOMM1371c52cec71: [keyserver] Introduce convertObject function (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMay 5 2023, 5:46 AM
This revision was automatically updated to reflect the committed changes.
Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM1371c52cec71: [keyserver] Introduce convertObject function.
michal mentioned this in D8355: [lib] Generate converters from validators.Jul 4 2023, 7:20 AM
inka mentioned this in D12199: [lib] Fix invite links validation.May 23 2024, 6:04 AM
ashoat mentioned this in D12387: [keyserver] Improve logging in checkInputValidator.Jun 11 2024, 6:41 AM

Revision Contents
Changeset List

PathSize
keyserver/
src/
utils/
130 lines
47 lines

Diff 25273

View Options

keyserver/src/utils/validation-utils.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

keyserver/src/utils/validation-utils.test.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes