-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix compatibility with Neo4j server and Neo4j::Driver #38
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Neo4j::Driver 1.02 no longer reblesses values received via Neo4j::Bolt. Like before, the returned values are still Neo4j::Types implementations, so their interface doesn't actually change. But the package name of such values changes from Neo4j::Driver::Type::* to Neo4j::Bolt::*, which trips up some checks in REST::Neo4p that expect to see "Driver" in the package name. Original discussion: majensen/perlbolt#37
It's necessary to call a method on the result within the try block. Any method will do; has_next() is one of the cheapest ones to call. Resolves #28.
ResultProcessor.pm doesn't actually use warnings, so `no warnings` is currently unnecessary, but I'm still adding it in this change. While doing so, I also fix some typos that had gone unnoticed because ResultProcessor.pm didn't use strict, either. Closes #34.
Neo4j 5 removed a number of deprecated procedures used by Neo4p to implement indices. As a result, indices don't work with Neo4p 0.4003 on Neo4j 5. Simply trying to replace the deprecated calls in DriverActions.pm with the new syntax seemed to work in Neo4j 4, but not Neo4 5. The issue is possibly related to the way Neo4p uses properties on the index. There might be changes that I missed in Neo4j 5 in addition to the new syntax. But since Neo4j 5 currently isn't officially supported by Neo4p, it might not matter much. This change for now simply skips the failing tests. Note that Neo4j < 5.17 doesn't allow query parameters in the new index syntax.
Back when Neo4p was updated for Neo4j 4, some tests were rewritten to use the $param syntax, which broke those tests on old Neo4j versions. This change reverts the syntax in affected tests back to {param} and adds the Neo4j::Driver option cypher_params v2 to keep things working on Neo4j 4+. The cypher_params option replaced the experimental cypher_filter option in 2021. https://metacpan.org/release/AJNN/Neo4j-Driver-0.52/view/lib/Neo4j/Driver/Deprecations.pod#Parameter-syntax-conversion-using-cypher_filter
Back when Neo4p was updated for Neo4j 4, queries that begin with MATCH were added to some tests. My understanding is that such queries are unsupported on Neo4j 1. This change simply skips the affected tests on Neo4j 1.
REST::Neo4p 0.4003 can't parse the metadata for entities returned from Neo4j < 3.5 via the transaction endpoint (unless the Neo4j::Driver agent is used). This change prevents that bug from breaking the test suite. To reproduce the bug (will trigger a "Can't call method on unblessed reference" error): REST::Neo4p->begin_work; (my $q = REST::Neo4p::Query->new('MATCH (n) RETURN n LIMIT 1'))->execute; print $q->fetch->[0]->id; Issues with the tx response format aren't new. The changelog for version 0.2220 included a fix for it, but the response format was updated in later Neo4j versions, forcing further changes. I believe the current iteration of the bug may have been introduced in 2077e4b.
This was
linked to
issues
Dec 11, 2024
This is great @johannessen - more power to you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Mark @majensen,
I’ve been working on an update for Neo4j::Driver that is just about ready for release now. (Neo4j-Driver-1.02-TRIAL) The new version removes some old cruft, cutting lines of code by over 10 %. It also improves performance. One of these improvements is that values in the query result are now passed through directly from Neo4j::Bolt to the user. That change breaks use of the Bolt protocol with REST::Neo4p. (HTTP still works fine.)
This PR fixes that issue. It also contains several other fixes, including for problems in the REST::Neo4p tests related to very old and new Neo4j versions. However, the known problem with indices in Neo4j 5 still exists. #34 (comment) This PR simply skips the associated tests on unsupported Neo4j versions.
This PR should make REST::Neo4p work reasonably well on all Neo4j versions in the range 1.9 through 5.26, and possibly beyond.
I understand you don’t plan on spending much time with REST::Neo4p going forward. But as long as it involves relatively little effort (such as merging these PRs and making a new release☺️ ), I think there is value in keeping it working.