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

Adapts procedure registry to depend on the cypher version #331

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

ncordon
Copy link
Collaborator

@ncordon ncordon commented Jan 22, 2025

Builds on top of #328.

  • packages/language-support/src/tests/testData.ts just adds the new shape for the registry in the mockSchema
  • packages/language-support/src/syntaxValidation/semanticAnalysis.js brings in the new changes to be able to update either the registry for Cypher 5 or Cypher 25.

What

Polls for the procedure registry for both Cypher 5 and Cypher 25 and wires that into the code.

This means for older versions we do SHOW PROCEDURES and SHOW FUNCTIONS and store that under 'cypher 5' in the schema.

For newer versions we poll all of the following queries:

CYPHER 5  SHOW PROCEDURES
CYPHER 25 SHOW PROCEDURES

CYPHER 5  SHOW FUNCTIONS
CYPHER 25 SHOW FUNCTIONS

Why

Starting from neo4j 2025.01 we will support two concurrent cypher versions, which will mean we could have procedures that were deprecated in Cypher 5 and removed in Cypher 25, or even arguments that were deprecated in Cypher 5 and removed in Cypher 25.

Copy link

changeset-bot bot commented Jan 22, 2025

⚠️ No Changeset found

Latest commit: 3a10389

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from 57c7f5d to ff25f9e Compare January 22, 2025 17:38
Comment on lines +14 to +16
if (featureFlags.cypher25 !== undefined) {
_internalFeatureFlags.cypher25 = featureFlags.cypher25;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to pass the feature flags to the lint worker because the _internalFeatureFlags variable from the outside lives in another thread

Comment on lines +26 to +28
if (process.env.CYPHER_25 === 'true') {
_internalFeatureFlags.cypher25 = true;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so we can set the feature flag in the language server from the vscode extension

@@ -48,7 +48,7 @@
"vscode-languageserver-types": "^3.17.3"
},
"scripts": {
"gen-parser": "antlr4 -Dlanguage=TypeScript -visitor src/antlr-grammar/CypherCmdLexer.g4 src/antlr-grammar/CypherCmdParser.g4 -o src/generated-parser/ -Xexact-output-dir",
"gen-parser": "export ANTLR4_TOOLS_ANTLR_VERSION=4.13.2 && antlr4 -Dlanguage=TypeScript -visitor src/antlr-grammar/CypherCmdLexer.g4 src/antlr-grammar/CypherCmdParser.g4 -o src/generated-parser/ -Xexact-output-dir",
Copy link
Collaborator Author

@ncordon ncordon Jan 23, 2025

Choose a reason for hiding this comment

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

This is because https://github.com/antlr/antlr4-tools will resolve and download the most up to date version of antlr if this environment variable is not set. But it was failing to contact the relevant url.

This hardcodes the version we want to use. Which we should do in any case, because we wouldn't want to use a different version for generating the parser and for the antlr4 runtime.

Comment on lines 12 to 13
procedures?: Partial<Record<CypherVersion, Record<string, Neo4jProcedure>>>;
functions?: Partial<Record<CypherVersion, Record<string, Neo4jFunction>>>;
Copy link
Collaborator Author

@ncordon ncordon Jan 23, 2025

Choose a reason for hiding this comment

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

We need a different procedure registry for each version because the same function / procedure could have a different signature in different versions.

At the moment there are not many examples of methods that have different signatures for different versions(just apoc.cypher.runTimeboxed). But there will be in the future.

Comment on lines 164 to 169
const versions: (CypherVersion | undefined)[] = isNewerNeo4j
? cypherVersions
: [undefined];

versions.forEach((cypherVersion) => {
const effectiveCypherVersion: CypherVersion = cypherVersion ?? 'cypher 5';
Copy link
Collaborator Author

@ncordon ncordon Jan 23, 2025

Choose a reason for hiding this comment

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

If we are not connected to a new neo4j, just poll for cypher 5, which means we cannot preprend either CYPHER 5 nor CYPHER 25 to the queries

Comment on lines 30 to +33
debug: {
module: debugServer,
transport: TransportKind.ipc,
options: { env: { CYPHER_25: 'true' } },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This only affects the server version we use in the tests, not the production one. So we'll have CYPHER_25 support enabled there by default.

Comment on lines -36 to -39
diagnostics = vscode.languages
.getDiagnostics()
.filter(([uri]) => uri.scheme === 'untitled')
.flatMap(([, diagnostics]) => diagnostics);
Copy link
Collaborator Author

@ncordon ncordon Jan 23, 2025

Choose a reason for hiding this comment

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

This was giving problems when having two documents opened that were temporary and not saved to file, because it would collect the diagnostics for both.

@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from bd075b5 to f3b41a1 Compare January 26, 2025 17:14
@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from 5084993 to 662b643 Compare January 26, 2025 20:05
Copy link
Collaborator

@anderson4j anderson4j left a comment

Choose a reason for hiding this comment

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

Good stuff! Had some minor suggestions for improvement, and a question or two.

Also think some of the changes though good might have been a bit "off topic" (reformatting tests to use URI/better messages are what I think of at the top of my head). If im correct in thinking these could be their own PR, reviewing would be a little easier if we split these into their own thing

Also I made some comments about "cypher 5" vs "CYPHER 5" merging with my PR before you had done it - they are gone for me since you fixed it, but still in comment count, so ignore if they still pop up on corrected spots

packages/language-support/src/helpers.ts Outdated Show resolved Hide resolved
packages/language-support/src/parserWrapper.ts Outdated Show resolved Hide resolved
packages/language-support/src/signatureHelp.ts Outdated Show resolved Hide resolved
packages/language-support/src/helpers.ts Show resolved Hide resolved
packages/schema-poller/src/metadataPoller.ts Show resolved Hide resolved
packages/schema-poller/src/queries/databases.ts Outdated Show resolved Hide resolved
@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from 12d87d9 to d6d9547 Compare January 29, 2025 14:46
Copy link
Collaborator

@anderson4j anderson4j left a comment

Choose a reason for hiding this comment

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

Works now!

@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from 720750d to bdaa8f7 Compare January 30, 2025 13:46
@ncordon ncordon force-pushed the polling-versioned-procs-fns branch from c8ccefd to 3a10389 Compare January 30, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants