Page MenuHomePhabricator
Differential D3803

[services] Backup/Blob - Add validate method to server write reactor base
ClosedPublic
Actions

Authored by • karol on Apr 21 2022, 2:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 11, 11:39 AM
Unknown Object (File)
Fri, Apr 11, 11:39 AM
Unknown Object (File)
Tue, Apr 8, 8:12 AM
Unknown Object (File)
Tue, Apr 8, 6:44 AM
Unknown Object (File)
Tue, Apr 8, 5:10 AM
Unknown Object (File)
Sun, Apr 6, 8:34 PM
Unknown Object (File)
Sun, Apr 6, 8:31 PM
Unknown Object (File)
Sun, Apr 6, 8:27 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
Commits
rCOMM5aefcb036f32: [services] Backup/Blob - Add validate method to server write reactor base
Summary

Depends on D3721

Adding the validate method to the server write reactor base

Test Plan
cd services
yarn run-backup-service
yarn run-blob-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Apr 21 2022, 2:23 AM
Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 21 2022, 2:23 AM
• karol added a child revision: D3804: [services] Backup/Blob - Use this when reffering status.Apr 21 2022, 2:26 AM
• karol edited the summary of this revision. (Show Details)
• karol edited the test plan for this revision. (Show Details)
• karol added a reviewer: tomek.
• karol added a parent revision: D3721: [services] Backup/Blob - wrap terminateCallback into try...catch.
• karol mentioned this in D3721: [services] Backup/Blob - wrap terminateCallback into try...catch.Apr 21 2022, 2:28 AM
Harbormaster completed remote builds in B8392: Diff 11708.Apr 21 2022, 2:30 AM
• karol requested review of this revision.Apr 21 2022, 2:30 AM
tomek accepted this revision.Apr 21 2022, 2:36 AM
This revision is now accepted and ready to land.Apr 21 2022, 2:36 AM
Closed by commit rCOMM5aefcb036f32: [services] Backup/Blob - Add validate method to server write reactor base (authored by • karol). · Explain WhyApr 21 2022, 3:19 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM5aefcb036f32: [services] Backup/Blob - Add validate method to server write reactor base.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
server/
base-reactors/
2 lines
blob/
src/
Reactors/
server/
base-reactors/
2 lines

Diff 11708

