-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
Adds initial support for external attachments using the Filesystem or MinIO #1320
Open
Spoffy
wants to merge
59
commits into
main
Choose a base branch
from
spoffy/external-attachments-prototype
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 53 commits
Commits
Show all changes
59 commits
Select commit
Hold shift + click to select a range
2473375
Enables checksumFile to handle streams
Spoffy ff2fe69
Routes attachment handling through AttachmentFileManager
Spoffy fc35744
Switches ActiveDoc to using AttachmentFileManager for attachments
Spoffy ef13def
Fixes a casing error in DocStorage
Spoffy d19ff54
Adds working filesystem store (in use by default)
Spoffy 8295f5e
Avoids storing external attachments in the sqlite file
Spoffy 8f09573
Renames some parameters in AttachmentStore
Spoffy 72f2e5c
Adds "removeAllWithPrefix?" to ExternalStorage and MinIOExternalStorage
Spoffy 5e26ec8
Adds a streaming API to MinIOExternalStorage
Spoffy 20a1a79
Adds prototype MinIO attachment store
Spoffy 482b5c7
Makes AttachmentStoreProvider function correctly
Spoffy 59d1633
Enables setting document attachment store
Spoffy c1058da
Makes store creation error if not setup
Spoffy 74102d0
Refactors DocInfo to be reusable
Spoffy 6640133
Fixes pool deletion not working for MinIO
Spoffy 85ba845
Adds lifecycle management to external attachments
Spoffy 3902f82
Adds attachment upload/download logging
Spoffy 4085c06
Fixes test build errors
Spoffy 65bc7a9
Misc. cleanup for external attachments.
Spoffy ce5282f
Fixes import ordering issues
Spoffy e53f6eb
Improves documentation of AttachmentFileManager
Spoffy 819e0d6
Switches AttachmentFileManager to structured logging
Spoffy 52b7e24
Tidies up imports
Spoffy 79396dc
Adds doc pool description to AttachmentStore
Spoffy d527e59
Improves naming of attachment store backend options
Spoffy c3c82bd
Adds explanation on store concepts
Spoffy dc19e54
Changes MinIOAttachmentStore to ExternalStorageAttachmentStore
Spoffy 4d7dc11
Makes attachment store API stream based
Spoffy 340c5b3
Adds tests for FilesystemAttachmentStore.ts
Spoffy 6af1818
Makes attachment store creation async, removes check methods
Spoffy 8ca7196
Adds tests for AttachmentStoreProvider
Spoffy 2d59def
Fixes type for FileInfo.storageId
Spoffy fc9db0c
Adds listAllStoreIds to IAttachmentStoreProvider
Spoffy 72eecf3
Fixes typing on AttachmentFileManagerLogInfo
Spoffy cdcfb0d
Make makeTestingFilesystemStoreSpec reuse the directory
Spoffy 0bddba2
Adds AttachmentFileManager unit tests
Spoffy b9d2f8c
Adds attachment store backend availability checks
Spoffy e5b508c
Merge branch 'main' into spoffy/external-attachments-prototype
Spoffy f712ef8
Cleans up linter issues
Spoffy 364ac08
Fixes findOrAttachFile test
Spoffy ae78bba
Removes unnecessary log line
Spoffy 4730d99
Makes _getDocumentSettings error if not found.
Spoffy c39806f
Undoes erroneous storage schema change
Spoffy 58a101d
Fix getDocumentSettings bad error condition
Spoffy 785971f
Merge branch 'main' into spoffy/external-attachments-prototype
Spoffy f93948f
Updates migration tests with additional fixtures
Spoffy a4768a5
Fixes an error with missing storageId in doc schema
Spoffy c41b4db
Attempt to change BlobMigrationV8 to V9 using upgradeDocument
Spoffy f0222ca
Updates BlobMigrationV9 and associated tooling
Spoffy 3008fdd
Fixes failing DocStorage tests
Spoffy 4a1c112
Renames "idOfDefaultAttachmentStore" to "attachmentStoreId"
Spoffy adac94b
Makes getFileData throw an error if file is missing
Spoffy 3b0f603
Updates comments to address feedback
Spoffy 2ba5848
Removes leftover debugging log
Spoffy 635a92c
Updates AttachmentStore comments
Spoffy dc55180
Makes removeAllWithPrefix check clearer
Spoffy ae87fa1
Simplifies MinIOExternalStorage "_listObjects" method
Spoffy b46b320
Fixes test import
Spoffy e44aadf
Removes TODO from coreCreator.ts
Spoffy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,217 @@ | ||
import { | ||
AttachmentStoreDocInfo, | ||
DocPoolId, | ||
getDocPoolIdFromDocInfo, | ||
IAttachmentStore | ||
} from "app/server/lib/AttachmentStore"; | ||
import { AttachmentStoreId, IAttachmentStoreProvider } from "app/server/lib/AttachmentStoreProvider"; | ||
import { checksumFileStream } from "app/server/lib/checksumFile"; | ||
import { DocStorage } from "app/server/lib/DocStorage"; | ||
import log from "app/server/lib/log"; | ||
import { LogMethods } from "app/server/lib/LogMethods"; | ||
import { MemoryWritableStream } from "app/server/utils/MemoryWritableStream"; | ||
import { Readable } from "node:stream"; | ||
|
||
export interface IAttachmentFileManager { | ||
addFile(storeId: AttachmentStoreId, fileExtension: string, fileData: Buffer): Promise<AddFileResult>; | ||
getFileData(fileIdent: string): Promise<Buffer | null>; | ||
} | ||
|
||
export interface AddFileResult { | ||
fileIdent: string; | ||
isNewFile: boolean; | ||
} | ||
|
||
export class StoresNotConfiguredError extends Error { | ||
constructor() { | ||
super('Attempted to access a file store, but AttachmentFileManager was initialized without store access'); | ||
} | ||
} | ||
|
||
export class StoreNotAvailableError extends Error { | ||
public readonly storeId: AttachmentStoreId; | ||
|
||
constructor(storeId: AttachmentStoreId) { | ||
super(`Store '${storeId}' is not a valid and available store`); | ||
this.storeId = storeId; | ||
} | ||
} | ||
|
||
export class MissingAttachmentError extends Error { | ||
public readonly fileIdent: string; | ||
|
||
constructor(fileIdent: string) { | ||
super(`Attachment file '${fileIdent}' could not be found in this document`); | ||
this.fileIdent = fileIdent; | ||
} | ||
} | ||
|
||
export class AttachmentRetrievalError extends Error { | ||
public readonly storeId: AttachmentStoreId; | ||
public readonly fileId: string; | ||
|
||
constructor(storeId: AttachmentStoreId, fileId: string, cause?: any) { | ||
const causeError = cause instanceof Error ? cause : undefined; | ||
const causeDescriptor = causeError ? `: ${cause.message}` : ''; | ||
super(`Unable to retrieve '${fileId}' from '${storeId}'${causeDescriptor}`); | ||
this.storeId = storeId; | ||
this.fileId = fileId; | ||
this.cause = causeError; | ||
} | ||
} | ||
|
||
|
||
interface AttachmentFileManagerLogInfo { | ||
fileIdent?: string; | ||
storeId?: string | null; | ||
} | ||
|
||
/** | ||
* Instantiated on a per-document basis to provide a document with access to its attachments. | ||
* Handles attachment uploading / fetching, as well as trying to ensure consistency with the local document database, | ||
* which tracks attachments and where they're stored. | ||
* | ||
* This class should prevent the document code from having to worry about accessing the underlying stores. | ||
*/ | ||
export class AttachmentFileManager implements IAttachmentFileManager { | ||
// _docPoolId is a critical point for security. Documents with a common pool id can access each others' attachments. | ||
paulfitz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private readonly _docPoolId: DocPoolId | null; | ||
private readonly _docName: string; | ||
private _log = new LogMethods( | ||
"AttachmentFileManager ", | ||
(logInfo: AttachmentFileManagerLogInfo) => this._getLogMeta(logInfo) | ||
); | ||
|
||
/** | ||
* @param _docStorage - Storage of this manager's document. | ||
* @param _storeProvider - Allows instantiating of stores. Should be provided except in test scenarios. | ||
* @param _docInfo - The document this manager is for. Should be provided except in test scenarios. | ||
*/ | ||
constructor( | ||
private _docStorage: DocStorage, | ||
private _storeProvider: IAttachmentStoreProvider | undefined, | ||
_docInfo: AttachmentStoreDocInfo | undefined, | ||
) { | ||
this._docName = _docStorage.docName; | ||
this._docPoolId = _docInfo ? getDocPoolIdFromDocInfo(_docInfo) : null; | ||
} | ||
|
||
public async addFile( | ||
storeId: AttachmentStoreId | undefined, | ||
fileExtension: string, | ||
fileData: Buffer | ||
): Promise<AddFileResult> { | ||
const fileIdent = await this._getFileIdentifier(fileExtension, Readable.from(fileData)); | ||
return this._addFile(storeId, fileIdent, fileData); | ||
} | ||
|
||
public async _addFile( | ||
storeId: AttachmentStoreId | undefined, | ||
fileIdent: string, | ||
fileData: Buffer | ||
): Promise<AddFileResult> { | ||
this._log.info({ fileIdent, storeId }, `adding file to ${storeId ? "external" : "document"} storage`); | ||
if (storeId === undefined) { | ||
return this._addFileToLocalStorage(fileIdent, fileData); | ||
} | ||
const store = await this._getStore(storeId); | ||
if (!store) { | ||
this._log.info({ fileIdent, storeId }, "tried to fetch attachment from an unavailable store"); | ||
throw new StoreNotAvailableError(storeId); | ||
} | ||
return this._addFileToAttachmentStore(store, fileIdent, fileData); | ||
} | ||
|
||
public async getFileData(fileIdent: string): Promise<Buffer> { | ||
const fileInfo = await this._docStorage.getFileInfo(fileIdent); | ||
if (!fileInfo) { | ||
this._log.error({ fileIdent }, "cannot find file metadata in document"); | ||
throw new MissingAttachmentError(fileIdent); | ||
} | ||
this._log.debug( | ||
{ fileIdent, storeId: fileInfo.storageId }, | ||
`fetching attachment from ${fileInfo.storageId ? "external" : "document "} storage` | ||
); | ||
if (!fileInfo.storageId) { | ||
return fileInfo.data; | ||
} | ||
const store = await this._getStore(fileInfo.storageId); | ||
if (!store) { | ||
this._log.warn({ fileIdent, storeId: fileInfo.storageId }, `unable to retrieve file, store is unavailable`); | ||
throw new StoreNotAvailableError(fileInfo.storageId); | ||
} | ||
return this._getFileDataFromAttachmentStore(store, fileIdent); | ||
} | ||
|
||
private async _addFileToLocalStorage(fileIdent: string, fileData: Buffer): Promise<AddFileResult> { | ||
const isNewFile = await this._docStorage.findOrAttachFile(fileIdent, fileData); | ||
|
||
return { | ||
fileIdent, | ||
isNewFile, | ||
}; | ||
} | ||
|
||
private async _getStore(storeId: AttachmentStoreId): Promise<IAttachmentStore | null> { | ||
if (!this._storeProvider) { | ||
throw new StoresNotConfiguredError(); | ||
} | ||
return this._storeProvider.getStore(storeId); | ||
} | ||
|
||
private _getDocPoolId(): string { | ||
if (!this._docPoolId) { | ||
throw new StoresNotConfiguredError(); | ||
} | ||
return this._docPoolId; | ||
} | ||
|
||
private async _getFileIdentifier(fileExtension: string, fileData: Readable): Promise<string> { | ||
const checksum = await checksumFileStream(fileData); | ||
return `${checksum}${fileExtension}`; | ||
} | ||
|
||
private async _addFileToAttachmentStore( | ||
store: IAttachmentStore, fileIdent: string, fileData: Buffer | ||
): Promise<AddFileResult> { | ||
const isNewFile = await this._docStorage.findOrAttachFile(fileIdent, undefined, store.id); | ||
|
||
// Verify the file exists in the store. This allows for a second attempt to correct a failed upload. | ||
const existsInRemoteStorage = !isNewFile && await store.exists(this._getDocPoolId(), fileIdent); | ||
|
||
if (!isNewFile && existsInRemoteStorage) { | ||
return { | ||
fileIdent, | ||
isNewFile: false, | ||
}; | ||
} | ||
|
||
// Possible issue if this upload fails - we have the file tracked in the document, but not available in the store. | ||
// TODO - Decide if we keep an entry in SQLite after an upload error or not. Probably not? | ||
await store.upload(this._getDocPoolId(), fileIdent, Readable.from(fileData)); | ||
|
||
// TODO - Confirm in doc storage that it's successfully uploaded? Need to decide how to handle a failed upload. | ||
paulfitz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return { | ||
fileIdent, | ||
isNewFile, | ||
}; | ||
} | ||
|
||
private async _getFileDataFromAttachmentStore(store: IAttachmentStore, fileIdent: string): Promise<Buffer> { | ||
try { | ||
const outputStream = new MemoryWritableStream(); | ||
await store.download(this._getDocPoolId(), fileIdent, outputStream); | ||
return outputStream.getBuffer(); | ||
} catch(e) { | ||
throw new AttachmentRetrievalError(store.id, fileIdent, e); | ||
} | ||
} | ||
|
||
private _getLogMeta(logInfo?: AttachmentFileManagerLogInfo): log.ILogMeta { | ||
return { | ||
docName: this._docName, | ||
docPoolId: this._docPoolId, | ||
...logInfo, | ||
}; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Stray debugging message?
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.
Yup, this and others. Thanks for catching this 😅
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.
Do you plan on removing it last minute? Just checking why it is still present.
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.
Nah, likely an oversight on my part when doing the other bits of your feedback. Glad this wasn't ticked off at least!