fix(admin): map onCallStreamEffect setup errors to HttpsErrors - #130
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Detail bug report: View on Detail
Closes #111
Bug
onCallStreamEffect(the streaming callable trigger inpackages/admin/src/lib/functions/on-call-stream.ts) did not map setup-phase schema failures to typedHttpsErrors the way its siblingonCallEffectdoes. It ran the whole effect throughrunwith the error channel cast tonever, then unconditionally rethrew whatever raw error rejected the promise.As a result, when a caller sent input that failed
inputSchema, a rawSchemaErrorescaped the handler. Thefirebase-functionsruntime collapses any non-HttpsErrorthrown by a callable to a genericHttpsError('internal', 'INTERNAL'), so the client lost both the correct code (invalid-argument) and the actionable decode message. Chunk-encoding failures againstchunkSchemacollapsed the same way — losing the developer-facing'Failed to encode function output'message. The divergence was unintentional: the streaming wrapper landed just before the sibling's setup-error recovery contract, so the contract was never applied to it.Fix
Mirrored
onCallEffect's setup-error recovery inonCallStreamEffect, restricted to the boundary phases:decode-inputfailures are wrapped asFunctionSetupError({ phase: 'decode-input' })and recovered viadefaultSetupErrorResponsetoHttpsError('invalid-argument', <schema message>); the handler and stream never run on bad input.encode-outputfailures (a chunk violatingchunkSchema) are wrapped asFunctionSetupError({ phase: 'encode-output' })and recovered toHttpsError('internal', 'Failed to encode function output').run(error channel cast tonever) torunExit, branching onExit.isSuccess, squashing the cause withCause.squash, and rethrowing — so anHttpsErrorrejection reachesfirebase-functionsintact and is serialized with its code/message rather than collapsed tointernal.!isExpectedRejection(error), soHttpsErrors andErrorReporter.ignore-annotated errors are no longer logged asDefect in onCallStream.FunctionSetupErrors still reach the caller unchanged (recovery is restricted to the boundaries, not wrapped around the handler), matching the sibling's contract.To avoid duplication, extracted
defaultSetupErrorResponseinto a new internal modulepackages/admin/src/lib/functions/recover-callable-setup-error.ts(mirrors the existingrecover-setup-error.ts@internalconvention), now imported by both wrappers. The change toon-call.tsis a pure extraction — behavior is identical.Testing
on-call-stream.spec.ts, 18 tests) — strengthened the previously-blind…rejects.toThrow()to assertinstanceof HttpsError,code === 'invalid-argument', the decode message, and that the handler did not run; added a setup-error recovery block (decode message, chunk-encode →internal/'Failed to encode function output', mid-stream chunk keeps prior chunks, handlerHttpsErrorverbatim, handlerFunctionSetupErrornot recovered) and a defect-logging block mirroringreport.spec.ts(4 cases). All pass.on-call.spec.tsandreport.spec.tspass unchanged (14 tests), confirming thedefaultSetupErrorResponseextraction is behavior-preserving.pnpm vitest run packages/adminpasses 79/79 across 8 files; no regressions in unrelated modules.pnpm nx run-many -t buildsucceeds for 8 projects;eslintandprettier --checkare clean on all changed files.firebase-functions@7.3.2onCallHandler(production HTTP entry, ESM build forHttpsErroridentity) with mocked Express req/res. Post-fix: decode-input → HTTP 400INVALID_ARGUMENTwith the decode message; encode-output → HTTP 500 with'Failed to encode function output'; handlerHttpsError('permission-denied')→ HTTP 403 verbatim; siblingonCallEffectstill → 400INVALID_ARGUMENT. Ran the same probe against the unmodified buggy baseline and reproduced the exact report (decode-input → 500{ message: 'INTERNAL', status: 'INTERNAL' }; encode-output message collapsed to'INTERNAL'), confirming the probe catches the bug and the fix resolves it.onCallStreamEffectcallable to@example/backend, startedfirebase emulators:start --only functions, and hit it withcurl: bad input ({ count: 'nope' }) → HTTP 400INVALID_ARGUMENTExpected number at ["count"]; happy path ({ count: 3 }) → HTTP 200 with[{index:0},{index:1},{index:2}];grep -c "Defect in onCallStream"over the emulator log returned0for the bad-input call. Temporary example changes reverted afterward; the example app is untouched in the final tree.Notes
typechecknx target fails in this environment on theeffect-firebasedependency package with 29 pre-existing errors unrelated to this change (e.g.firestore-service.jsresolution). Verified identical on the unmodified baseline (git stash+ re-run); admin's own lib typechecks cleanly via itsbuildtarget.onSetupErroroption foronCallStreamEffect(full sibling parity for custom mid-stream chunk-encode recovery) is deliberately out of scope — it has non-trivial semantics for a chunk that fails partway through a stream and is not required to fix the reported wire-level collapse. Left to a future feature.Automatic Fixes PRs can be configured here.