Skip to content

Commit bd4af4a

Browse files
atennak1atennak1
authored
fix literal detection to ignore resource types and parameters (#219)
1 parent 9af4b24 commit bd4af4a

File tree

6 files changed

+188
-54
lines changed

6 files changed

+188
-54
lines changed

‎src/services/extractToParameter/AllOccurrencesFinder.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export class AllOccurrencesFinder {
4343
const sections = syntaxTree.findTopLevelSections([TopLevelSection.Resources, TopLevelSection.Outputs]);
4444

4545
for (const sectionNode of sections.values()) {
46-
this.traverseForMatches(sectionNode, targetValue, targetType, occurrences);
46+
this.traverseForMatches(sectionNode, targetValue, targetType, occurrences, documentUri);
4747
}
4848

4949
return occurrences;
@@ -54,16 +54,27 @@ export class AllOccurrencesFinder {
5454
targetValue: string | number | boolean | unknown[],
5555
targetType: LiteralValueType,
5656
occurrences: Range[],
57+
documentUri: string,
5758
): void {
59+
// First check without propertyPath for performance
5860
const literalInfo = this.literalDetector.detectLiteralValue(node);
5961

60-
if (literalInfo && this.isMatchingLiteral(literalInfo, targetValue, targetType) && !literalInfo.isReference) {
61-
occurrences.push(literalInfo.range);
62+
if (literalInfo && this.isMatchingLiteral(literalInfo, targetValue, targetType)) {
63+
// Only compute propertyPath for matching literals to check if extractable
64+
const syntaxTree = this.syntaxTreeManager.getSyntaxTree(documentUri);
65+
const propertyPath = syntaxTree?.getPathAndEntityInfo(node)?.propertyPath;
66+
67+
// Re-check with propertyPath to exclude non-extractable paths
68+
const literalInfoWithPath = this.literalDetector.detectLiteralValue(node, propertyPath);
69+
70+
if (literalInfoWithPath) {
71+
occurrences.push(literalInfoWithPath.range);
72+
}
6273
return; // Don't traverse children to avoid duplicates
6374
}
6475

6576
for (const child of node.children) {
66-
this.traverseForMatches(child, targetValue, targetType, occurrences);
77+
this.traverseForMatches(child, targetValue, targetType, occurrences, documentUri);
6778
}
6879
}
6980

‎src/services/extractToParameter/ExtractToParameterProvider.ts‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,16 +55,12 @@ export class ExtractToParameterProvider implements IExtractToParameterProvider {
5555
return false;
5656
}
5757

58-
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode);
58+
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode, context.propertyPath);
5959

6060
if (!literalInfo) {
6161
return false;
6262
}
6363

64-
if (literalInfo.isReference) {
65-
return false;
66-
}
67-
6864
return true;
6965
}
7066

@@ -77,9 +73,9 @@ export class ExtractToParameterProvider implements IExtractToParameterProvider {
7773
return false;
7874
}
7975

80-
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode);
76+
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode, context.propertyPath);
8177

