-
-
Notifications
You must be signed in to change notification settings - Fork 424
Makes document attachment usage consider external attachments #1600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 17 commits
3938de5
9b5638b
49e5890
b5c102d
09692d4
922a781
85e1469
aacb5ee
bc5ae34
8776ddb
eb2e61d
572b042
90889ae
545b753
3d4c312
aa16df7
a3d5583
58c797f
054792a
e8acbee
e6f70e8
a55f7e6
6a75682
e121a2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1054,6 +1054,8 @@ export class ActiveDoc extends EventEmitter { | |
unused: 0, | ||
}; | ||
|
||
const sizesToUpdate = new Map<string, number>(); | ||
|
||
await unpackTarArchive(tarFile, async (file) => { | ||
try { | ||
const fileIdent = archiveFilePathToAttachmentIdent(file.path); | ||
|
@@ -1063,6 +1065,7 @@ export class ActiveDoc extends EventEmitter { | |
fallbackStoreId, | ||
); | ||
if (isAdded) { | ||
sizesToUpdate.set(fileIdent, file.size); | ||
results.added += 1; | ||
} else { | ||
results.unused += 1; | ||
|
@@ -1076,6 +1079,11 @@ export class ActiveDoc extends EventEmitter { | |
} | ||
}); | ||
|
||
// This updates _grist_Attachments.fileSize with the size of the uploaded files. | ||
// This prevents a loophole where a user has altered `fileSize`, imported the altered document, | ||
// then restored the originals. | ||
await this._updateAttachmentFileSizesUsingFileIdent(docSession, sizesToUpdate); | ||
|
||
return results; | ||
} | ||
|
||
|
@@ -2614,6 +2622,37 @@ export class ActiveDoc extends EventEmitter { | |
}]; | ||
} | ||
|
||
private async _updateAttachmentFileSizesUsingFileIdent(docSession: OptDocSession, | ||
newFileSizesByFileIdent: Map<string, number>): Promise<void> { | ||
const rowIdsToUpdate: number[] = []; | ||
const newFileSizesForRows: CellValue[] = []; | ||
const attachments = this.docData?.getMetaTable("_grist_Attachments").getRecords(); | ||
for (const attachmentRec of attachments ?? []) { | ||
const newSize = newFileSizesByFileIdent.get(attachmentRec.fileIdent); | ||
if (newSize && newSize !== attachmentRec.fileSize) { | ||
rowIdsToUpdate.push(attachmentRec.id); | ||
newFileSizesForRows.push(newSize); | ||
} | ||
} | ||
|
||
if (rowIdsToUpdate.length === 0) { | ||
return; | ||
} | ||
|
||
const action: BulkUpdateRecord = ['BulkUpdateRecord', '_grist_Attachments', rowIdsToUpdate, { | ||
fileSize: newFileSizesForRows | ||
}]; | ||
|
||
await this._applyUserActionsWithExtendedOptions( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about wrapping it with Ideally this would all be done in "transaction like" scope, but I don't know if it is important to worry about that here. We have some mechanism for locking the doc modifications, but maybe this is overkill here. But there is other mechanism available - the upsert action There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a difficult question. The options seem to be:
If this is very rare, I think I'd prefer to let it fail and force the user to re-import, than import attachments with an invalid file size. I don't think upsert is an option unfortunately, as it's not correct to add a new record if it doesn't exist. Best case, we have an extra attachment being tracked that doesn't exist. Worst case, it breaks other code because it's not a fully valid record that's added (unless we try to reinsert the whole record...). My inclination is to leave it, add a comment, see if it actually comes up in production. How do other parts of the codebase handle the in-memory data and data engine being out of sync? Just locking? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The minimum was to wrap this in try catch, and rethrow something more meaningful, explaining what has happen. I don't know for sure, but we have some data corruptions problems we don't know how to fix or don't know the reason. So here, we know that the ids can go wrong, so we can anticipate it and explain in more details. I just checked the docstring for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this, but apparently that method doesn't support metatables Instead, I've wrapped it in a more specific error, and added some logging around that error too. How does that look? |
||
docSession, | ||
[action], | ||
{attachment: true}, | ||
); | ||
|
||
// Updates doc's overall attachment usage to reflect any changes to file sizes. | ||
await this._updateAttachmentsSize(); | ||
} | ||
|
||
/** | ||
* If the software is newer than the document, migrate the document by fetching all tables, and | ||
* giving them to the sandbox so that it can produce migration actions. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth checking if
rowIdsToUpdate
is empty and returning earlier (without all external calls)?