Skip to content

Fix: Handle circular references in flattenAttributes function #1433

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

Merged
merged 7 commits into from
Oct 24, 2024
Merged

Fix: Handle circular references in flattenAttributes function #1433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6915d9e
Fix: Handle circular references in flattenAttributes function
yadavshubham01 Oct 24, 2024
f508bf5
Merge branch 'main' into Update-flattenAttributes
yadavshubham01 Oct 24, 2024
b29ba40
Update flattenAttributes.test.ts
yadavshubham01 Oct 24, 2024
e58686c
Update packages/core/src/v3/utils/flattenAttributes.ts
ericallam Oct 24, 2024
400dbb2
Update packages/core/src/v3/utils/flattenAttributes.ts
ericallam Oct 24, 2024
c908f3a
Create brown-laws-rest.md
ericallam Oct 24, 2024
cb3f598
Merge branch 'main' into Update-flattenAttributes
ericallam Oct 24, 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
5 changes: 5 additions & 0 deletions .changeset/brown-laws-rest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@trigger.dev/core": patch
---

Fix: Handle circular references in flattenAttributes function
31 changes: 27 additions & 4 deletions packages/core/src/v3/utils/flattenAttributes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { Attributes } from "@opentelemetry/api";

export const NULL_SENTINEL = "$@null((";
export const CIRCULAR_REFERENCE_SENTINEL = "$@circular((";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure the uniqueness of CIRCULAR_REFERENCE_SENTINEL to prevent data conflicts.

While CIRCULAR_REFERENCE_SENTINEL is used to indicate circular references, there's a possibility that user data may contain this exact string, leading to incorrect interpretation during unflattening. Consider using a unique symbol or a less common string to minimize the risk of conflicts with actual data values.


export function flattenAttributes(
obj: Record<string, unknown> | Array<unknown> | string | boolean | number | null | undefined,
prefix?: string
prefix?: string ,
seen: WeakSet<object> = new WeakSet()
): Attributes {
const result: Attributes = {};

Expand Down Expand Up @@ -38,13 +40,25 @@ export function flattenAttributes(
return result;
}

// Check for circular reference
if (obj !== null && typeof obj === "object" && seen.has(obj)) {
result[prefix || ""] = CIRCULAR_REFERENCE_SENTINEL;
return result;
}

// Add object to seen set
if (obj !== null && typeof obj === "object") {
seen.add(obj);
}


for (const [key, value] of Object.entries(obj)) {
const newPrefix = `${prefix ? `${prefix}.` : ""}${Array.isArray(obj) ? `[${key}]` : key}`;
if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
if (typeof value[i] === "object" && value[i] !== null) {
// update null check here as well
Object.assign(result, flattenAttributes(value[i], `${newPrefix}.[${i}]`));
Object.assign(result, flattenAttributes(value[i], `${newPrefix}.[${i}]`,seen));
} else {
if (value[i] === null) {
result[`${newPrefix}.[${i}]`] = NULL_SENTINEL;
Expand All @@ -55,7 +69,7 @@ export function flattenAttributes(
}
} else if (isRecord(value)) {
// update null check here
Object.assign(result, flattenAttributes(value, newPrefix));
Object.assign(result, flattenAttributes(value, newPrefix, seen));
} else {
if (typeof value === "number" || typeof value === "string" || typeof value === "boolean") {
result[newPrefix] = value;
Expand Down Expand Up @@ -135,8 +149,10 @@ export function unflattenAttributes(
}

const lastPart = parts[parts.length - 1];

if (lastPart !== undefined) {
current[lastPart] = rehydrateNull(value);
current[lastPart] = rehydrateNull(rehydrateCircular(value));

}
}

Expand All @@ -153,6 +169,13 @@ export function unflattenAttributes(
return result;
}

function rehydrateCircular(value: any): any {
if (value === CIRCULAR_REFERENCE_SENTINEL) {
return "[Circular Reference]";
}
return value;
}
Comment on lines +172 to +177
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider parameterizing the circular reference placeholder.

The hardcoded string "[Circular Reference]" might not be suitable for all use cases. Consider making it configurable:

-function rehydrateCircular(value: any): any {
+function rehydrateCircular(
+  value: any,
+  placeholder: string = "[Circular Reference]"
+): any {
   if (value === CIRCULAR_REFERENCE_SENTINEL) {
-    return "[Circular Reference]";
+    return placeholder;
   }
   return value;
 }

Then update the call site:

-current[lastPart] = rehydrateNull(rehydrateCircular(value));
+current[lastPart] = rehydrateNull(rehydrateCircular(value, options?.circularPlaceholder));

Committable suggestion was skipped due to low confidence.


export function primitiveValueOrflattenedAttributes(
obj: Record<string, unknown> | Array<unknown> | string | boolean | number | undefined,
prefix: string | undefined
Expand Down
37 changes: 37 additions & 0 deletions packages/core/test/flattenAttributes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,29 @@ describe("flattenAttributes", () => {

expect(flattenAttributes(obj, "retry.byStatus")).toEqual(expected);
});

it("handles circular references correctly", () => {
const user = { name: "Alice" };
user["blogPosts"] = [{ title: "Post 1", author: user }]; // Circular reference

const result = flattenAttributes(user);
expect(result).toEqual({
"name": "Alice",
"blogPosts.[0].title": "Post 1",
"blogPosts.[0].author": "$@circular((",
});
});

it("handles nested circular references correctly", () => {
const user = { name: "Bob" };
user["friends"] = [user]; // Circular reference

const result = flattenAttributes(user);
expect(result).toEqual({
"name": "Bob",
"friends.[0]": "$@circular((",
});
});
});

describe("unflattenAttributes", () => {
Expand Down Expand Up @@ -223,4 +246,18 @@ describe("unflattenAttributes", () => {
};
expect(unflattenAttributes(flattened)).toEqual(expected);
});

it("rehydrates circular references correctly", () => {
const flattened = {
"name": "Alice",
"blogPosts.[0].title": "Post 1",
"blogPosts.[0].author": "$@circular((",
};

const result = unflattenAttributes(flattened);
expect(result).toEqual({
name: "Alice",
blogPosts: [{ title: "Post 1", author: "[Circular Reference]" }],
});
});
});