View Options

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerWriteReactorBase.h

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import { import {
contentStringForMediaArray, contentStringForMediaArray,
multimediaMessagePreview, multimediaMessagePreview,
shimUploadURI,
} from '../../media/media-utils'; } from '../../media/media-utils';
import type { PlatformDetails } from '../../types/device-types'; import type { PlatformDetails } from '../../types/device-types';
import type { Media, Video, Image } from '../../types/media-types';
import { import {
messageTypes, messageTypes,
assertMessageType, assertMessageType,
isMediaMessageType, isMediaMessageType,
} from '../../types/message-types'; } from '../../types/message-types';
import type { import type {
MessageInfo, MessageInfo,
RawMessageInfo, RawMessageInfo,
RawMultimediaMessageInfo,
ClientDBMessageInfo, ClientDBMessageInfo,
} from '../../types/message-types'; } from '../../types/message-types';
import type { import type {
ImagesMessageData, ImagesMessageData,
RawImagesMessageInfo, RawImagesMessageInfo,
ImagesMessageInfo, ImagesMessageInfo,
} from '../../types/messages/images'; } from '../../types/messages/images';
import type { import type {
▲ Show 20 Lines • Show All 168 Lines • ▼ Show 20 Lines ): RawMediaMessageInfo | RawImagesMessageInfo {
} }
}, },
shimUnsupportedMessageInfo( shimUnsupportedMessageInfo(
rawMessageInfo: RawMediaMessageInfo | RawImagesMessageInfo, rawMessageInfo: RawMediaMessageInfo | RawImagesMessageInfo,
platformDetails: ?PlatformDetails, platformDetails: ?PlatformDetails,
): RawMediaMessageInfo | RawImagesMessageInfo | RawUnsupportedMessageInfo { ): RawMediaMessageInfo | RawImagesMessageInfo | RawUnsupportedMessageInfo {
if (rawMessageInfo.type === messageTypes.IMAGES) { if (rawMessageInfo.type === messageTypes.IMAGES) {
const shimmedRawMessageInfo = shimMediaMessageInfo( return rawMessageInfo;
rawMessageInfo,
platformDetails,
);
return shimmedRawMessageInfo;
} else { } else {
ashoatUnsubmitted
Not Done
Inline Actions

Nit – I would remove this else block... the if has a return, so the else isn't necessary

ashoat: Nit – I would remove this `else` block... the `if` has a `return`, so the `else` isn't…
rohanAuthorUnsubmitted
Done
Inline Actions

Do you have a preference on merging the if statements or keeping the logic separate?

if (rawMessageInfo.type === messageTypes.IMAGES) {
  return rawMessageInfo;
}
if (hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}

vs.

if (rawMessageInfo.type === messageTypes.IMAGES || hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}
rohan: Do you have a preference on merging the if statements or keeping the logic separate? ```…
ashoatUnsubmitted
Not Done
Inline Actions

No strong preference. I would probably keep the former personally but I'm okay either way

ashoat: No strong preference. I would probably keep the former personally but I'm okay either way
const shimmedRawMessageInfo = shimMediaMessageInfo(
rawMessageInfo,
platformDetails,
);
// TODO figure out first native codeVersion supporting video playback // TODO figure out first native codeVersion supporting video playback
ashoatUnsubmitted
Not Done
Inline Actions

I think we can remove this TODO, right @atul?

ashoat: I think we can remove this `TODO`, right @atul?
atulUnsubmitted
Not Done
Inline Actions

Yes, sorry I should have remove that earlier.

atul: Yes, sorry I should have remove that earlier.
if (hasMinCodeVersion(platformDetails, 158)) { if (hasMinCodeVersion(platformDetails, 158)) {
return shimmedRawMessageInfo; return rawMessageInfo;
} }
const { id } = shimmedRawMessageInfo; const { id } = rawMessageInfo;
invariant(id !== null && id !== undefined, 'id should be set on server'); invariant(id !== null && id !== undefined, 'id should be set on server');
return { return {
type: messageTypes.UNSUPPORTED, type: messageTypes.UNSUPPORTED,
id, id,
threadID: shimmedRawMessageInfo.threadID, threadID: rawMessageInfo.threadID,
creatorID: shimmedRawMessageInfo.creatorID, creatorID: rawMessageInfo.creatorID,
time: shimmedRawMessageInfo.time, time: rawMessageInfo.time,
robotext: multimediaMessagePreview(shimmedRawMessageInfo), robotext: multimediaMessagePreview(rawMessageInfo),
unsupportedMessageInfo: shimmedRawMessageInfo, unsupportedMessageInfo: rawMessageInfo,
}; };
} }
}, },
unshimMessageInfo( unshimMessageInfo(
unwrapped: RawMediaMessageInfo | RawImagesMessageInfo, unwrapped: RawMediaMessageInfo | RawImagesMessageInfo,
messageInfo: RawMessageInfo, messageInfo: RawMessageInfo,
): ?RawMessageInfo { ): ?RawMessageInfo {
▲ Show 20 Lines • Show All 71 Lines • ▼ Show 20 Lines ): string {
); );
}, },
generatesNotifs: async () => pushTypes.NOTIF, generatesNotifs: async () => pushTypes.NOTIF,
includedInRepliesCount: true, includedInRepliesCount: true,
}); });
function shimMediaMessageInfo(
rawMessageInfo: RawMultimediaMessageInfo,
platformDetails: ?PlatformDetails,
): RawMultimediaMessageInfo {
if (rawMessageInfo.type === messageTypes.IMAGES) {
let uriChanged = false;
const newMedia: Image[] = [];
for (const singleMedia of rawMessageInfo.media) {
const shimmedURI = shimUploadURI(singleMedia.uri, platformDetails);
if (shimmedURI === singleMedia.uri) {
newMedia.push(singleMedia);
} else {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Image));
uriChanged = true;
}
}
if (!uriChanged) {
return rawMessageInfo;
}
return ({
...rawMessageInfo,
media: newMedia,
}: RawImagesMessageInfo);
} else {
let uriChanged = false;
const newMedia: Media[] = [];
for (const singleMedia of rawMessageInfo.media) {
const shimmedURI = shimUploadURI(singleMedia.uri, platformDetails);
if (shimmedURI === singleMedia.uri) {
newMedia.push(singleMedia);
} else if (singleMedia.type === 'photo') {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Image));
uriChanged = true;
} else {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Video));
uriChanged = true;
}
}
if (!uriChanged) {
return rawMessageInfo;
}
return ({
...rawMessageInfo,
media: newMedia,
}: RawMediaMessageInfo);
}
}
// Four photos were uploaded before dimensions were calculated server-side, // Four photos were uploaded before dimensions were calculated server-side,
// and delivered to clients without dimensions in the MultimediaMessageInfo. // and delivered to clients without dimensions in the MultimediaMessageInfo.
const preDimensionUploads = { const preDimensionUploads = {
'156642': { width: 1440, height: 1080 }, '156642': { width: 1440, height: 1080 },
'156649': { width: 720, height: 803 }, '156649': { width: 720, height: 803 },
'156794': { width: 720, height: 803 }, '156794': { width: 720, height: 803 },
'156877': { width: 574, height: 454 }, '156877': { width: 574, height: 454 },
}; };
View Options

services/blob/src/Reactors/server/base-reactors/ServerWriteReactorBase.h

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import { import {
contentStringForMediaArray, contentStringForMediaArray,
multimediaMessagePreview, multimediaMessagePreview,
shimUploadURI,
} from '../../media/media-utils'; } from '../../media/media-utils';
import type { PlatformDetails } from '../../types/device-types'; import type { PlatformDetails } from '../../types/device-types';
import type { Media, Video, Image } from '../../types/media-types';
import { import {
messageTypes, messageTypes,
assertMessageType, assertMessageType,
isMediaMessageType, isMediaMessageType,
} from '../../types/message-types'; } from '../../types/message-types';
import type { import type {
MessageInfo, MessageInfo,
RawMessageInfo, RawMessageInfo,
RawMultimediaMessageInfo,
ClientDBMessageInfo, ClientDBMessageInfo,
} from '../../types/message-types'; } from '../../types/message-types';
import type { import type {
ImagesMessageData, ImagesMessageData,
RawImagesMessageInfo, RawImagesMessageInfo,
ImagesMessageInfo, ImagesMessageInfo,
} from '../../types/messages/images'; } from '../../types/messages/images';
import type { import type {
▲ Show 20 Lines • Show All 168 Lines • ▼ Show 20 Lines ): RawMediaMessageInfo | RawImagesMessageInfo {
} }
}, },
shimUnsupportedMessageInfo( shimUnsupportedMessageInfo(
rawMessageInfo: RawMediaMessageInfo | RawImagesMessageInfo, rawMessageInfo: RawMediaMessageInfo | RawImagesMessageInfo,
platformDetails: ?PlatformDetails, platformDetails: ?PlatformDetails,
): RawMediaMessageInfo | RawImagesMessageInfo | RawUnsupportedMessageInfo { ): RawMediaMessageInfo | RawImagesMessageInfo | RawUnsupportedMessageInfo {
if (rawMessageInfo.type === messageTypes.IMAGES) { if (rawMessageInfo.type === messageTypes.IMAGES) {
const shimmedRawMessageInfo = shimMediaMessageInfo( return rawMessageInfo;
rawMessageInfo,
platformDetails,
);
return shimmedRawMessageInfo;
} else { } else {
ashoatUnsubmitted
Not Done
Inline Actions

Nit – I would remove this else block... the if has a return, so the else isn't necessary

ashoat: Nit – I would remove this `else` block... the `if` has a `return`, so the `else` isn't…
rohanAuthorUnsubmitted
Done
Inline Actions

Do you have a preference on merging the if statements or keeping the logic separate?

if (rawMessageInfo.type === messageTypes.IMAGES) {
  return rawMessageInfo;
}
if (hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}

vs.

if (rawMessageInfo.type === messageTypes.IMAGES || hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}
rohan: Do you have a preference on merging the if statements or keeping the logic separate? ```…
ashoatUnsubmitted
Not Done
Inline Actions

No strong preference. I would probably keep the former personally but I'm okay either way

ashoat: No strong preference. I would probably keep the former personally but I'm okay either way
const shimmedRawMessageInfo = shimMediaMessageInfo(
rawMessageInfo,
platformDetails,
);
// TODO figure out first native codeVersion supporting video playback // TODO figure out first native codeVersion supporting video playback
ashoatUnsubmitted
Not Done
Inline Actions

I think we can remove this TODO, right @atul?

ashoat: I think we can remove this `TODO`, right @atul?
atulUnsubmitted
Not Done
Inline Actions

Yes, sorry I should have remove that earlier.

atul: Yes, sorry I should have remove that earlier.
if (hasMinCodeVersion(platformDetails, 158)) { if (hasMinCodeVersion(platformDetails, 158)) {
return shimmedRawMessageInfo; return rawMessageInfo;
} }
const { id } = shimmedRawMessageInfo; const { id } = rawMessageInfo;
invariant(id !== null && id !== undefined, 'id should be set on server'); invariant(id !== null && id !== undefined, 'id should be set on server');
return { return {
type: messageTypes.UNSUPPORTED, type: messageTypes.UNSUPPORTED,
id, id,
threadID: shimmedRawMessageInfo.threadID, threadID: rawMessageInfo.threadID,
creatorID: shimmedRawMessageInfo.creatorID, creatorID: rawMessageInfo.creatorID,
time: shimmedRawMessageInfo.time, time: rawMessageInfo.time,
robotext: multimediaMessagePreview(shimmedRawMessageInfo), robotext: multimediaMessagePreview(rawMessageInfo),
unsupportedMessageInfo: shimmedRawMessageInfo, unsupportedMessageInfo: rawMessageInfo,
}; };
} }
}, },
unshimMessageInfo( unshimMessageInfo(
unwrapped: RawMediaMessageInfo | RawImagesMessageInfo, unwrapped: RawMediaMessageInfo | RawImagesMessageInfo,
messageInfo: RawMessageInfo, messageInfo: RawMessageInfo,
): ?RawMessageInfo { ): ?RawMessageInfo {
▲ Show 20 Lines • Show All 71 Lines • ▼ Show 20 Lines ): string {
); );
}, },
generatesNotifs: async () => pushTypes.NOTIF, generatesNotifs: async () => pushTypes.NOTIF,
includedInRepliesCount: true, includedInRepliesCount: true,
}); });
function shimMediaMessageInfo(
rawMessageInfo: RawMultimediaMessageInfo,
platformDetails: ?PlatformDetails,
): RawMultimediaMessageInfo {
if (rawMessageInfo.type === messageTypes.IMAGES) {
let uriChanged = false;
const newMedia: Image[] = [];
for (const singleMedia of rawMessageInfo.media) {
const shimmedURI = shimUploadURI(singleMedia.uri, platformDetails);
if (shimmedURI === singleMedia.uri) {
newMedia.push(singleMedia);
} else {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Image));
uriChanged = true;
}
}
if (!uriChanged) {
return rawMessageInfo;
}
return ({
...rawMessageInfo,
media: newMedia,
}: RawImagesMessageInfo);
} else {
let uriChanged = false;
const newMedia: Media[] = [];
for (const singleMedia of rawMessageInfo.media) {
const shimmedURI = shimUploadURI(singleMedia.uri, platformDetails);
if (shimmedURI === singleMedia.uri) {
newMedia.push(singleMedia);
} else if (singleMedia.type === 'photo') {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Image));
uriChanged = true;
} else {
newMedia.push(({ ...singleMedia, uri: shimmedURI }: Video));
uriChanged = true;
}
}
if (!uriChanged) {
return rawMessageInfo;
}
return ({
...rawMessageInfo,
media: newMedia,
}: RawMediaMessageInfo);
}
}
// Four photos were uploaded before dimensions were calculated server-side, // Four photos were uploaded before dimensions were calculated server-side,
// and delivered to clients without dimensions in the MultimediaMessageInfo. // and delivered to clients without dimensions in the MultimediaMessageInfo.
const preDimensionUploads = { const preDimensionUploads = {
'156642': { width: 1440, height: 1080 }, '156642': { width: 1440, height: 1080 },
'156649': { width: 720, height: 803 }, '156649': { width: 720, height: 803 },
'156794': { width: 720, height: 803 }, '156794': { width: 720, height: 803 },
'156877': { width: 574, height: 454 }, '156877': { width: 574, height: 454 },
}; };