Skip to content

Commit cbc2dfe

Browse files
author
Akila Tennakoon
committed
short circuiting and more tests
1 parent 14d5c39 commit cbc2dfe

File tree

4 files changed

+269
-100
lines changed

4 files changed

+269
-100
lines changed

‎src/handlers/DocumentHandler.ts‎

Lines changed: 76 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { DidChangeTextDocumentParams } from 'vscode-languageserver';
33
import { TextDocumentChangeEvent } from 'vscode-languageserver/lib/common/textDocuments';
44
import { NotificationHandler } from 'vscode-languageserver-protocol';
55
import { TextDocument } from 'vscode-languageserver-textdocument';
6-
import { CloudFormationFileType } from '../document/Document';
6+
import { CloudFormationFileType, Document } from '../document/Document';
77
import { createEdit } from '../document/DocumentUtils';
88
import { LspDocuments } from '../protocol/LspDocuments';
99
import { ServerComponents } from '../server/ServerComponents';
@@ -34,26 +34,9 @@ export function didOpenHandler(components: ServerComponents): (event: TextDocume
3434
}
3535
}
3636

37-
components.cfnLintService.lintDelayed(content, uri, LintTrigger.OnOpen).catch((reason) => {
38-
// Handle cancellation gracefully - user might have closed/changed the document
39-
if (reason instanceof CancellationError) {
40-
// Do nothing - cancellation is expected behavior
41-
} else {
42-
log.error(reason, `Linting error for ${uri}`);
43-
}
44-
});
37+
triggerValidation(components, content, uri, LintTrigger.OnOpen, ValidationTrigger.OnOpen);
4538

46-
// Trigger Guard validation
47-
components.guardService.validateDelayed(content, uri, ValidationTrigger.OnOpen).catch((reason) => {
48-
// Handle cancellation gracefully - user might have closed/changed the document
49-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
50-
// Do nothing
51-
} else {
52-
log.error(reason, `Guard validation error for ${uri}`);
53-
}
54-
});
55-
56-
components.documentManager.sendDocumentMetadata(0);
39+
components.documentManager.sendDocumentMetadata();
5740
};
5841
}
5942

@@ -71,67 +54,63 @@ export function didChangeHandler(
7154
return;
7255
}
7356

74-
const changes = params.contentChanges;
75-
let finalContent = textDocument.getText();
57+
// This is the document AFTER changes
58+
const document = new Document(textDocument);
59+
const finalContent = document.getText();
7660

77-
try {
78-
const tree = components.syntaxTreeManager.getSyntaxTree(documentUri);
79-
let currentContent = tree ? tree.content() : '';
61+
const tree = components.syntaxTreeManager.getSyntaxTree(documentUri);
8062

81-
for (const change of changes) {
82-
if ('range' in change) {
83-
// Incremental change
84-
const start: Point = {
85-
row: change.range.start.line,
86-
column: change.range.start.character,
87-
};
88-
const end: Point = {
89-
row: change.range.end.line,
90-
column: change.range.end.character,
91-
};
92-
93-
const { edit, newContent } = createEdit(currentContent, change.text, start, end);
63+
// Short-circuit if this is not a template
64+
if (!document.isTemplate()) {
65+
if (tree) {
66+
// Clean-up if was but no longer is a template
67+
components.syntaxTreeManager.deleteSyntaxTree(documentUri);
68+
}
69+
return;
70+
}
9471

95-
if (tree) {
72+
if (tree) {
73+
// This starts as the text BEFORE changes
74+
let currentContent = tree.content();
75+
try {
76+
const changes = params.contentChanges;
77+
for (const change of changes) {
78+
if ('range' in change) {
79+
// Incremental change
80+
const start: Point = {
81+
row: change.range.start.line,
82+
column: change.range.start.character,
83+
};
84+
const end: Point = {
85+
row: change.range.end.line,
86+
column: change.range.end.character,
87+
};
88+
const { edit, newContent } = createEdit(currentContent, change.text, start, end);
9689
components.syntaxTreeManager.updateWithEdit(documentUri, newContent, edit);
90+
currentContent = newContent;
9791
} else {
98-
components.syntaxTreeManager.add(documentUri, newContent);
92+
// Full document change
93+
components.syntaxTreeManager.add(documentUri, change.text);
94+
currentContent = change.text;
9995
}
100-
101-
currentContent = newContent;
102-
} else {
103-
// Full document change
104-
components.syntaxTreeManager.add(documentUri, change.text);
105-
currentContent = change.text;
10696
}
97+
} catch (error) {
98+
log.error({ error, uri: documentUri, version }, 'Error updating tree - recreating');
99+
components.syntaxTreeManager.add(documentUri, finalContent);
107100
}
108-
finalContent = currentContent;
109-
} catch (error) {
110-
log.error({ error, uri: documentUri, version }, 'Error updating tree - recreating');
111-
components.syntaxTreeManager.add(documentUri, textDocument.getText());
101+
} else {
102+
// If we don't have a tree yet, just parse the final document
103+
components.syntaxTreeManager.add(documentUri, finalContent);
112104
}
113105

114-
// Trigger cfn-lint validation
115-
components.cfnLintService.lintDelayed(finalContent, documentUri, LintTrigger.OnChange, true).catch((reason) => {
116-
// Handle both getTextDocument and linting errors
117-
if (reason instanceof CancellationError) {
118-
// Do nothing - cancellation is expected behavior
119-
} else {
120-
log.error(reason, `Error in didChange processing for ${documentUri}`);
121-
}
122-
});
123-
124-
// Trigger Guard validation
125-
components.guardService
126-
.validateDelayed(finalContent, documentUri, ValidationTrigger.OnChange, true)
127-
.catch((reason) => {
128-
// Handle both getTextDocument and validation errors
129-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
130-
// Do nothing
131-
} else {
132-
log.error(reason, `Error in Guard didChange processing for ${documentUri}`);
133-
}
134-
});
106+
triggerValidation(
107+
components,
108+
finalContent,
109+
documentUri,
110+
LintTrigger.OnChange,
111+
ValidationTrigger.OnChange,
112+
true,
113+
);
135114

136115
// Republish validation diagnostics if available
137116
const validationDetails = components.validationManager
@@ -179,26 +158,33 @@ export function didSaveHandler(components: ServerComponents): (event: TextDocume
179158
const documentUri = event.document.uri;
180159
const documentContent = event.document.getText();
181160

182-
// Trigger cfn-lint validation
183-
components.cfnLintService.lintDelayed(documentContent, documentUri, LintTrigger.OnSave).catch((reason) => {
184-
if (reason instanceof CancellationError) {
185-
// Do nothing - cancellation is expected behavior
186-
} else {
187-
log.error(reason, `Linting error for ${documentUri}`);
188-
}
189-
});
190-
191-
// Trigger Guard validation
192-
components.guardService
193-
.validateDelayed(documentContent, documentUri, ValidationTrigger.OnSave)
194-
.catch((reason) => {
195-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
196-
// Do nothing
197-
} else {
198-
log.error(reason, `Guard validation error for ${documentUri}`);
199-
}
200-
});
161+
triggerValidation(components, documentContent, documentUri, LintTrigger.OnSave, ValidationTrigger.OnSave);
201162

202163
components.documentManager.sendDocumentMetadata(0);
203164
};
204165
}
166+
167+
function triggerValidation(
168+
components: ServerComponents,
169+
content: string,
170+
uri: string,
171+
lintTrigger: LintTrigger,
172+
validationTrigger: ValidationTrigger,
173+
debounce?: boolean,
174+
): void {
175+
components.cfnLintService.lintDelayed(content, uri, lintTrigger, debounce).catch((reason) => {
176+
if (reason instanceof CancellationError) {
177+
// Do nothing - cancellation is expected behavior
178+
} else {
179+
log.error(reason, `Linting error for ${uri}`);
180+
}
181+
});
182+
183+
components.guardService.validateDelayed(content, uri, validationTrigger, debounce).catch((reason) => {
184+
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
185+
// Do nothing
186+
} else {
187+
log.error(reason, `Guard validation error for ${uri}`);
188+
}
189+
});
190+
}

