Skip to content

triggering a param add pop-up if running a cypher with missing params #452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 10, 2025
28 changes: 14 additions & 14 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion packages/language-support/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ export { _internalFeatureFlags } from './featureFlags';
export { formatQuery } from './formatting/formatting';
export { antlrUtils } from './helpers';
export { CypherTokenType, lexerSymbols } from './lexerSymbols';
export { parse, parserWrapper, parseStatementsStrs } from './parserWrapper';
export {
parse,
parseParameters,
parserWrapper,
parseStatementsStrs,
} from './parserWrapper';
export { signatureHelp, toSignatureInformation } from './signatureHelp';
export {
applySyntaxColouring,
Expand Down
18 changes: 18 additions & 0 deletions packages/language-support/src/parserWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,24 @@ export function createParsingResult(
return parsingResult;
}

const getClearParamName = (name: string): string => {
if (name.startsWith('`') && name.endsWith('`')) {
return name.slice(1, -1);
}
return name;
};

export function parseParameters(
query: string,
consoleCommandsEnabled: boolean,
): string[] {
const parsingResult = createParsingResult(query, consoleCommandsEnabled);
const parameters = parsingResult.statementsParsing.flatMap((statement) =>
statement.collectedParameters.map((param) => getClearParamName(param.name)),
);
return [...new Set(parameters)];
}

// This listener collects all labels and relationship types
class LabelAndRelTypesCollector extends ParseTreeListener {
labelOrRelTypes: LabelOrRelType[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,7 @@ function errorOnUndeclaredParameters(
if (parameterName.startsWith('`') && parameterName.endsWith('`')) {
parameterName = parameterName.substring(1, parameterName.length - 1);
}
const paramExists = !!dbSchema.parameters?.[parameterName];
if (!paramExists) {
if (dbSchema.parameters?.[parameterName] === undefined) {
errors.push(
generateSyntaxDiagnostic(
parameter.rawText,
Expand Down
26 changes: 15 additions & 11 deletions packages/vscode-extension/src/commandHandlers/params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function validateParamInput(
return undefined;
}

export async function addParameter(): Promise<void> {
export async function addParameter(defaultParamName?: string): Promise<void> {
const connected = await isConnected();

if (!connected) {
Expand All @@ -54,12 +54,14 @@ export async function addParameter(): Promise<void> {
return;
}

const paramName = await window.showInputBox({
prompt: 'Parameter name',
placeHolder:
'The name you want to store your parameter with, for example: param, p, `my parameter`',
ignoreFocusOut: true,
});
const paramName =
defaultParamName ??
(await window.showInputBox({
prompt: 'Parameter name',
placeHolder:
'The name you want to store your parameter with, for example: param, p, `my parameter`',
ignoreFocusOut: true,
}));
if (!paramName) {
await window.showErrorMessage('Parameter name cannot be empty.');
return;
Expand All @@ -68,7 +70,9 @@ export async function addParameter(): Promise<void> {
const dbSchema = schemaPoller.metadata.dbSchema;
let timeout: NodeJS.Timeout;
const paramValue = await window.showInputBox({
prompt: 'Parameter value',
prompt: defaultParamName
? `Parameter value for the parameter ${defaultParamName}`
: 'Parameter value',
placeHolder:
'The value for your parameter (anything you could evaluate in a RETURN), for example: 1234, "some string", datetime(), {a: 1, b: "some string"}',
ignoreFocusOut: true,
Expand All @@ -84,7 +88,7 @@ export async function addParameter(): Promise<void> {
});

if (!paramValue) {
await window.showErrorMessage('Parameter value cannot be empty.');
void window.showErrorMessage('Parameter value cannot be empty.');
return;
}

Expand Down Expand Up @@ -150,7 +154,7 @@ export async function evaluateParam(
const db = getCurrentDatabase();

if (db.type === 'system') {
await window.showErrorMessage(
void window.showErrorMessage(
'Parameters cannot be evaluated against a system database. Please connect to a user database.',
);
return;
Expand Down Expand Up @@ -186,7 +190,7 @@ export async function evaluateParam(
if (e instanceof Neo4jError) {
//If we can get past linting-check with invalid query but still have failing query
//when executing, we catch here as a backup
await window.showErrorMessage(
void window.showErrorMessage(
'Failed to evaluate parameter: ' + e.message,
);
} else {
Expand Down
15 changes: 14 additions & 1 deletion packages/vscode-extension/src/cypherRunner.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { parseStatementsStrs } from '@neo4j-cypher/language-support';
import {
parseParameters,
parseStatementsStrs,
} from '@neo4j-cypher/language-support';
import { Uri } from 'vscode';
import { addParameter } from './commandHandlers/params';
import { Connection } from './connectionService';
import { getDeserializedParams } from './parameterService';
import ResultWindow from './webviews/resultWindow';

export default class CypherRunner {
Expand All @@ -10,7 +15,15 @@ export default class CypherRunner {

async run(connection: Connection, uri: Uri, input: string) {
const statements = parseStatementsStrs(input);
const statementParams = parseParameters(input, false);
const filePath = uri.toString();
const parameters = getDeserializedParams();

for (const param of statementParams) {
if (parameters[param] === undefined) {
await addParameter(param);
}
}

if (this.results.has(filePath)) {
const resultWindow = this.results.get(filePath);
Expand Down
2 changes: 1 addition & 1 deletion packages/vscode-extension/tests/fixtures/params.cypher
Original file line number Diff line number Diff line change
@@ -1 +1 @@
RETURN $a, $b, $`some param`, $`some-param`
RETURN $a, $b, $`some param`, $`some-param`, $a + $b;
51 changes: 48 additions & 3 deletions packages/vscode-extension/tests/specs/webviews/params.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { browser } from '@wdio/globals';
import { before } from 'mocha';
import { Workbench } from 'wdio-vscode-service';
import { Key } from 'webdriverio';
import {
checkResultsContent,
executeFile,
Expand All @@ -14,6 +15,14 @@ suite('Params panel testing', () => {
workbench = await browser.getWorkbench();
});

async function escapeModal(count: number) {
for (let i = 0; i < count; i++) {
await browser.pause(500);
await browser.keys([Key.Escape]);
await waitUntilNotification(browser, 'Parameter value cannot be empty.');
}
}
Comment on lines +18 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this flaky when you just did waitUntilNotification without browser.pause, or why was this added? In general it feels a little sketchy to use browser.pause() but in some cases it seems hard to avoid. (For example with waiting for the inputBox to open to type things into it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, actually, it was flaky. So, I'd like to keep these just in case, so that the browser wouldn't click on the Escape button before the parameter add pop-up opens :/


async function addParamWithInputBox() {
await browser.executeWorkbench(async (vscode) => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
Expand Down Expand Up @@ -100,10 +109,13 @@ suite('Params panel testing', () => {
await clearParams();

await executeFile(workbench, 'params.cypher');

await escapeModal(4);

await checkResultsContent(workbench, async () => {
const text = await (await $('#query-error')).getText();
await expect(text).toContain(
'Error executing query RETURN $a, $b, $`some param`, $`some-param`:\nExpected parameter(s): a, b, some param, some-param',
'Error executing query RETURN $a, $b, $`some param`, $`some-param`, $a + $b;:\nExpected parameter(s): a, b, some param, some-param',
);
});
});
Expand Down Expand Up @@ -155,10 +167,13 @@ suite('Params panel testing', () => {
await forceDeleteParam('b');

await executeFile(workbench, 'params.cypher');

await escapeModal(2);

await checkResultsContent(workbench, async () => {
const text = await (await $('#query-error')).getText();
await expect(text).toContain(
'Error executing query RETURN $a, $b, $`some param`, $`some-param`:\nExpected parameter(s): a, b',
'Error executing query RETURN $a, $b, $`some param`, $`some-param`, $a + $b;:\nExpected parameter(s): a, b',
);
});
});
Expand Down Expand Up @@ -187,11 +202,41 @@ suite('Params panel testing', () => {
// to execute the file we need to be on the user database
await forceSwitchDatabase('neo4j');
await executeFile(workbench, 'params.cypher');

await escapeModal(4);

await checkResultsContent(workbench, async () => {
const text = await (await $('#query-error')).getText();
await expect(text).toContain(
'Error executing query RETURN $a, $b, $`some param`, $`some-param`:\nExpected parameter(s): a, b, some param, some-param',
'Error executing query RETURN $a, $b, $`some param`, $`some-param`, $a + $b;:\nExpected parameter(s): a, b',
);
});
});

test('Should trigger parameter add pop-up when running a query with an unknown parameter', async () => {
await clearParams();
await forceAddParam('a', '1998');
await executeFile(workbench, 'params.cypher');

// initial pop-up for param b
await browser.pause(1000);
await browser.keys(['1', '2', Key.Enter]);

// initial pop-up for param `some param`
await browser.pause(1000);
await browser.keys(['f', 'a', 'l', 's', 'e', Key.Enter]);

// initial pop-up for param `some-param`
await browser.pause(1000);
await browser.keys(['5', Key.Enter]);

await checkResultsContent(workbench, async () => {
const queryResult = await (await $('#query-result')).getText();
await expect(queryResult).toContain('1998');
await expect(queryResult).toContain('12');
await expect(queryResult).toContain('false');
await expect(queryResult).toContain('5');
await expect(queryResult).toContain('2010');
});
});
});
Loading