Page MenuHomePhabricator
Differential D7808

[keyserver] Convert upload responders
ClosedPublic
Actions

Authored by michal on May 15 2023, 3:57 AM.
Tags
None
Referenced Files
F5120690: D7808.diff
Thu, Apr 3, 8:31 AM
Unknown Object (File)
Wed, Apr 2, 3:21 AM
Unknown Object (File)
Tue, Apr 1, 8:17 AM
Unknown Object (File)
Fri, Mar 21, 8:36 AM
Unknown Object (File)
Mon, Mar 17, 12:55 PM
Unknown Object (File)
Mon, Mar 17, 12:55 PM
Unknown Object (File)
Mon, Mar 17, 12:55 PM
Unknown Object (File)
Mon, Mar 17, 12:54 PM
View All Files
Subscribers
ashoat
atul

Details

Reviewers
kamil
tomek
Commits
rCOMMc095237c8405: [keyserver] Convert upload responders
Summary

Adds validators for upload responders.

Depends on D7807

Test Plan

Try sending a photo and a video, check if there are any errors.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

michal created this revision.May 15 2023, 3:57 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptMay 15 2023, 3:57 AM
Harbormaster completed remote builds in B19334: Diff 26448.May 15 2023, 4:15 AM
michal requested review of this revision.May 15 2023, 4:15 AM
tomek accepted this revision.May 16 2023, 3:32 AM
tomek added inline comments.
keyserver/src/uploads/uploads.js
223 ↗(On Diff #26448)

Why the original type was concrete instead of any / mixed? Do we have to replace it with mixed?

This revision is now accepted and ready to land.May 16 2023, 3:32 AM
michal added inline comments.May 16 2023, 9:47 AM
keyserver/src/uploads/uploads.js
223 ↗(On Diff #26448)

@tomek Not sure, I think it was just a mistake, I can't find any additional input validation for this responder in particular.
I also tested it just now and was able to send anything to this responder and it only tripped on the newly added validateInput.

michal added a child revision: D7848: [keyserver] Validate fetchPrimaryInviteLinksResponder output.May 17 2023, 2:31 AM
michal updated this revision to Diff 26592.May 17 2023, 3:42 AM

Rebase, validate uploadMediaMetadataResponder, fix bug with uploadDownloadResponder

michal requested review of this revision.May 17 2023, 3:50 AM
michal added inline comments.
keyserver/src/uploads/uploads.js
158

These are not converted because they come from the upload URI. We could convert them here but then on the output side we would need to convert them by adding a new tURI and then have the conversion function parse the URL and convert only the id inside it but I don't think that's needed.
These URIs already have the Comm keyserver hardcoded, so it will require even more changes anyway when we are adding multi keyserver support. And if in the future all uploads will be handled by blob service, this endpoint won't be even needed.

Harbormaster completed remote builds in B19442: Diff 26592.May 17 2023, 4:12 AM
tomek accepted this revision.May 19 2023, 12:58 AM
This revision is now accepted and ready to land.May 19 2023, 12:58 AM
Closed by commit rCOMMc095237c8405: [keyserver] Convert upload responders (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMay 22 2023, 4:32 AM
This revision was automatically updated to reflect the committed changes.
Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMMc095237c8405: [keyserver] Convert upload responders.

Revision Contents
Changeset List

PathSize
keyserver/
src/
uploads/
59 lines
lib/
types/
9 lines

Diff 26592

View Options

keyserver/src/uploads/uploads.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

lib/types/media-types.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };