Skip to content

Enables snapshot external storage provider by default #1798

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 12, 2025
Merged

Enables snapshot external storage provider by default #1798

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12b1507
Enables snapshot external storage provider by default
Spoffy Aug 29, 2025
e21af60
Adds warning and errors for external storage modes
Spoffy Sep 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions app/server/lib/AttachmentStoreProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ export async function makeTempFilesystemStoreSpec(
}

const settings = appSettings.section("attachmentStores");
const GRIST_EXTERNAL_ATTACHMENTS_MODE = settings.flag("mode").readString({
const GRIST_EXTERNAL_ATTACHMENTS_MODE = settings.flag("mode").requireString({
envVar: "GRIST_EXTERNAL_ATTACHMENTS_MODE",
defaultValue: "none",
defaultValue: "snapshots",
});

export function getConfiguredStandardAttachmentStore(): string | undefined {
Expand All @@ -168,15 +168,22 @@ export function getConfiguredStandardAttachmentStore(): string | undefined {
}
}

export class UnsupportedExternalAttachmentsMode extends Error {
constructor(storeType: string) {
super(`Unsupported external attachments mode on this version of Grist: ${storeType}`);
}
}

export async function getConfiguredAttachmentStoreConfigs(): Promise<IAttachmentStoreConfig[]> {
if (GRIST_EXTERNAL_ATTACHMENTS_MODE === 'snapshots') {
const snapshotProvider = create.getAttachmentStoreOptions().snapshots;
// This shouldn't happen - it could only happen if a version of Grist removes the snapshot provider from ICreate.
Copy link
Member

@paulfitz paulfitz Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment now out of date?

Copy link
Contributor Author

@Spoffy Spoffy Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still valid here, just a very unlikely edge case. This could only happen if someone makes a custom fork of Grist / removes the snapshot provider support entirely.

Copy link
Member

@paulfitz paulfitz Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be reasonable to continue to throw at the site immediately below then? If it really should never happen? I'm confused then.

Copy link
Contributor Author

@Spoffy Spoffy Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

50/50 - I think either would make sense here.

The overall change here is moving from "Error when snapshots aren't available but requested" to "Do nothing when snapshots are requested but not available".

This could easily throw here given it's unlikely to ever come up, and if it is, it's indicative of a code issue rather than a config one.

I defaulted I think mostly to keep the semantics here consistent across all config values (e.g. GRIST_EXTERNAL_ATTACHMENTS_MODE="gobbledygook" won't error).

I'm happy with either - error when ATTACHMENTS_MODE is set to something the Grist version doesn't support, or quietly ignore it. What do you think?

Copy link
Member

@paulfitz paulfitz Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing for things that shouldn't happen has saved us many times when they do, so that has my vote.

The desired behavior in this change is, if a user makes a grist installation and configures external storage, external attachments become available using that store (if there's no specific configuration to counter that). It throwing here is consistent with that outcome, I vote for keeping it.

Copy link
Contributor Author

@Spoffy Spoffy Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thy will be thrown.

if (snapshotProvider === undefined) {
throw new Error("Snapshot provider is not available on this version of Grist");
throw new UnsupportedExternalAttachmentsMode("snapshots");
}
if (!(await isAttachmentStoreOptionAvailable(snapshotProvider))) {
throw new Error("The currently configured external storage does not support attachments");
log.warn("External attachment store 'snapshots' requested, but no snapshot storage is configured.");
return [];
}
return [{
label: 'snapshots',
Expand All @@ -190,5 +197,9 @@ export async function getConfiguredAttachmentStoreConfigs(): Promise<IAttachment
spec: await makeTempFilesystemStoreSpec(),
}];
}
return [];
if (!GRIST_EXTERNAL_ATTACHMENTS_MODE) {
return [];
}
// GRIST_EXTERNAL_ATTACHMENTS_MODE has some value that doesn't make sense.
throw new UnsupportedExternalAttachmentsMode(GRIST_EXTERNAL_ATTACHMENTS_MODE);
}