Merge pull request #1529 from session-foundation/fix-uneeded-local-refs-attachments-id

Bilb · web-flow · commit d4a198ab080e · 2025-07-30T14:54:40.000+10:00
fix: remove uneeded local refs to attachment ids
diff --git a/protos/SignalService.proto b/protos/SignalService.proto
@@ -261,7 +261,7 @@ message AttachmentPointer {
   }
 
   // @required
-  required fixed64 id          = 1;
+  required fixed64 deprecated_id = 1;
   optional string  contentType = 2;
   optional bytes   key         = 3;
   optional uint32  size        = 4;
diff --git a/ts/components/conversation/ImageGrid.tsx b/ts/components/conversation/ImageGrid.tsx
@@ -59,7 +59,7 @@ const Row = (
             attachment={attachment}
             playIconOverlay={isVideoAttachment(attachment)}
             height={renderedSize}
-            key={attachment.id}
+            key={attachment.path}
             width={renderedSize}
             url={isMessageVisible ? getThumbnailUrl(attachment) : undefined}
             attachmentIndex={startIndex + index}
diff --git a/ts/components/conversation/right-panel/overlay/message-info/components/AttachmentInfo.tsx b/ts/components/conversation/right-panel/overlay/message-info/components/AttachmentInfo.tsx
@@ -26,7 +26,7 @@ export const AttachmentInfo = (props: Props) => {
     <Flex $container={true} $flexDirection="column">
       <LabelWithInfo
         label={tr('attachmentsFileId')}
-        info={attachment?.id ? String(attachment.id) : tr('attachmentsNa')}
+        info={attachment?.url ? String(attachment.url) : tr('attachmentsNa')}
       />
       <StyledLabelContainer $container={true} $flexDirection="row" $flexWrap="wrap">
         <LabelWithInfo
diff --git a/ts/models/message.ts b/ts/models/message.ts
@@ -715,7 +715,6 @@ export class MessageModel extends Model<MessageAttributes> {
     }
 
     const {
-      id,
       path,
       contentType,
       width,
@@ -737,7 +736,6 @@ export class MessageModel extends Model<MessageAttributes> {
       false;
 
     return {
-      id,
       contentType,
       caption,
       size: size || 0,
@@ -817,11 +815,12 @@ export class MessageModel extends Model<MessageAttributes> {
       fileIdsToLink.push(attachmentIdAsStrFromUrl(preview.image.url));
     }
 
-    if (quote && quote.attachments?.length) {
+    if (quote && quote.attachments?.length && quote.attachments[0].thumbnail) {
       // typing for all of this Attachment + quote + preview + send or unsend is pretty bad
-      const firstQuoteAttachmentId = (quote.attachments[0].thumbnail as any)?.id;
-      if (firstQuoteAttachmentId) {
-        fileIdsToLink.push(firstQuoteAttachmentId);
+      const firstQuoteAttachmentUrl =
+        'url' in quote.attachments[0].thumbnail ? quote.attachments[0].thumbnail.url : undefined;
+      if (firstQuoteAttachmentUrl && attachmentIdAsStrFromUrl(firstQuoteAttachmentUrl)) {
+        fileIdsToLink.push(attachmentIdAsStrFromUrl(firstQuoteAttachmentUrl));
       }
     }
 
@@ -1105,6 +1104,7 @@ export class MessageModel extends Model<MessageAttributes> {
     }
 
     const errorStr = `${providedError.name} - "${providedError.message || 'unknown error message'}"`;
+    window.log.info(`save message error to msg ${this.idForLogging()} with error:`, errorStr);
 
     this.set({ errors: errorStr });
     await this.commit();
diff --git a/ts/receiver/attachments.ts b/ts/receiver/attachments.ts
@@ -2,7 +2,7 @@ import { omit, startsWith } from 'lodash';
 
 import { MessageModel } from '../models/message';
 import { Data } from '../data/data';
-import { AttachmentDownloads } from '../session/utils';
+import { AttachmentDownloads, attachmentIdAsStrFromUrl } from '../session/utils';
 import { ConversationModel } from '../models/conversation';
 import { getUnpaddedAttachment } from '../session/crypto/BufferPadding';
 import { decryptAttachment } from '../util/crypto/attachmentsEncrypter';
@@ -32,7 +32,7 @@ export async function downloadAttachment(attachment: {
   let res: ArrayBuffer | null = null;
   // try to get the fileId from the end of the URL
 
-  const attachmentId = attachment.id || attachment.url;
+  const attachmentId = attachmentIdAsStrFromUrl(attachment.url);
   if (!defaultFileServer) {
     window.log.warn(
       `downloadAttachment attachment is neither opengroup attachment nor fileserver... Dropping it ${asURL.href}`
@@ -92,7 +92,6 @@ export async function downloadAttachment(attachment: {
  */
 export async function downloadAttachmentSogsV3(
   attachment: {
-    id: number;
     url: string;
     size: number | null;
   },
@@ -103,7 +102,10 @@ export async function downloadAttachmentSogsV3(
     throw new Error(`Didn't find such a room ${roomInfos.serverUrl}: ${roomInfos.roomId}`);
   }
 
-  const dataUint = await sogsV3FetchFileByFileID(roomDetails, `${attachment.id}`);
+  const dataUint = await sogsV3FetchFileByFileID(
+    roomDetails,
+    attachmentIdAsStrFromUrl(attachment.url)
+  );
 
   if (!dataUint?.length) {
     window?.log?.error('Failed to download attachment. Length is 0');
diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts
@@ -29,10 +29,10 @@ import { GroupV2Receiver } from './groupv2/handleGroupV2Message';
 import { ConversationTypeEnum } from '../models/types';
 import { ed25519Str } from '../session/utils/String';
 
-function cleanAttachment(attachment: any) {
+function cleanAttachment(attachment: SignalService.IAttachmentPointer) {
   return {
     ...omit(attachment, 'thumbnail'),
-    id: attachment.id.toString(),
+    id: 0,
     key: attachment.key ? StringUtils.decode(attachment.key, 'base64') : null,
     digest:
       attachment.digest && attachment.digest.length > 0
diff --git a/ts/session/apis/file_server_api/FileServerApi.ts b/ts/session/apis/file_server_api/FileServerApi.ts
@@ -60,7 +60,7 @@ export const uploadFileToFsWithOnionV4 = async (
     return null;
   }
 
-  const fileId = result?.body?.id as number | undefined;
+  const fileId = result?.body?.id as string | undefined;
   const expires = result?.body?.expires as number; // expires is returned as a floating point timestamp in seconds, i.e. 1754863793.186137.
   if (
     !fileId ||
diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts
@@ -204,6 +204,7 @@ export type SubRequestUpdateRoomType = {
   type: 'updateRoom';
   updateRoom: {
     roomId: string;
+    // imageId has to be a number to be understood by the sogs
     imageId: number; // the fileId uploaded to this sogs and to be referenced as preview/room image
     // name and other options are unsupported for now
   };
diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3RoomImage.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3RoomImage.ts
@@ -1,5 +1,5 @@
 import AbortController from 'abort-controller';
-import { isNumber } from 'lodash';
+import { isString, toNumber } from 'lodash';
 import { batchFirstSubIsSuccess, batchGlobalIsSuccess, sogsBatchSend } from './sogsV3BatchPoll';
 import { uploadFileToRoomSogs3 } from './sogsV3SendFile';
 import { OpenGroupRequestCommonType } from '../../../../data/types';
@@ -17,25 +17,30 @@ import { DURATION } from '../../../constants';
 export const uploadImageForRoomSogsV3 = async (
   fileContent: Uint8Array,
   roomInfos: OpenGroupRequestCommonType
-): Promise<{ fileUrl: string; fileId: number } | null> => {
+): Promise<{ fileUrl: string } | null> => {
   if (!fileContent || !fileContent.length) {
     return null;
   }
 
   const result = await uploadFileToRoomSogs3(fileContent, roomInfos);
-  if (!result || !isNumber(result.fileId)) {
+  if (!result || !isString(result.fileId)) {
     return null;
   }
-  const { fileId, fileUrl } = result;
-  if (!fileId || !fileContent.length) {
+  const { fileUrl } = result;
+  if (!fileContent.length) {
     return null;
   }
 
   const batchResult = await sogsBatchSend(
     roomInfos.serverUrl,
     new Set([roomInfos.roomId]),
     new AbortController().signal,
-    [{ type: 'updateRoom', updateRoom: { roomId: roomInfos.roomId, imageId: fileId } }],
+    [
+      {
+        type: 'updateRoom',
+        updateRoom: { roomId: roomInfos.roomId, imageId: toNumber(result.fileId) },
+      },
+    ],
     'batch',
     30 * DURATION.SECONDS // longer time for image upload
   );
@@ -45,6 +50,5 @@ export const uploadImageForRoomSogsV3 = async (
   }
   return {
     fileUrl,
-    fileId,
   };
 };
diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendFile.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendFile.ts
@@ -1,3 +1,4 @@
+import { isNumber } from 'lodash';
 import AbortController from 'abort-controller';
 import { OpenGroupData } from '../../../../data/opengroups';
 import { roomHasBlindEnabled } from '../../../../types/sqlSharedTypes';
@@ -12,7 +13,7 @@ import { DURATION } from '../../../constants';
 export const uploadFileToRoomSogs3 = async (
   fileContent: Uint8Array,
   roomInfos: OpenGroupRequestCommonType
-): Promise<{ fileId: number; fileUrl: string } | null> => {
+): Promise<{ fileUrl: string; fileId: string } | null> => {
   if (!fileContent || !fileContent.length) {
     return null;
   }
@@ -40,13 +41,14 @@ export const uploadFileToRoomSogs3 = async (
   }
 
   const fileId = (result?.body as any | undefined)?.id as number | undefined;
-  if (!fileId) {
+
+  if (!fileId || !isNumber(fileId)) {
     return null;
   }
   const fileUrl = `${roomInfos.serverUrl}/room/${roomDetails.roomId}/file/${fileId}`;
 
   return {
-    fileId,
     fileUrl,
+    fileId: `${fileId}`,
   };
 };
diff --git a/ts/session/messages/outgoing/visibleMessage/VisibleMessage.ts b/ts/session/messages/outgoing/visibleMessage/VisibleMessage.ts
@@ -74,12 +74,12 @@ export interface VisibleMessageParams extends ExpirableMessageParams {
 export class VisibleMessage extends DataMessage {
   public readonly reaction?: Reaction;
 
-  private readonly attachments?: Array<AttachmentPointerWithUrl & { id: Long }>;
+  private readonly attachments?: Array<AttachmentPointerWithUrl & { deprecatedId: Long }>;
   private readonly body?: string;
   private readonly quote?: Quote;
   private readonly profileKey?: Uint8Array;
   private readonly profile?: SignalService.DataMessage.ILokiProfile;
-  private readonly preview?: Array<PreviewWithAttachmentUrl & { id?: Long }>;
+  private readonly preview?: Array<PreviewWithAttachmentUrl & { deprecatedId?: Long }>;
 
   /// In the case of a sync message, the public key of the person the message was targeted at.
   /// - Note: `null or undefined` if this isn't a sync message.
@@ -94,7 +94,7 @@ export class VisibleMessage extends DataMessage {
     });
     this.attachments = params.attachments?.map(attachment => ({
       ...attachment,
-      id: attachmentIdAsLongFromUrl(attachment.url),
+      deprecatedId: attachmentIdAsLongFromUrl(attachment.url),
     }));
     this.body = params.body;
     this.quote = params.quote;
@@ -106,7 +106,9 @@ export class VisibleMessage extends DataMessage {
 
     this.preview = params.preview?.map(attachment => ({
       ...attachment,
-      id: attachment.image?.url ? attachmentIdAsLongFromUrl(attachment.image.url) : undefined,
+      deprecatedId: attachment.image?.url
+        ? attachmentIdAsLongFromUrl(attachment.image.url)
+        : undefined,
     }));
     this.reaction = params.reaction;
     this.syncTarget = params.syncTarget;
@@ -176,7 +178,7 @@ export class VisibleMessage extends DataMessage {
           item.url = preview.url;
         }
         item.image = preview.image
-          ? { ...preview.image, id: attachmentIdAsLongFromUrl(preview.image.url) }
+          ? { ...preview.image, deprecatedId: attachmentIdAsLongFromUrl(preview.image.url) }
           : null;
 
         return item;
diff --git a/ts/session/utils/Attachments.ts b/ts/session/utils/Attachments.ts
@@ -1,5 +1,5 @@
 import * as crypto from 'crypto';
-import _ from 'lodash';
+import _, { isEmpty, isString } from 'lodash';
 import Long from 'long';
 
 import { Attachment } from '../../types/Attachment';
@@ -154,14 +154,26 @@ export async function uploadQuoteThumbnailsToFileServer(
 }
 
 export function attachmentIdAsStrFromUrl(url: string) {
-  return attachmentIdAsLongFromUrl(url).toString();
+  const lastSegment = url?.split('/')?.pop();
+  if (!lastSegment) {
+    throw new Error('attachmentIdAsStrFromUrl last is not valid');
+  }
+  return lastSegment;
 }
 
 export function attachmentIdAsLongFromUrl(url: string) {
   const lastSegment = url?.split('/')?.pop();
   if (!lastSegment) {
     throw new Error('attachmentIdAsLongFromUrl last is not valid');
   }
-  // this throws if not a valid long
-  return Long.fromString(lastSegment);
+  try {
+    // this throws if not a valid long
+    return Long.fromString(lastSegment);
+  } catch (e) {
+    window.log.warn(`attachmentIdAsLongFromUrl failed with ${e.message}.. Returning 0.`);
+    if (isString(lastSegment) && !isEmpty(lastSegment)) {
+      return Long.fromNumber(0);
+    }
+    throw e;
+  }
 }
diff --git a/ts/session/utils/AttachmentsDownload.ts b/ts/session/utils/AttachmentsDownload.ts
@@ -184,9 +184,7 @@ async function _runJob(job: any) {
       // Attachments on the server expire after 60 days, then start returning 404
       if (error && error.code === 404) {
         logger.warn(
-          `_runJob: Got 404 from server, marking attachment ${
-            attachment.id
-          } from message ${found.idForLogging()} as permanent error`
+          `_runJob: Got 404 from server, marking attachment from message ${found.idForLogging()} as permanent error`
         );
 
         // Make sure to fetch the message from DB here right before writing it.
diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts
@@ -130,7 +130,6 @@ export type PropsForCommunityInvitation = {
 };
 
 export type PropsForAttachment = AttachmentType & {
-  id: number;
   isVoiceMessage: boolean;
   size: number;
   path: string;
diff --git a/ts/state/ducks/sogsRoomInfo.tsx b/ts/state/ducks/sogsRoomInfo.tsx
@@ -71,13 +71,10 @@ const changeCommunityAvatar = createAsyncThunk(
       window?.log?.warn('File upload for community failed');
       return false;
     }
-    const { fileId: avatarImageId, fileUrl } = uploadedFileDetails;
+    const { fileUrl } = uploadedFileDetails;
 
     // this is kind of a hack just made to avoid having a specific function downloading from sogs by URL rather than fileID
-    const downloaded = await downloadAttachmentSogsV3(
-      { id: avatarImageId, size: null, url: fileUrl },
-      roomInfos
-    );
+    const downloaded = await downloadAttachmentSogsV3({ size: null, url: fileUrl }, roomInfos);
 
     if (!downloaded || !(downloaded.data instanceof ArrayBuffer)) {
       const typeFound = typeof downloaded;
diff --git a/ts/test/session/unit/messages/ChatMessage_test.ts b/ts/test/session/unit/messages/ChatMessage_test.ts
@@ -156,7 +156,7 @@ describe('VisibleMessage', () => {
     const decoded = SignalService.Content.decode(plainText);
     expect(decoded.dataMessage?.attachments).to.have.lengthOf(1);
     const firstAttachment = decoded?.dataMessage?.attachments?.[0];
-    const decodedID = toNumber(firstAttachment?.id);
+    const decodedID = toNumber(firstAttachment?.deprecatedId);
     expect(decodedID).to.be.equal(1234);
     expect(firstAttachment?.contentType).to.be.deep.equal('contentType');
     expect(firstAttachment?.url).to.be.deep.equal('http://thisisaareal/url/1234');
diff --git a/ts/types/Attachment.ts b/ts/types/Attachment.ts
@@ -54,7 +54,6 @@ export interface AttachmentType {
 
 export interface AttachmentTypeWithPath extends AttachmentType {
   path: string;
-  id: number;
   flags?: number;
   error?: any;
 }

Original file line number	Diff line number	Diff line change
`@@ -261,7 +261,7 @@ message AttachmentPointer {`
`261`	`261`	`}`
`262`	`262`
`263`	`263`	`// @required`
`264`		`- required fixed64 id = 1;`
	`264`	`+ required fixed64 deprecated_id = 1;`
`265`	`265`	`optional string contentType = 2;`
`266`	`266`	`optional bytes key = 3;`
`267`	`267`	`optional uint32 size = 4;`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ export const uploadFileToFsWithOnionV4 = async (`
`60`	`60`	`return null;`
`61`	`61`	`}`
`62`	`62`
`63`		`- const fileId = result?.body?.id as number \| undefined;`
	`63`	`+ const fileId = result?.body?.id as string \| undefined;`
`64`	`64`	`const expires = result?.body?.expires as number; // expires is returned as a floating point timestamp in seconds, i.e. 1754863793.186137.`
`65`	`65`	`if (`
`66`	`66`	`!fileId \|\|`