Skip to content
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

Adds support for cypher versions in semantic analysis-linting #328

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/blue-penguins-worry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@neo4j-cypher/language-support': patch
'@neo4j-cypher/schema-poller': patch
---

Updates semantic error worker to use given cypher version
1 change: 1 addition & 0 deletions packages/language-support/src/dbSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ export interface DbSchema {
propertyKeys?: string[];
procedures?: Record<string, Neo4jProcedure>;
functions?: Record<string, Neo4jFunction>;
defaultLanguage?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's best to restrict the type here to CypherVersion, defined in packages/language-support/src/types.ts

}
35 changes: 34 additions & 1 deletion packages/language-support/src/parserWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { DiagnosticSeverity, Position } from 'vscode-languageserver-types';
import { _internalFeatureFlags } from './featureFlags';
import {
ClauseContext,
CypherVersionContext,
default as CypherParser,
FunctionNameContext,
LabelNameContext,
Expand Down Expand Up @@ -45,6 +46,7 @@ export interface ParsedStatement {
collectedVariables: string[];
collectedFunctions: ParsedFunction[];
collectedProcedures: ParsedProcedure[];
cypherVersion?: string;
Copy link
Collaborator

@ncordon ncordon Jan 20, 2025

Choose a reason for hiding this comment

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

Same here, it would be best to use the type restricting what the string can contain

}

export interface ParsingResult {
Expand Down Expand Up @@ -174,8 +176,14 @@ export function createParsingResult(query: string): ParsingResult {
const labelsCollector = new LabelAndRelTypesCollector();
const variableFinder = new VariableCollector();
const methodsFinder = new MethodsCollector(tokens);
const preparserFinder = new PreparserCollector();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Every collector is in charge of extracting one type of structure here. I'd say say cypherVersionCollector here is more consistent with how we've named the rest

const errorListener = new SyntaxErrorsListener(tokens);
parser._parseListeners = [labelsCollector, variableFinder, methodsFinder];
parser._parseListeners = [
labelsCollector,
variableFinder,
methodsFinder,
preparserFinder,
];
parser.addErrorListener(errorListener);
const ctx = parser.statementsOrCommands();
// The statement is empty if we cannot find anything that is not EOF or a space
Expand Down Expand Up @@ -204,6 +212,7 @@ export function createParsingResult(query: string): ParsingResult {
collectedVariables: variableFinder.variables,
collectedFunctions: methodsFinder.functions,
collectedProcedures: methodsFinder.procedures,
cypherVersion: preparserFinder.cypherVersion,
};
});

Expand Down Expand Up @@ -403,6 +412,30 @@ class MethodsCollector extends ParseTreeListener {
}
}

class PreparserCollector extends ParseTreeListener {
public cypherVersion: string;

constructor() {
super();
}

enterEveryRule() {
/* no-op */
}
visitTerminal() {
/* no-op */
}
visitErrorNode() {
/* no-op */
}

exitEveryRule(ctx: unknown) {
if (ctx instanceof CypherVersionContext) {
this.cypherVersion = ctx.getText();
Copy link
Collaborator

@ncordon ncordon Jan 20, 2025

Choose a reason for hiding this comment

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

This would have to be CYPHER ${ctx.getText()} to accomodate for cypherVersion being a custom type

}
}
}

