Skip to content

Fixes encoding of circular referenced object / complex object #1525

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+47 −12
Open

Fixes encoding of circular referenced object / complex object #1525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74d205c
Add encoding tests And implementation for circular references
DavidWeiss2 Feb 20, 2024
2f92f08
Add encoding test case for circular reference in nested object
DavidWeiss2 Feb 20, 2024
1cd966d
Fix encoding issue and clean up weakmap
DavidWeiss2 Feb 20, 2024
1922293
Fix circular reference encoding in https.spec.ts
DavidWeiss2 Feb 20, 2024
53e5b66
Refactor encode function to use WeakSet for self reference tracking, …
DavidWeiss2 Mar 22, 2024
c68b120
Merge branch 'master' into master
DavidWeiss2 Mar 22, 2024
b5f375d
Merge branch 'master' into master
DavidWeiss2 Nov 14, 2024
37e41ac
Merge branch 'master' into master
DavidWeiss2 Nov 26, 2024
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
15 changes: 15 additions & 0 deletions spec/common/providers/https.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,21 @@ describe("encoding/decoding", () => {
});
});

it("Throws object with self reference", () => {
class TestClass {
foo: string;
bar: number;
self: TestClass;
constructor(foo: string, bar: number) {
this.foo = foo;
this.bar = bar;
this.self = this;
}
}
const testObject = new TestClass("hello", 1);
expect(()=>https.encode(testObject)).to.throw(`Data cannot be encoded in JSON: ${testObject}`);
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Missing spaces around the arrow function. Should be () => https.encode(testObject) for consistency with project style (see line 968).

Suggested change
expect(()=>https.encode(testObject)).to.throw(`Data cannot be encoded in JSON: ${testObject}`);
expect(() => https.encode(testObject)).to.throw(`Data cannot be encoded in JSON: ${testObject}`);

Copilot uses AI. Check for mistakes.
});

it("encodes function as an empty object", () => {
expect(https.encode(() => "foo")).to.deep.equal({});
});
Expand Down
44 changes: 32 additions & 12 deletions src/common/providers/https.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,14 @@ const LONG_TYPE = "type.googleapis.com/google.protobuf.Int64Value";
/** @hidden */
const UNSIGNED_LONG_TYPE = "type.googleapis.com/google.protobuf.UInt64Value";

/** @hidden */
const SELF_REFERENCE_WEAKSET = new WeakSet<object|((...args:any[])=>any)>();
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The type definition object|((...args:any[])=>any) is overly verbose and redundant. Functions are already objects in JavaScript, so WeakSet<object> would suffice. Additionally, the type annotation is unnecessary here since TypeScript can infer it.

Suggested change
const SELF_REFERENCE_WEAKSET = new WeakSet<object|((...args:any[])=>any)>();
const SELF_REFERENCE_WEAKSET = new WeakSet();

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Using a module-level WeakSet for tracking circular references creates a potential issue in concurrent request scenarios. If multiple encode calls execute simultaneously (common in serverless environments), they will share the same WeakSet, potentially causing false positives where an object from one request is detected as circular in another. The WeakSet should be passed as a parameter through recursive calls or use a different approach for circular reference detection.

