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

[master < T938] Bolt v5 support #938

Merged
merged 22 commits into from Jun 12, 2023
Merged

Conversation

andrejtonev
Copy link
Contributor

@andrejtonev andrejtonev commented May 16, 2023

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

To keep docs changelog up to date, one more thing to do:

  • Write a release note here
  • Tag someone from docs team in the comments

resolves #920
resolves #921

@andrejtonev andrejtonev changed the title Minimum working implementation of Bolt v5 [master < 921] Bolt v5 support May 16, 2023
@gitbuda gitbuda added the bolt An issues or feature related to bolt protocol. label May 16, 2023
@gitbuda gitbuda added this to the mg-v2.9.0 milestone May 16, 2023
@gitbuda gitbuda mentioned this pull request May 16, 2023
1 task
@andrejtonev andrejtonev marked this pull request as ready for review May 18, 2023 14:45
@andrejtonev
Copy link
Contributor Author

Missing notifications_minimum_severity and notifications_disabled_categories.
However, this was not added on purpose, since we are currently ignoring the extra field (as explained in the #939 issue).

Modified .clang-format to default to Google for all languages
Modified yaml check to support multiple documents
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Looks good, I left few comments. I want to test this on Monday and then that is it

src/communication/bolt/v1/session.hpp Show resolved Hide resolved
src/communication/bolt/v1/states/init.hpp Show resolved Hide resolved
src/communication/bolt/v1/states/handlers.hpp Outdated Show resolved Hide resolved
src/communication/bolt/v1/states/executing.hpp Outdated Show resolved Hide resolved
tests/drivers/node/v5_8/docs_how_to_query.js Show resolved Hide resolved
tests/drivers/node/v5_8/docs_how_to_query.js Show resolved Hide resolved
tests/drivers/node/v5_8/max_query_length.js Show resolved Hide resolved
tests/drivers/python/v5_8/max_query_length.py Show resolved Hide resolved
@andrejtonev
Copy link
Contributor Author

andrejtonev commented May 19, 2023

The current version will fail during the driver tests due to the Java version on the CI server.
Neo4j Java drivers v1 and v4 need Java 11 which is currently installed and the tests should pass.
The v5 drivers need Java 17. In addition to Java, the tests use Maven package manager (as suggested by the neo4j team) which is missing as well.

Another test that does not fail, but is installing important missing packages on the fly is the GO v5 test. It needs a newer version of golang (>1.18.0), which it downloads and deploys locally. It's not ideal, but it's working so far.

@gitbuda

@gitbuda gitbuda changed the title [master < 921] Bolt v5 support [master < T938] Bolt v5 support May 22, 2023
@gitbuda gitbuda changed the title [master < T938] Bolt v5 support [master < T] Bolt v5 support May 22, 2023
@antoniofilipovic antoniofilipovic changed the title [master < T] Bolt v5 support [master < T938] Bolt v5 support May 22, 2023
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Tested it and all works, approve from me

@gitbuda gitbuda merged commit 30ec570 into master Jun 12, 2023
6 checks passed
@gitbuda gitbuda deleted the T921-MG-Bolt-v5-server-side-support branch June 12, 2023 16:55
@andrejtonev
Copy link
Contributor Author

Changelog:

  1. Added support for Bolt v5.2
  2. Added support for element_id (string representation of ID; currently just mirrors the integer ID)
  3. Works with default authorization values (newer Neo4j drivers do not send values that are set to their default values; for example when connecting, the user can skip defining the username and password; this was causing problems with out implementation, but now we prefill the fields to their default values - empty strings in the authorization's case)

@vpavicic

@vpavicic
Copy link
Contributor

vpavicic commented Jun 26, 2023

@andrejtonev
We decided we would not get into details, just say that we support Bolt v5

But this is great info, so up until 5.2, not further?

@andrejtonev
Copy link
Contributor Author

Yes. Actually we added support specifically for 5.2. 5.0 and 5.1 are not supported, those will default back to v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bolt An issues or feature related to bolt protocol.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for Bolt v5 server-side Cannot connect to authless memgraph instance w/ neo4j python driver
5 participants