type CypherCmd = { type: 'cypher'; query: string };
type RuleTokens = {
start: Token;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ function copySettingSeverity(
export function wrappedSemanticAnalysis(
query: string,
dbSchema: DbSchema,
parsedVersion: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be optional: parsedVersion?: string

): SemanticAnalysisResult {
try {
if (JSON.stringify(dbSchema) !== JSON.stringify(previousSchema)) {
Expand All @@ -63,7 +64,20 @@ export function wrappedSemanticAnalysis(
});
}

const semanticErrorsResult = analyzeQuery(query, 'cypher 5');
const validCypherVersions = ['cypher 5', 'cypher 25'];
let cypherVersion = 'cypher 5';
const fullParsedVersion = 'cypher ' + parsedVersion;
const defaultVersion = dbSchema.defaultLanguage?.toLowerCase();

if (parsedVersion && validCypherVersions.includes(fullParsedVersion)) {
cypherVersion = fullParsedVersion;
} else if (
dbSchema.defaultLanguage &&
validCypherVersions.includes(defaultVersion)
) {
cypherVersion = defaultVersion;
}
const semanticErrorsResult = analyzeQuery(query, cypherVersion);
const errors: SemanticAnalysisElement[] = semanticErrorsResult.errors;
const notifications: SemanticAnalysisElement[] =
semanticErrorsResult.notifications;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ export function lintCypherQuery(
const { notifications, errors } = wrappedSemanticAnalysis(
cmd.statement,
dbSchema,
current.cypherVersion,
);

// This contains both the syntax and the semantic errors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,53 @@ import { testData } from '../testData';
import { getDiagnosticsForQuery } from './helpers';

describe('Semantic validation spec', () => {
test('Semantic analysis is dependant on cypher version', () => {
const query1 = 'CYPHER 5 MATCH (n)-[r]->(m) SET r += m';
const diagnostics1 = getDiagnosticsForQuery({ query: query1 });
const query2 = 'CYPHER 25 MATCH (n)-[r]->(m) SET r += m';
const diagnostics2 = getDiagnosticsForQuery({ query: query2 });
expect(diagnostics1[0].message).not.toEqual(diagnostics2[0].message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion these kind of checks could be flaky at some point because:

  • We are accessing a hardcoded index in the array.
  • We are not doing explicit checks, but implicit ones, it'd be best to test concrete error messages.

We have a mechanism to assert on specific things, which is using the toMatchInlineSnapshot(), so it's not big effort to have those kind of explicit tests if we can.

});

test('Semantic analysis defaults to cypher 5 when no default version is given, and no version is given in query', () => {
const query1 = 'CYPHER 5 MATCH (n)-[r]->(m) SET r += m';
const diagnostics1 = getDiagnosticsForQuery({ query: query1 });
const query2 = 'MATCH (n)-[r]->(m) SET r += m';
const diagnostics2 = getDiagnosticsForQuery({ query: query2 });
expect(diagnostics1[0].message).toEqual(diagnostics2[0].message);
});

//TODO: Maybe this should actually yield a warning
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this card https://trello.com/c/0sFWd1iZ, it's mentioned that syntax errors arent working in general now that we've switched to semantic-worker errors, so maybe we want to handle this error there too?

Should be quite simple to check and throw as is though, so maybe I should just do this one now and handle the rest later?

test('Semantic analysis defaults to cypher 5 when faulty version is given', () => {
const query1 = 'CYPHER 5 MATCH (n)-[r]->(m) SET r += m';
const diagnostics1 = getDiagnosticsForQuery({ query: query1 });
const query2 = 'CYPHER 800 MATCH (n)-[r]->(m) SET r += m';
const diagnostics2 = getDiagnosticsForQuery({ query: query2 });
expect(diagnostics1[0].message).toEqual(diagnostics2[0].message);
});

test('Semantic analysis uses default language if no language is defined in query', () => {
const query1 = 'MATCH (n)-[r]->(m) SET r += m';
const diagnostics1 = getDiagnosticsForQuery({
query: query1,
dbSchema: { defaultLanguage: 'CYPHER 25' },
});
const query2 = 'CYPHER 25 MATCH (n)-[r]->(m) SET r += m';
const diagnostics2 = getDiagnosticsForQuery({ query: query2 });
expect(diagnostics1[0].message).toEqual(diagnostics2[0].message);
});

test('In-query version takes priority for semantic analysis even if defaultLanguage is defined', () => {
const query1 = 'CYPHER 5 MATCH (n)-[r]->(m) SET r += m';
const diagnostics1 = getDiagnosticsForQuery({
query: query1,
dbSchema: { defaultLanguage: 'CYPHER 25' },
});
const query2 = 'CYPHER 25 MATCH (n)-[r]->(m) SET r += m';
const diagnostics2 = getDiagnosticsForQuery({ query: query2 });
expect(diagnostics1[0].message).not.toEqual(diagnostics2[0].message);
});

test('Does not trigger semantic errors when there are syntactic errors', () => {
const query = 'METCH (n) RETURN m';

Expand Down
7 changes: 7 additions & 0 deletions packages/schema-poller/src/metadataPoller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ export class MetadataPoller {
this.dbSchema.aliasNames = result.data.databases.flatMap(
(db) => db.aliases ?? [],
);
const dbs = result.data.databases;
const currentDb = dbs.find(
(db) => db.name === this.connection.currentDb,
);
if (currentDb) {
this.dbSchema.defaultLanguage = currentDb.defaultLanguage;
}
}
},
});
Expand Down
1 change: 1 addition & 0 deletions packages/schema-poller/src/queries/databases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export type Database = {
writer?: boolean;
access?: string;
constituents?: string[];
defaultLanguage?: string; // to be introduced
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@ncordon ncordon Jan 20, 2025

Choose a reason for hiding this comment

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

I think we can have CypherVersion as type all the way from here. So to do that you probably have to modify the result transformer:

    map(record) {
      const obj = record.toObject();
      if (obj.defaultLanguage) {
        obj.defaultLanguage = (obj.defaultLanguage as string).toLowerCase();
      }
      return obj as Database;
    },

};

/**
Expand Down
Loading