fix: escape periods in attribute keys during flatten/unflatten#3800
fix: escape periods in attribute keys during flatten/unflatten#3800deepshekhardas wants to merge 1 commit into
Conversation
Object keys containing periods (e.g. 'Key 0.002mm') were incorrectly split on the '.' delimiter during flatten/unflatten, causing data corruption in the dashboard logs view. Escape dots with \$@dot sentinel during flattening and unescape during unflattening. Fixes triggerdotdev#1510
|
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR modifies the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Map keys with dots are not escaped (pre-existing inconsistency)
At line 121, Map string keys are interpolated directly into the prefix without dot escaping: this.#processValue(value, \${prefix || "map"}.${keyStr}`, depth). This is inconsistent with the new escaping for regular object keys at line 204 (key.replace(/./g, DOT_ESCAPE)). If a Map has a key like "a.b"`, the dot won't be escaped, leading to incorrect unflattening. However, this is a pre-existing issue — Map key handling was already inconsistent before this PR, and Maps are a relatively niche case for OTEL attributes. Not filing as a bug since it's pre-existing, but worth noting for completeness.
(Refers to lines 120-121)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const escapedKey = Array.isArray(obj) ? `[${key}]` : key.replace(/\./g, DOT_ESCAPE); | ||
| const newPrefix = `${prefix ? `${prefix}.` : ""}${escapedKey}`; |
There was a problem hiding this comment.
🟡 Round-trip data corruption for keys naturally containing the $@dot escape sequence
The escape sequence $@dot is never itself escaped during flattening, so keys that naturally contain the literal string $@dot are silently corrupted during round-trip. During flattenAttributes, key.replace(/\./g, DOT_ESCAPE) won't modify a key like "a$@dotb" (no dots to replace), so it stays as "a$@dotb". But during unflattenAttributes at flattenAttributes.ts:285, part.split(DOT_ESCAPE).join(".") transforms "a$@dotb" into "a.b", corrupting the key. For example: {"a$@dotb": 1} → flatten → {"a$@dotb": 1} → unflatten → {"a.b": 1}. This is a fundamental flaw in the escape mechanism — proper escaping requires that the escape sequence itself be escaped (e.g., $@dot → $@dot$@dot or a different sentinel).
Prompt for agents
The escape mechanism is not reversible because the escape sequence itself is not escaped. When flattenAttributes encounters a key like "a$@dotb" (no dots), it passes it through unchanged. But unflattenAttributes unconditionally replaces all occurrences of $@dot with dots, corrupting the key to "a.b".
To fix this properly, you need a two-phase escape:
1. First escape any existing occurrences of the escape marker (e.g., replace "$@dot" with "$@dot$@dot" or use a dedicated escape-of-escape like "$@esc"), THEN replace dots with the escape marker.
2. In unflattenAttributes, reverse the process: first unescape the dot marker, then unescape the escape-of-escape marker.
Alternatively, choose an escape sequence that is guaranteed not to collide (e.g., a longer/more unique sentinel), though this only reduces probability rather than eliminating the issue.
Files to update:
- packages/core/src/v3/utils/flattenAttributes.ts: both the escaping in doFlatten (line 204) and unescaping in unflattenAttributes (line 285)
- packages/core/test/flattenAttributes.test.ts: add a test for keys containing the escape sequence
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| export const NULL_SENTINEL = "$@null(("; | ||
| export const CIRCULAR_REFERENCE_SENTINEL = "$@circular(("; | ||
| const DOT_ESCAPE = "$@dot"; |
There was a problem hiding this comment.
🟡 Escape sequence $@dot starts with $, causing keys beginning with . to be silently dropped by removePrivateProperties
The $@dot escape sequence starts with $, which conflicts with the removePrivateProperties filter at apps/webapp/app/v3/eventRepository/common.server.ts:155 that drops all keys starting with $. When a user has an object key starting with . (e.g., {".foo": "bar"}), flattenAttributes escapes it to "$@dotfoo". This key is stored in the database. When later read back, removePrivateProperties sees the key starts with $ and filters it out before unflattenAttributes ever runs. The property is silently lost. Before this PR, a key like ".foo" would be stored as ".foo" and would not be filtered (it doesn't start with $). While keys starting with . are uncommon, this is a silent data loss regression.
Affected data flow
- User logs
{".hidden": "value"}from a task - SDK flattens to
{"$@dothidden": "value"} - Server stores
$@dothiddenas attribute key - On read,
removePrivatePropertiesfilters it out (starts with$) - Property is silently lost from span display
Prompt for agents
The escape sequence $@dot starts with $, which collides with the convention in removePrivateProperties (apps/webapp/app/v3/eventRepository/common.server.ts:155) that filters out all attribute keys starting with $. This means any user object key starting with a literal dot (.) will be escaped to a key starting with $@dot, which then gets silently dropped.
The simplest fix is to choose an escape sequence that does NOT start with $ or ctx. — the two prefixes filtered by removePrivateProperties. For example, using __dot__ or ~dot~ or any sequence that doesn't start with a character used for internal attribute prefixes.
Files to update:
- packages/core/src/v3/utils/flattenAttributes.ts: change the DOT_ESCAPE constant (line 5)
- packages/core/test/flattenAttributes.test.ts: update test expectations to match new escape sequence
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #1510
Object keys containing periods (e.g.
{ "Key 0.002mm": 31.4 }) were incorrectly split on the.delimiter during flattening, causing dashboard display corruption.Fix: Escape
.in object keys with$@dotsentinel during flatten and unescape during unflatten. This is consistent with the existing$@null((and$@circular((sentinel pattern.Changes:
flattenAttributes.ts: escape dots in non-array keys during flattenunflattenAttributes(): unescape sentinel back to.after splitting