Copilot uses AI. Check for mistakes.
/**
* Encodes arbitrary data in our special format for JSON.
* This is exposed only for testing.
*/
/** @hidden */
export function encode(data: any): any {
export function encode(data: unknown): any {
if (data === null || typeof data === "undefined") {
return null;
}
Expand All @@ -430,18 +432,36 @@ export function encode(data: any): any {
if (Array.isArray(data)) {
return data.map(encode);
}
if (typeof data === "object" || typeof data === "function") {
// Sadly we don't have Object.fromEntries in Node 10, so we can't use a single
// list comprehension
const obj: Record<string, any> = {};
for (const [k, v] of Object.entries(data)) {
obj[k] = encode(v);
}
return obj;
if(!isFunctionOrObject(data)||SELF_REFERENCE_WEAKSET.has(data)) {
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Missing spaces around operators. Should be if (!isFunctionOrObject(data) || SELF_REFERENCE_WEAKSET.has(data)) for consistency with project style.

Suggested change
if(!isFunctionOrObject(data)||SELF_REFERENCE_WEAKSET.has(data)) {
if (!isFunctionOrObject(data) || SELF_REFERENCE_WEAKSET.has(data)) {

Copilot uses AI. Check for mistakes.
logger.error("Data cannot be encoded in JSON.", data);
throw new Error(`Data cannot be encoded in JSON: ${data}`);
}
Comment on lines +435 to +438
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The logic is inverted. The condition !isFunctionOrObject(data) returns true when data is NOT an object/function (which is correct to throw), but combining it with || SELF_REFERENCE_WEAKSET.has(data) means it will also throw when a circular reference is detected AND when data is not an object. This breaks the existing test at line 967 which expects functions to encode as empty objects {}. The condition should be split: handle non-object/function first, then check for circular references separately.

Suggested change
if(!isFunctionOrObject(data)||SELF_REFERENCE_WEAKSET.has(data)) {
logger.error("Data cannot be encoded in JSON.", data);
throw new Error(`Data cannot be encoded in JSON: ${data}`);
}
if (!isFunctionOrObject(data)) {
logger.error("Data cannot be encoded in JSON.", data);
throw new Error(`Data cannot be encoded in JSON: ${data}`);
}
if (SELF_REFERENCE_WEAKSET.has(data)) {
logger.error("Data cannot be encoded in JSON (circular reference detected).", data);
throw new Error(`Data cannot be encoded in JSON (circular reference detected): ${data}`);
}

Copilot uses AI. Check for mistakes.

// Sadly we don't have Object.fromEntries in Node 10, so we can't use a single
// list comprehension
const obj: Record<string, any> = {};

SELF_REFERENCE_WEAKSET.add(data);

for (const [k, v] of Object.entries(data)) {
obj[k] = encode(v);
}
// If we got this far, the data is not encodable.
logger.error("Data cannot be encoded in JSON.", data);
throw new Error(`Data cannot be encoded in JSON: ${data}`);

// clean after recursive call -
// we don't want to keep references to objects that are not part of the current object
SELF_REFERENCE_WEAKSET.delete(data);

return obj;
}

function isFunctionOrObject(data: unknown): data is object|((...args:any[])=>any) {
const isObjectOrFunction = typeof data === "object" || typeof data === "function";

if (!isObjectOrFunction) {
// If we got this far, the data is not encodable.
return false;
}
return true;
Comment on lines +457 to +464
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The type predicate object|((...args:any[])=>any) is redundant. Since functions are objects in JavaScript, the predicate should be data is object. Additionally, this function could be simplified to a single line: return typeof data === 'object' || typeof data === 'function';

Suggested change
function isFunctionOrObject(data: unknown): data is object|((...args:any[])=>any) {
const isObjectOrFunction = typeof data === "object" || typeof data === "function";
if (!isObjectOrFunction) {
// If we got this far, the data is not encodable.
return false;
}
return true;
function isFunctionOrObject(data: unknown): data is object {
return typeof data === "object" || typeof data === "function";

Copilot uses AI. Check for mistakes.
Comment on lines +458 to +464
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

This function is unnecessarily complex. The comment 'If we got this far, the data is not encodable' is misleading since the function just checks if data is an object or function, not whether it's encodable. The entire function can be simplified to: return typeof data === 'object' || typeof data === 'function';

Suggested change
const isObjectOrFunction = typeof data === "object" || typeof data === "function";
if (!isObjectOrFunction) {
// If we got this far, the data is not encodable.
return false;
}
return true;
return typeof data === "object" || typeof data === "function";

Copilot uses AI. Check for mistakes.
}

/**
Expand Down
Loading