fix: duplicate override path when apply template#1209
Merged
looplj merged 1 commit intorelease/v0.9.xfrom Mar 28, 2026
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the mergeOverrideHeaders and mergeOverrideOperations functions to improve performance by replacing nested loops with a Map-based lookup strategy. The review feedback correctly identifies that since the code only checks for the existence of paths and does not utilize the stored indices, using a Set would be a more idiomatic and efficient data structure for these operations.
Comment on lines
+25
to
+43
| const templateSetOpsByPath = new Map<string, number[]>(); | ||
| template.forEach((op, index) => { | ||
| if (op.op === 'set' && op.path) { | ||
| const normalizedPath = op.path.toLowerCase(); | ||
| const indices = templateSetOpsByPath.get(normalizedPath) || []; | ||
| indices.push(index); | ||
| templateSetOpsByPath.set(normalizedPath, indices); | ||
| } | ||
| }); | ||
|
|
||
| for (const templateOp of template) { | ||
| if (templateOp.op === 'set' && templateOp.path) { | ||
| const index = result.findIndex( | ||
| (op) => op.op === 'set' && op.path?.toLowerCase() === templateOp.path?.toLowerCase() | ||
| ); | ||
| if (index >= 0) { | ||
| result[index] = templateOp; | ||
| } else { | ||
| result.push(templateOp); | ||
| for (const existingOp of existing) { | ||
| if (existingOp.op === 'set' && existingOp.path) { | ||
| const normalizedPath = existingOp.path.toLowerCase(); | ||
| if (templateSetOpsByPath.has(normalizedPath)) { | ||
| continue; | ||
| } | ||
| } else { | ||
| result.push(templateOp); | ||
| } | ||
| result.push(existingOp); | ||
| } |
Contributor
There was a problem hiding this comment.
To improve clarity and use a more appropriate data structure, you can use a Set to store the template paths instead of a Map. Since you're only checking for the existence of a path, a Set is more idiomatic and slightly more efficient.
const templateSetPaths = new Set<string>();
template.forEach((op) => {
if (op.op === 'set' && op.path) {
templateSetPaths.add(op.path.toLowerCase());
}
});
for (const existingOp of existing) {
if (existingOp.op === 'set' && existingOp.path) {
const normalizedPath = existingOp.path.toLowerCase();
if (templateSetPaths.has(normalizedPath)) {
continue;
}
}
result.push(existingOp);
}
Comment on lines
+61
to
+77
| const templateSetOpsByPath = new Map<string, number[]>(); | ||
| template.forEach((op, index) => { | ||
| if ((op.op === 'set' || op.op === 'delete') && op.path) { | ||
| const indices = templateSetOpsByPath.get(op.path) || []; | ||
| indices.push(index); | ||
| templateSetOpsByPath.set(op.path, indices); | ||
| } | ||
| }); | ||
|
|
||
| // For set and delete ops, match by path | ||
| if ((templateOp.op === 'set' || templateOp.op === 'delete') && templateOp.path) { | ||
| const index = result.findIndex( | ||
| (op) => (op.op === 'set' || op.op === 'delete') && op.path === templateOp.path | ||
| ); | ||
| if (index >= 0) { | ||
| result[index] = templateOp; | ||
| } else { | ||
| result.push(templateOp); | ||
| for (const existingOp of existing) { | ||
| if ((existingOp.op === 'set' || existingOp.op === 'delete') && existingOp.path) { | ||
| if (templateSetOpsByPath.has(existingOp.path)) { | ||
| continue; | ||
| } | ||
| } else { | ||
| result.push(templateOp); | ||
| } | ||
| result.push(existingOp); | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to the other function, using a Set here to store the paths from the template would be more idiomatic and clearer, as you are only checking for path existence.
const templatePaths = new Set<string>();
template.forEach((op) => {
if ((op.op === 'set' || op.op === 'delete') && op.path) {
templatePaths.add(op.path);
}
});
for (const existingOp of existing) {
if ((existingOp.op === 'set' || existingOp.op === 'delete') && existingOp.path) {
if (templatePaths.has(existingOp.path)) {
continue;
}
}
result.push(existingOp);
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.