-
Notifications
You must be signed in to change notification settings - Fork 431
fix(storage-resize-images): restore the extension's default for an omitted deleteOriginal #3100
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
base: kits
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| - fix: restore the extension's default for an omitted `deleteOriginal`. The extension mapped every `DELETE_ORIGINAL_FILE` value other than `"true"`/`"false"` (unset included) to delete-on-success; the kit resolved an omitted `deleteOriginal` to never-delete. `resolveResizeImagesConfig` now resolves an omitted `deleteOriginal` to delete-on-success, matching the extension. Only library consumers who omit the field are affected; deploys are unaffected (the Firebase CLI writes the declared `"false"` default into `.env`, and an env var that is present resolves as before). Pass `deleteOriginal: "false"` to keep originals. | ||
| - fix: restore the `us-central1` content-filter fallback. `checkImageContent` threw `FUNCTION_REGION is required for Vertex AI filtering.` when no region was available; the extension fell back to `us-central1`. The Vertex AI call now uses the function's region when known and `us-central1` otherwise, matching the extension. Normal CLI deploys were unaffected (the Firebase CLI sets `FUNCTION_REGION` on deployed functions); the throw was reachable for library consumers, emulator runs, and hand-rolled environments. | ||
| - Initial release of kit, see README for differences between the legacy extension and this kit |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,12 @@ loads them at deploy time and prompts for any required values that are missing. | |
| | `customFilterPrompt` | `CUSTOM_FILTER_PROMPT` | no | (empty) | Custom filter prompt | | ||
| | `placeholderImagePath` | `PLACEHOLDER_IMAGE_PATH` | no | (empty) | Placeholder for filtered images | | ||
|
|
||
| The `deleteOriginal` default above is what the CLI writes into `.env` at deploy | ||
| time. Omitting `deleteOriginal` when calling `resolveResizeImagesConfig` | ||
| directly deletes the original on a successful resize, matching how the | ||
| extension treated an unset `DELETE_ORIGINAL_FILE`; pass `"false"` to keep | ||
| originals. | ||
|
|
||
| ## Multiple instances | ||
|
|
||
| To resize images from several buckets or pipelines, add one entry per instance | ||
|
|
@@ -180,6 +186,16 @@ the extension is installed. A malformed value fails the deploy with | |
| `Invalid includePathList: must be a comma-separated list of absolute path | ||
| values.` rather than being rejected by an install prompt. | ||
|
|
||
| ### An omitted `isAnimated` keeps animation | ||
|
|
||
| The extension's config parser had a bug: `overrideIsAnimated === "true" || | ||
| undefined` never evaluated the intended unset check, so an unset `IS_ANIMATED` | ||
| produced first-frame-only output even though the parameter's declared default | ||
| was `true`. The kit deliberately fixes this rather than reproducing it: an | ||
| omitted `isAnimated` resolves to `true`, the default the extension intended. | ||
| Deploys are unaffected either way, since the CLI writes `IS_ANIMATED=true` | ||
|
Contributor
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. Non-blocking, and also cheap here. |
||
| into `.env`; pass `isAnimated: false` for first-frame-only output. | ||
|
|
||
| ### No backfill | ||
|
|
||
| There is no function to resize images that already exist in the bucket. The | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,7 +127,6 @@ function deleteOriginalFile( | |
| return DELETE_IMAGE.always; | ||
| case false: | ||
| case "false": | ||
| case undefined: | ||
| return DELETE_IMAGE.never; | ||
|
Contributor
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. With |
||
| default: | ||
| return DELETE_IMAGE.onSuccess; | ||
|
Contributor
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. Non-blocking. The |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,7 @@ import { checkImageContent } from "../src/content-filter"; | |
| import * as events from "../src/events"; | ||
| import { | ||
| DELETE_IMAGE, | ||
| type ResizeImagesConfig, | ||
| type ResolvedResizeImagesConfig, | ||
| resolveResizeImagesConfig, | ||
| } from "../src/export-config"; | ||
|
|
@@ -97,16 +98,22 @@ const mock = <T>(fn: T) => fn as unknown as ReturnType<typeof vi.fn>; | |
|
|
||
| const bucketStub = {}; | ||
|
|
||
| // deleteOriginal is pinned: an omitted value resolves to on_success, and | ||
| // these tests exercise handler logic, not the resolver's defaults. | ||
| const baseInput: ResizeImagesConfig = { | ||
| bucket: "demo-bucket", | ||
| sizes: "200x200", | ||
| region: "us-central1", | ||
| deleteOriginal: "false", | ||
| }; | ||
|
|
||
| function makeCtx( | ||
| overrides: Partial<ResolvedResizeImagesConfig> = {} | ||
| overrides: Partial<ResolvedResizeImagesConfig> = {}, | ||
| input: ResizeImagesConfig = baseInput | ||
| ): HandlerContext { | ||
| return { | ||
| config: { | ||
| ...resolveResizeImagesConfig({ | ||
| bucket: "demo-bucket", | ||
| sizes: "200x200", | ||
| region: "us-central1", | ||
| }), | ||
| ...resolveResizeImagesConfig(input), | ||
| ...overrides, | ||
| }, | ||
| storage: { | ||
|
|
@@ -390,6 +397,62 @@ describe("generateResizedImageHandler", () => { | |
| expect(deleteRemoteFile).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("an omitted deleteOriginal deletes the original after a successful run", async () => { | ||
| // The extension resolved an unset DELETE_ORIGINAL_FILE to on_success. | ||
| const remoteFile = { delete: vi.fn() }; | ||
| mock(downloadOriginalFile).mockResolvedValue(["/tmp/test.jpg", remoteFile]); | ||
| const ctx = makeCtx( | ||
| {}, | ||
| { bucket: "demo-bucket", sizes: "200x200", region: "us-central1" } | ||
| ); | ||
|
|
||
| await generateResizedImageHandler(mockObject, ctx, false); | ||
|
|
||
| expect(deleteRemoteFile).toHaveBeenCalledWith( | ||
| remoteFile, | ||
| "images/test.jpg" | ||
| ); | ||
| expect(deleteRemoteFile).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("an omitted deleteOriginal deletes a filter-blocked original once its placeholder resizes", async () => { | ||
| // Matches the extension: the blocked original is replaced and then removed | ||
| // under on_success, unless failedImagesPath stored a copy first. | ||
| const remoteFile = { delete: vi.fn() }; | ||
| mock(downloadOriginalFile).mockResolvedValue(["/tmp/test.jpg", remoteFile]); | ||
| mock(checkImageContent).mockResolvedValue(false); | ||
| const ctx = makeCtx( | ||
| {}, | ||
| { bucket: "demo-bucket", sizes: "200x200", region: "us-central1" } | ||
| ); | ||
|
|
||
| await generateResizedImageHandler(mockObject, ctx, false); | ||
|
|
||
| expect(deleteRemoteFile).toHaveBeenCalledWith( | ||
| remoteFile, | ||
| "images/test.jpg" | ||
| ); | ||
| expect(deleteRemoteFile).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("an omitted deleteOriginal keeps the original on a failed run", async () => { | ||
|
Contributor
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. Non-blocking: this one passes with the source change reverted, because an omitted |
||
| mock(downloadOriginalFile).mockResolvedValue([ | ||
| "/tmp/test.jpg", | ||
| { delete: vi.fn() }, | ||
| ]); | ||
| mock(resizeImages).mockResolvedValue([ | ||
| { status: "fulfilled", value: { success: false } }, | ||
| ]); | ||
| const ctx = makeCtx( | ||
| {}, | ||
| { bucket: "demo-bucket", sizes: "200x200", region: "us-central1" } | ||
| ); | ||
|
|
||
| await generateResizedImageHandler(mockObject, ctx, false); | ||
|
|
||
| expect(deleteRemoteFile).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("cleans up the temp files it created", async () => { | ||
| const ctx = makeCtx(); | ||
| mock(checkImageContent).mockResolvedValue(false); | ||
|
|
||
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.
Non-blocking, and cheap to fix in this PR. Reading firebase-tools 15.29.0 (
lib/deploy/functions/params.js,resolveParams), a param absent from.envis prompted for with its declared default and the resolved value is injected into the function environment, and a non-interactive deploy fails outright instead. Nothing is written back into.env, so the deploy-safety conclusion holds but by a different route than this sentence describes. Also, the table above still lists thedeleteOriginaldefault asfalse, so a reader who only scans the table takes away the opposite of the new behaviour; a footnote on that row pointing down here would close the gap. This comes from reading the dependency source, not from a live deploy.