‎tst/integration/DocumentHandler.test.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,107 @@ describe('DocumentHandler', () => {
9999
expect(extension.components.documentManager.get(uri)).toBeUndefined();
100100
});
101101
});
102+
103+
it('should create syntax tree for template documents on open', async () => {
104+
const content = 'AWSTemplateFormatVersion: "2010-09-09"\nResources: {}';
105+
106+
await extension.openDocument({
107+
textDocument: {
108+
uri,
109+
languageId: 'yaml',
110+
version: 1,
111+
text: content,
112+
},
113+
});
114+
115+
await WaitFor.waitFor(() => {
116+
const tree = extension.components.syntaxTreeManager.getSyntaxTree(uri);
117+
expect(tree).toBeDefined();
118+
expect(tree?.content()).toBe(content);
119+
});
120+
});
121+
122+
it('should update syntax tree on incremental document changes', async () => {
123+
const initialContent = 'AWSTemplateFormatVersion: "2010-09-09"\nResources: {}';
124+
125+
await extension.openDocument({
126+
textDocument: {
127+
uri,
128+
languageId: 'yaml',
129+
version: 1,
130+
text: initialContent,
131+
},
132+
});
133+
134+
await WaitFor.waitFor(() => {
135+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeDefined();
136+
});
137+
138+
const editRange = { start: { line: 0, character: 35 }, end: { line: 0, character: 37 } };
139+
const editText = '00';
140+
const expectedContent = TestExtension.applyEdit(initialContent, editRange, editText);
141+
142+
await extension.changeDocument({
143+
textDocument: { uri, version: 2 },
144+
contentChanges: [
145+
{
146+
range: editRange,
147+
text: editText,
148+
},
149+
],
150+
});
151+
152+
await WaitFor.waitFor(() => {
153+
const tree = extension.components.syntaxTreeManager.getSyntaxTree(uri);
154+
expect(tree).toBeDefined();
155+
expect(tree?.content()).toBe(expectedContent);
156+
});
157+
});
158+
159+
it('should delete syntax tree when document is closed', async () => {
160+
const content = 'AWSTemplateFormatVersion: "2010-09-09"';
161+
162+
await extension.openDocument({
163+
textDocument: {
164+
uri,
165+
languageId: 'yaml',
166+
version: 1,
167+
text: content,
168+
},
169+
});
170+
171+
await WaitFor.waitFor(() => {
172+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeDefined();
173+
});
174+
175+
await extension.closeDocument({
176+
textDocument: { uri },
177+
});
178+
179+
await WaitFor.waitFor(() => {
180+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeUndefined();
181+
});
182+
});
183+
184+
it('should not create syntax tree for non-template documents', async () => {
185+
const content = 'someKey: someValue\nanotherKey: anotherValue';
186+
187+
await extension.openDocument({
188+
textDocument: {
189+
uri,
190+
languageId: 'yaml',
191+
version: 1,
192+
text: content,
193+
},
194+
});
195+
196+
await WaitFor.waitFor(() => {
197+
expect(extension.components.documentManager.get(uri)).toBeDefined();
198+
});
199+
200+
// Give it time to potentially create a tree (it shouldn't)
201+
await new Promise((resolve) => setTimeout(resolve, 100));
202+
203+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeUndefined();
204+
});
102205
});

0 commit comments

Comments
 (0)