82-
if (!literalInfo || literalInfo.isReference) {
78+
if (!literalInfo) {
8379
return false;
8480
}
8581

@@ -178,8 +174,8 @@ export class ExtractToParameterProvider implements IExtractToParameterProvider {
178174
return undefined;
179175
}
180176

181-
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode);
182-
if (!literalInfo || literalInfo.isReference) {
177+
const literalInfo = this.literalDetector.detectLiteralValue(context.syntaxNode, context.propertyPath);
178+
if (!literalInfo) {
183179
return undefined;
184180
}
185181

‎src/services/extractToParameter/ExtractToParameterTypes.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,6 @@ export type LiteralValueInfo = {
124124
value: string | number | boolean | unknown[];
125125
type: LiteralValueType;
126126
range: Range;
127-
isReference: boolean;
128127
};
129128

130129
/**

‎src/services/extractToParameter/LiteralValueDetector.ts‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { SyntaxNode } from 'tree-sitter';
22
import { Range } from 'vscode-languageserver';
3-
import { IntrinsicFunction } from '../../context/ContextType';
3+
import { IntrinsicFunction, TopLevelSection } from '../../context/ContextType';
4+
import { PropertyPath } from '../../context/syntaxtree/SyntaxTree';
45
import { LiteralValueInfo, LiteralValueType } from './ExtractToParameterTypes';
56

67
/**
@@ -9,17 +10,26 @@ import { LiteralValueInfo, LiteralValueType } from './ExtractToParameterTypes';
910
* actual literals and existing references/intrinsic functions.
1011
*/
1112
export class LiteralValueDetector {
12-
public detectLiteralValue(node: SyntaxNode): LiteralValueInfo | undefined {
13+
public detectLiteralValue(node: SyntaxNode, propertyPath?: PropertyPath): LiteralValueInfo | undefined {
1314
if (!node || node.type === 'ERROR') {
1415
return undefined;
1516
}
1617

18+
// Exclude non-extractable paths (Parameters, Resource Type)
19+
if (this.isNonExtractablePath(propertyPath)) {
20+
return undefined;
21+
}
22+
1723
let nodeForRange = node;
1824
if (node.type === 'string_content' && node.parent?.type === 'string') {
1925
nodeForRange = node.parent;
2026
}
2127

22-
const isReference = this.isIntrinsicFunctionOrReference(nodeForRange);
28+
// Exclude intrinsic functions and references from extraction
29+
if (this.isIntrinsicFunctionOrReference(nodeForRange)) {
30+
return undefined;
31+
}
32+
2333
const literalInfo = this.extractLiteralInfo(node);
2434

2535
if (literalInfo?.value === undefined) {
@@ -30,10 +40,27 @@ export class LiteralValueDetector {
3040
value: literalInfo.value,
3141
type: literalInfo.type,
3242
range: this.nodeToRange(nodeForRange),
33-
isReference,
3443
};
3544
}
3645

46+
private isNonExtractablePath(propertyPath?: PropertyPath): boolean {
47+
if (!propertyPath || propertyPath.length === 0) {
48+
return false;
49+
}
50+
51+
// Exclude all values in Parameters section
52+
if (propertyPath[0] === TopLevelSection.Parameters) {
53+
return true;
54+
}
55+
56+
// Exclude Resources > LogicalId > Type
57+
if (propertyPath.length === 3 && propertyPath[0] === TopLevelSection.Resources && propertyPath[2] === 'Type') {
58+
return true;
59+
}
60+
61+
return false;
62+
}
63+
3764
private isIntrinsicFunctionOrReference(node: SyntaxNode): boolean {
3865
if (!node) {
3966
return false;

‎tst/unit/services/extractToParameter/AllOccurrencesFinder.test.ts‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,5 +115,82 @@ describe('AllOccurrencesFinder', () => {
115115

116116
expect(occurrences).toHaveLength(2);
117117
});
118+
119+
it('should exclude Resource Type values from occurrences', () => {
120+
const mockResourcesSection = {
121+
type: 'object',
122+
children: [
123+
{
124+
type: 'string',
125+
text: '"AWS::S3::Bucket"',
126+
startPosition: { row: 2, column: 0 },
127+
endPosition: { row: 2, column: 17 },
128+
children: [],
129+
},
130+
{
131+
type: 'string',
132+
text: '"AWS::S3::Bucket"',
133+
startPosition: { row: 5, column: 0 },
134+
endPosition: { row: 5, column: 17 },
135+
children: [],
136+
},
137+
],
138+
};
139+
140+
const sectionsMap = new Map();
141+
sectionsMap.set(TopLevelSection.Resources, mockResourcesSection as any);
142+
143+
mockSyntaxTree.findTopLevelSections.returns(sectionsMap);
144+
mockSyntaxTreeManager.getSyntaxTree.returns(mockSyntaxTree);
145+
146+
// Mock getPathAndEntityInfo to return Resource Type path
147+
mockSyntaxTree.getPathAndEntityInfo.returns({
148+
path: [],
149+
propertyPath: ['Resources', 'MyBucket', 'Type'],
150+
entityRootNode: undefined,
151+
});
152+
153+
const occurrences = finder.findAllOccurrences(
154+
'file:///test.json',
155+
'AWS::S3::Bucket',
156+
LiteralValueType.STRING,
157+
);
158+
159+
// Should find 0 occurrences because Resource Type values are excluded
160+
expect(occurrences).toHaveLength(0);
161+
});
162+
163+
it('should exclude Parameters section values from occurrences', () => {
164+
const mockResourcesSection = {
165+
type: 'object',
166+
children: [
167+
{
168+
type: 'string',
169+
text: '"String"',
170+
startPosition: { row: 2, column: 0 },
171+
endPosition: { row: 2, column: 8 },
172+
children: [],
173+
},
174+
],
175+
};
176+
177+
const sectionsMap = new Map();
178+
sectionsMap.set(TopLevelSection.Resources, mockResourcesSection as any);
179+
180+
mockSyntaxTree.findTopLevelSections.returns(sectionsMap);
181+
mockSyntaxTreeManager.getSyntaxTree.returns(mockSyntaxTree);
182+
183+
// Mock getPathAndEntityInfo to return Parameters path
184+
mockSyntaxTree.getPathAndEntityInfo.returns({
185+
path: [],
186+
propertyPath: ['Parameters', 'MyParam', 'Type'],
187+
entityRootNode: undefined,
188+
});
189+
190+
const occurrences = finder.findAllOccurrences('file:///test.json', 'String', LiteralValueType.STRING);
191+
192+
// Should find 0 occurrences because Parameters values are excluded
193+
expect(occurrences).toHaveLength(0);
194+
});
118195
});
119196
});

0 commit comments

Comments
 (0)