Adds support for cypher versions in semantic analysis-linting #328
Conversation
anderson4j
added
depends on semantic analysis
🦋 Changeset detectedLatest commit: b9fc356 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -30,6 +30,7 @@ export type Database = { | |||
| writer?: boolean; | |||
| access?: string; | |||
| constituents?: string[]; | |||
| defaultLanguage?: string; // to be introduced | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
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;
},
| expect(diagnostics1[0].message).toEqual(diagnostics2[0].message); | ||
| }); | ||
|
|
||
| //TODO: Maybe this should actually yield a warning |
There was a problem hiding this comment.
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?
| @@ -11,4 +11,5 @@ export interface DbSchema { | |||
| propertyKeys?: string[]; | |||
| procedures?: Record<string, Neo4jProcedure>; | |||
| functions?: Record<string, Neo4jFunction>; | |||
| defaultLanguage?: string; | |||
There was a problem hiding this comment.
It's best to restrict the type here to CypherVersion, defined in packages/language-support/src/types.ts
| @@ -45,6 +46,7 @@ export interface ParsedStatement { | |||
| collectedVariables: string[]; | |||
| collectedFunctions: ParsedFunction[]; | |||
| collectedProcedures: ParsedProcedure[]; | |||
| cypherVersion?: string; | |||
There was a problem hiding this comment.
Same here, it would be best to use the type restricting what the string can contain
|
|
||
| exitEveryRule(ctx: unknown) { | ||
| if (ctx instanceof CypherVersionContext) { | ||
| this.cypherVersion = ctx.getText(); |
There was a problem hiding this comment.
This would have to be CYPHER ${ctx.getText()} to accomodate for cypherVersion being a custom type
| @@ -51,6 +51,7 @@ function copySettingSeverity( | |||
| export function wrappedSemanticAnalysis( | |||
| query: string, | |||
| dbSchema: DbSchema, | |||
| parsedVersion: string, | |||
There was a problem hiding this comment.
This needs to be optional: parsedVersion?: string
| 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); |
There was a problem hiding this comment.
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.
| @@ -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(); | |||
There was a problem hiding this comment.
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
Now if defaultLanguage-field is defined on a database, this language version will be used by default in the analysis, otherwise cypher 5 is used.
If the version is defined in-query, like
CYPHER 25 ...this version will overwrite the default version, if it is given in an array of supported versions we store ourselves (currently 5 and 25).