From 74d205c4a125cd2a43d856e9b1f5e10f6f9852fe Mon Sep 17 00:00:00 2001 From: David Weiss Date: Tue, 20 Feb 2024 10:13:27 +0200 Subject: [PATCH 1/5] Add encoding tests And implementation for circular references --- spec/common/providers/https.spec.ts | 62 +++++++++++++++++++++++++++++ src/common/providers/https.ts | 38 ++++++++++++------ 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/spec/common/providers/https.spec.ts b/spec/common/providers/https.spec.ts index 9ea0f4f79..db13d5e0c 100644 --- a/spec/common/providers/https.spec.ts +++ b/spec/common/providers/https.spec.ts @@ -869,6 +869,68 @@ describe("encoding/decoding", () => { }); }); + it("encodes 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.deep.equal({ + foo: "hello", + bar: 1, + self: { foo: "hello", bar: 1 }, + }); + }); + + it("encodes object with circular reference", () => { + class TestClass { + foo: string; + bar: number; + self: TestClass; + constructor(foo: string, bar: number) { + this.foo = foo; + this.bar = bar; + this.self = this; + } + } + const testObject1 = new TestClass("hello", 1); + const testObject2 = new TestClass("world", 2); + testObject1.self = testObject2; + testObject2.self = testObject1; + expect(https.encode(testObject1)).to.deep.equal({ + foo: "hello", + bar: 1, + self: { foo: "world", bar: 2, self: { foo: "hello", bar: 1 } }, + }); + }) + + it("encodes object with circular reference in nested object", () => { + class TestClass { + foo: string; + bar: number; + nested: { + self: TestClass; + }; + constructor(foo: string, bar: number) { + this.foo = foo; + this.bar = bar; + this.nested = { self: this }; + } + } + const testObject = new TestClass("hello", 1); + expect(https.encode(testObject)).to.deep.equal({ + foo: "hello", + bar: 1, + nested: { self: { foo: "hello", bar: 1 } }, + }); + }); + it("encodes function as an empty object", () => { expect(https.encode(() => "foo")).to.deep.equal({}); }); diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 2f0e56538..9e4040888 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -395,12 +395,16 @@ const LONG_TYPE = "type.googleapis.com/google.protobuf.Int64Value"; /** @hidden */ const UNSIGNED_LONG_TYPE = "type.googleapis.com/google.protobuf.UInt64Value"; +/** @hidden */ +const SELF_REFERENCE_WEAKMAP = new WeakMap(); /** * 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; } @@ -421,18 +425,28 @@ 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 = {}; - for (const [k, v] of Object.entries(data)) { - obj[k] = encode(v); - } - return obj; + const isObjectOrFunction = typeof data === "object" || typeof data === "function"; + + if (!isObjectOrFunction) { + // 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}`); + } + // implementation of an hidden argument to keep track of objects that have been encoded + if (SELF_REFERENCE_WEAKMAP.has(data)) { + return { ...SELF_REFERENCE_WEAKMAP.get(data) }; // return a shallow copy of the object + } + + // Sadly we don't have Object.fromEntries in Node 10, so we can't use a single + // list comprehension + const obj: Record = {}; + + SELF_REFERENCE_WEAKMAP.set(data, obj); + + 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}`); + return obj; } /** From 2f92f08cadfa9e0fdf00d933bb72f5796913977d Mon Sep 17 00:00:00 2001 From: David Weiss Date: Tue, 20 Feb 2024 13:06:15 +0200 Subject: [PATCH 2/5] Add encoding test case for circular reference in nested object --- spec/common/providers/https.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/common/providers/https.spec.ts b/spec/common/providers/https.spec.ts index db13d5e0c..4bd4537f4 100644 --- a/spec/common/providers/https.spec.ts +++ b/spec/common/providers/https.spec.ts @@ -908,6 +908,11 @@ describe("encoding/decoding", () => { bar: 1, self: { foo: "world", bar: 2, self: { foo: "hello", bar: 1 } }, }); + expect(https.encode(testObject2)).to.deep.equal({ + foo: "world", + bar: 2, + self: { foo: "hello", bar: 1, self: { foo: "world", bar: 2 } }, + }); }) it("encodes object with circular reference in nested object", () => { From 1cd966d7535d7762735e76908c00a65aa4d969a7 Mon Sep 17 00:00:00 2001 From: David Weiss Date: Tue, 20 Feb 2024 13:11:08 +0200 Subject: [PATCH 3/5] Fix encoding issue and clean up weakmap --- spec/common/providers/https.spec.ts | 2 +- src/common/providers/https.ts | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/spec/common/providers/https.spec.ts b/spec/common/providers/https.spec.ts index 4bd4537f4..e42f40043 100644 --- a/spec/common/providers/https.spec.ts +++ b/spec/common/providers/https.spec.ts @@ -913,7 +913,7 @@ describe("encoding/decoding", () => { bar: 2, self: { foo: "hello", bar: 1, self: { foo: "world", bar: 2 } }, }); - }) + }); it("encodes object with circular reference in nested object", () => { class TestClass { diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 9e4040888..2f67bdd4b 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -396,15 +396,13 @@ const LONG_TYPE = "type.googleapis.com/google.protobuf.Int64Value"; const UNSIGNED_LONG_TYPE = "type.googleapis.com/google.protobuf.UInt64Value"; /** @hidden */ -const SELF_REFERENCE_WEAKMAP = new WeakMap(); +const SELF_REFERENCE_WEAKMAP = new WeakMap(); /** * Encodes arbitrary data in our special format for JSON. * This is exposed only for testing. */ /** @hidden */ -export function encode( - data: unknown -): any { +export function encode(data: unknown): any { if (data === null || typeof data === "undefined") { return null; } @@ -446,6 +444,11 @@ export function encode( for (const [k, v] of Object.entries(data)) { obj[k] = encode(v); } + + // clean after recursive call - + // we don't want to keep references to objects that are not part of the current object + SELF_REFERENCE_WEAKMAP.delete(data); + return obj; } From 1922293bc9c0eae74739b6cab1417540aa029025 Mon Sep 17 00:00:00 2001 From: David Weiss Date: Tue, 20 Feb 2024 13:28:23 +0200 Subject: [PATCH 4/5] Fix circular reference encoding in https.spec.ts --- spec/common/providers/https.spec.ts | 28 ++++++++++++++++++++++++++++ src/common/providers/https.ts | 4 ++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/spec/common/providers/https.spec.ts b/spec/common/providers/https.spec.ts index e42f40043..4160e901c 100644 --- a/spec/common/providers/https.spec.ts +++ b/spec/common/providers/https.spec.ts @@ -936,6 +936,34 @@ describe("encoding/decoding", () => { }); }); + it("encodes object with circular reference without histical data", () => { + class TestClass { + foo: string; + bar: number; + self: TestClass | undefined; + constructor(foo: string, bar: number) { + this.foo = foo; + this.bar = bar; + } + } + const testObject = new TestClass("hello", 1); + expect(https.encode(testObject)).to.deep.equal({ + foo: "hello", + bar: 1, + }); + testObject.self = testObject; + expect(https.encode(testObject)).to.deep.equal({ + foo: "hello", + bar: 1, + self: { foo: "hello", bar: 1 }, + }); + delete testObject.self; + expect(https.encode(testObject)).to.deep.equal({ + foo: "hello", + bar: 1, + }); + }); + it("encodes function as an empty object", () => { expect(https.encode(() => "foo")).to.deep.equal({}); }); diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 2f67bdd4b..7f8bff98d 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -396,7 +396,7 @@ const LONG_TYPE = "type.googleapis.com/google.protobuf.Int64Value"; const UNSIGNED_LONG_TYPE = "type.googleapis.com/google.protobuf.UInt64Value"; /** @hidden */ -const SELF_REFERENCE_WEAKMAP = new WeakMap(); +const SELF_REFERENCE_WEAKMAP = new WeakMap(); /** * Encodes arbitrary data in our special format for JSON. * This is exposed only for testing. @@ -445,7 +445,7 @@ export function encode(data: unknown): any { obj[k] = encode(v); } - // clean after recursive call - + // clean after recursive call - // we don't want to keep references to objects that are not part of the current object SELF_REFERENCE_WEAKMAP.delete(data); From 53e5b661ebc1c48ced339d0119fdd45ede552629 Mon Sep 17 00:00:00 2001 From: David Weiss Date: Sat, 23 Mar 2024 00:15:48 +0200 Subject: [PATCH 5/5] Refactor encode function to use WeakSet for self reference tracking, and to throw on this case --- spec/common/providers/https.spec.ts | 84 +---------------------------- src/common/providers/https.ts | 27 +++++----- 2 files changed, 17 insertions(+), 94 deletions(-) diff --git a/spec/common/providers/https.spec.ts b/spec/common/providers/https.spec.ts index 4160e901c..3535f105e 100644 --- a/spec/common/providers/https.spec.ts +++ b/spec/common/providers/https.spec.ts @@ -869,7 +869,7 @@ describe("encoding/decoding", () => { }); }); - it("encodes object with self reference", () => { + it("Throws object with self reference", () => { class TestClass { foo: string; bar: number; @@ -881,87 +881,7 @@ describe("encoding/decoding", () => { } } const testObject = new TestClass("hello", 1); - expect(https.encode(testObject)).to.deep.equal({ - foo: "hello", - bar: 1, - self: { foo: "hello", bar: 1 }, - }); - }); - - it("encodes object with circular reference", () => { - class TestClass { - foo: string; - bar: number; - self: TestClass; - constructor(foo: string, bar: number) { - this.foo = foo; - this.bar = bar; - this.self = this; - } - } - const testObject1 = new TestClass("hello", 1); - const testObject2 = new TestClass("world", 2); - testObject1.self = testObject2; - testObject2.self = testObject1; - expect(https.encode(testObject1)).to.deep.equal({ - foo: "hello", - bar: 1, - self: { foo: "world", bar: 2, self: { foo: "hello", bar: 1 } }, - }); - expect(https.encode(testObject2)).to.deep.equal({ - foo: "world", - bar: 2, - self: { foo: "hello", bar: 1, self: { foo: "world", bar: 2 } }, - }); - }); - - it("encodes object with circular reference in nested object", () => { - class TestClass { - foo: string; - bar: number; - nested: { - self: TestClass; - }; - constructor(foo: string, bar: number) { - this.foo = foo; - this.bar = bar; - this.nested = { self: this }; - } - } - const testObject = new TestClass("hello", 1); - expect(https.encode(testObject)).to.deep.equal({ - foo: "hello", - bar: 1, - nested: { self: { foo: "hello", bar: 1 } }, - }); - }); - - it("encodes object with circular reference without histical data", () => { - class TestClass { - foo: string; - bar: number; - self: TestClass | undefined; - constructor(foo: string, bar: number) { - this.foo = foo; - this.bar = bar; - } - } - const testObject = new TestClass("hello", 1); - expect(https.encode(testObject)).to.deep.equal({ - foo: "hello", - bar: 1, - }); - testObject.self = testObject; - expect(https.encode(testObject)).to.deep.equal({ - foo: "hello", - bar: 1, - self: { foo: "hello", bar: 1 }, - }); - delete testObject.self; - expect(https.encode(testObject)).to.deep.equal({ - foo: "hello", - bar: 1, - }); + expect(()=>https.encode(testObject)).to.throw(`Data cannot be encoded in JSON: ${testObject}`); }); it("encodes function as an empty object", () => { diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 7f8bff98d..30e702065 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -396,7 +396,7 @@ const LONG_TYPE = "type.googleapis.com/google.protobuf.Int64Value"; const UNSIGNED_LONG_TYPE = "type.googleapis.com/google.protobuf.UInt64Value"; /** @hidden */ -const SELF_REFERENCE_WEAKMAP = new WeakMap(); +const SELF_REFERENCE_WEAKSET = new WeakSetany)>(); /** * Encodes arbitrary data in our special format for JSON. * This is exposed only for testing. @@ -423,23 +423,16 @@ export function encode(data: unknown): any { if (Array.isArray(data)) { return data.map(encode); } - const isObjectOrFunction = typeof data === "object" || typeof data === "function"; - - if (!isObjectOrFunction) { - // If we got this far, the data is not encodable. + 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}`); } - // implementation of an hidden argument to keep track of objects that have been encoded - if (SELF_REFERENCE_WEAKMAP.has(data)) { - return { ...SELF_REFERENCE_WEAKMAP.get(data) }; // return a shallow copy of the object - } - + // Sadly we don't have Object.fromEntries in Node 10, so we can't use a single // list comprehension const obj: Record = {}; - SELF_REFERENCE_WEAKMAP.set(data, obj); + SELF_REFERENCE_WEAKSET.add(data); for (const [k, v] of Object.entries(data)) { obj[k] = encode(v); @@ -447,11 +440,21 @@ export function encode(data: unknown): any { // clean after recursive call - // we don't want to keep references to objects that are not part of the current object - SELF_REFERENCE_WEAKMAP.delete(data); + 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; +} + /** * Decodes our special format for JSON into native types. * This is exposed only for testing.