fix(v3): handle dot keys in flattenAttributes (#1510)#3185
fix(v3): handle dot keys in flattenAttributes (#1510)#3185deepshekhardas wants to merge 2 commits intotriggerdotdev:mainfrom
Conversation
… name The CommonCommandOptions schema was evaluating the default profile name at module load time, which caused the --profile option to be ignored. Changed to use an optional string with a .transform() to lazily evaluate the default only when needed. Fixes triggerdotdev#2542
|
|
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 (3)
WalkthroughThis pull request modifies the profile option handling in the CLI to use schema-level transformation instead of command-line defaults, introduces escaping mechanisms for preserving dots within object keys during attribute flattening and unflattening operations, and adds comprehensive test coverage for the new dot-escaping functionality. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
🚩 Missing changeset for public package changes
Per CLAUDE.md and CONTRIBUTING.md, changes to packages under packages/ require a changeset (pnpm run changeset:add). This PR modifies packages/core and packages/cli-v3 (both public packages) but no .changeset/ file appears in the diff. This may be intentional if changesets are added separately, but it's worth confirming before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function escapeKey(key: string): string { | ||
| return key.replace(/\./g, "\\."); | ||
| } |
There was a problem hiding this comment.
🟡 Incomplete escape scheme: keys containing backslashes cause incorrect unflattening (regression)
The new escapeKey only escapes dots (. → \.) but does not escape existing backslash characters in keys. This means unflattenAttributes cannot distinguish between an escaped dot \. (intended as a literal dot in a key) and a literal backslash at the end of a key followed by the path separator dot. This is a regression from the old behavior.
Reproduction scenario
Consider flattenAttributes({ "a\\": { b: "value" } }) (key is literal a\):
escapeKey("a\\")→"a\\"(no dots, no change)- Nested key
bproduces flattened key"a\\.b"(chars: a, , ., b) - Old
unflattenAttributesusedsplit(".")→["a\\", "b"]→ correctly creates{ "a\\": { b: "value" } } - New
unflattenAttributesusessplit(/(?<!\\)\./)→ the dot IS preceded by\, so lookbehind prevents splitting →["a\\.b"]→unescapeKeyconverts\.to.→ incorrectly creates{ "a.b": "value" }
The fix requires also escaping backslashes in escapeKey (\ → \\) and updating unescapeKey and the split regex accordingly.
Prompt for agents
In packages/core/src/v3/utils/flattenAttributes.ts, the escapeKey/unescapeKey functions at lines 3-9 need to handle backslash escaping in addition to dot escaping to avoid ambiguity. The fix:
1. escapeKey (line 3-5): First escape backslashes, then escape dots:
function escapeKey(key: string): string {
return key.replace(/\\/g, "\\\\").replace(/\./g, "\\.");
}
2. unescapeKey (line 7-9): Reverse the escaping in opposite order:
function unescapeKey(key: string): string {
return key.replace(/\\\./g, ".").replace(/\\\\\\\\/g, "\\");
}
3. The split regex at line 293 needs to be updated to split on dots preceded by an even number of backslashes (including zero). A simple negative lookbehind (?<!\\) is insufficient. Consider using a custom split function or a regex like split(/(?<!(?:^|[^\\])(?:\\\\)*\\)\./) or implement a character-by-character split function that tracks escape state.
All three changes must be coordinated. Update the tests in packages/core/test/flattenAttributes.test.ts to cover keys containing literal backslashes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function escapeKey(key: string): string { | ||
| return key.replace(/\./g, "\\."); | ||
| } | ||
|
|
||
| function unescapeKey(key: string): string { | ||
| return key.replace(/\\\./g, "."); | ||
| } |
There was a problem hiding this comment.
🚩 Version mismatch risk: new flatten with old unflatten
If a newer SDK client uses the new flattenAttributes (which escapes dots in keys, e.g. "a\\.b"), but the webapp server is still running the old unflattenAttributes (which uses split(".")), the old code would split "a\\.b" into ["a\\", "b"] — creating a key a\ nested into b instead of the intended single key a.b. This creates a brief window during rolling deploys where data could be incorrectly unflattened. The impact depends on whether the SDK and webapp packages are always deployed in lockstep.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #1510. Escapes dots in keys during flattening and unescapes them during unflattening.