Repository navigation
fix(emulator): keep firebase-admin statics when proxying; don't crash logging circular thrown values #11460
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
fix(emulator): keep firebase-admin statics when proxying; don't crash logging circular thrown values #11460
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 |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { expect } from "chai"; | ||
|
|
||
| import { Proxied } from "./functionsEmulatorRuntime"; | ||
|
|
||
| describe("Proxied.getOriginal", () => { | ||
| it("keeps own static properties of bound functions", () => { | ||
| // Mirrors firebase-admin 13, where `admin.firestore` is an arrow function | ||
| // (no prototype) with members attached to it. | ||
| const firestore: any = () => "instance"; | ||
| firestore.Timestamp = class Timestamp {}; | ||
| firestore.FieldValue = { serverTimestamp: () => "ts" }; | ||
| Object.defineProperty(firestore, "lazy", { get: () => 42, enumerable: true }); | ||
|
|
||
| const bound = Proxied.getOriginal({ firestore }, "firestore"); | ||
|
|
||
| expect(bound()).to.equal("instance"); | ||
| expect(bound.Timestamp).to.equal(firestore.Timestamp); | ||
| expect(bound.FieldValue.serverTimestamp()).to.equal("ts"); | ||
| expect(bound.lazy).to.equal(42); | ||
| }); | ||
|
|
||
| it("still binds methods to the target", () => { | ||
| const target = { | ||
| value: 7, | ||
| get() { | ||
| return this.value; | ||
| }, | ||
| }; | ||
| expect(Proxied.getOriginal(target, "get")()).to.equal(7); | ||
| }); | ||
|
|
||
| it("returns constructors and non-functions untouched", () => { | ||
| class Foo {} | ||
| expect(Proxied.getOriginal({ Foo }, "Foo")).to.equal(Foo); | ||
| expect(Proxied.getOriginal({ a: 1 }, "a")).to.equal(1); | ||
| expect(Proxied.getOriginal({}, "missing")).to.equal(undefined); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,7 +95,7 @@ interface ProxyTarget extends Object { | |
| obj.value === 1; | ||
| obj.incremented === 2; | ||
| */ | ||
| class Proxied<T extends ProxyTarget> { | ||
| export class Proxied<T extends ProxyTarget> { | ||
| /** | ||
| * Gets a property from the original object. | ||
| */ | ||
|
|
@@ -107,10 +107,30 @@ class Proxied<T extends ProxyTarget> { | |
| } else if (Proxied.isConstructor(value) || typeof value !== "function") { | ||
| return value; | ||
| } else { | ||
| return value.bind(target); | ||
| return Proxied.bindKeepingStatics(value, target); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Binds a function to a target while preserving the function's own static | ||
| * properties. Function.prototype.bind drops them, which breaks namespaces | ||
| * that are functions with attached members (e.g. firebase-admin's | ||
| * `admin.firestore.Timestamp`). | ||
| */ | ||
| private static bindKeepingStatics(fn: (...args: any[]) => any, target: any): any { | ||
| const bound = fn.bind(target); | ||
| for (const key of Reflect.ownKeys(fn)) { | ||
| if (key === "length" || key === "name" || key === "prototype") { | ||
| continue; | ||
| } | ||
|
Comment on lines
+123
to
+125
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. In strict mode, accessing or defining the if (
key === "length" ||
key === "name" ||
key === "prototype" ||
key === "arguments" ||
key === "caller"
) {
continue;
} |
||
| const desc = Object.getOwnPropertyDescriptor(fn, key); | ||
| if (desc) { | ||
| Object.defineProperty(bound, key, desc); | ||
| } | ||
| } | ||
| return bound; | ||
| } | ||
|
|
||
| /** | ||
| * Run the original target. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||
| import { inspect } from "util"; | ||||||||||||||
| import { defaultTo } from "lodash"; | ||||||||||||||
|
|
||||||||||||||
| interface FirebaseErrorOptions { | ||||||||||||||
|
|
@@ -47,7 +48,12 @@ export function getErrMsg(err: unknown, defaultMsg?: string): string { | |||||||||||||
| } else if (defaultMsg) { | ||||||||||||||
| return defaultMsg; | ||||||||||||||
| } | ||||||||||||||
| return JSON.stringify(err); | ||||||||||||||
| try { | ||||||||||||||
| return JSON.stringify(err); | ||||||||||||||
| } catch { | ||||||||||||||
|
Comment on lines
+51
to
+53
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.
Suggested change
|
||||||||||||||
| // e.g. a circular structure; fall back to something that cannot throw. | ||||||||||||||
| return inspect(err); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
|
|
||||||||||||||
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.
To adhere to the repository style guide which states 'Never use
anyorunknownas an escape hatch', we can avoid usinganyhere by usingObject.assignto combine the function and its static properties. This allows TypeScript to infer the correct intersection type automatically.References