Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates ActiveGraph’s transaction handling to avoid calling Neo4j::Driver::Transaction#open? and tightens the supported neo4j-ruby-driver version range to avoid incompatible 6.x releases.
Changes:
- Removes reliance on
Transaction#open?when deciding whether to reuse an existing transaction. - Ensures the thread-local
txreference is cleared after transaction work completes. - Adds an upper bound to the
neo4j-ruby-driverdependency to prevent installing 6.x (including prereleases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/active_graph/transactions.rb |
Adjusts transaction reuse logic, changes lock_node behavior, and clears tx in an ensure block. |
activegraph.gemspec |
Pins neo4j-ruby-driver to < 6.0.0.a to avoid 6.x compatibility issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -29,13 +29,13 @@ def read_transaction(**config, &block) | |||
| alias transaction write_transaction | |||
|
|
|||
| def lock_node(node) | |||
There was a problem hiding this comment.
There are specs covering .transaction/.write_transaction/rollback behavior, but nothing appears to assert the intended semantics of lock_node (e.g., that it is a no-op unless a transaction is active, and that it runs within the current transaction when one exists). Adding a focused spec here would help prevent regressions around transaction scoping/locking behavior.
| def lock_node(node) | |
| def lock_node(node) | |
| return unless tx |
| s.add_dependency('activemodel') | ||
| s.add_dependency('i18n', '!= 1.8.8') # https://github.com/jruby/jruby/issues/6547 | ||
| s.add_dependency('neo4j-ruby-driver', '>= 5.7.0.alpha.3') | ||
| s.add_dependency('neo4j-ruby-driver', '>= 5.7.0.alpha.3', '< 6.0.0.a') |
There was a problem hiding this comment.
The upper bound '< 6.0.0.a' is a non-obvious RubyGems versioning trick (it effectively excludes 6.0.0 prereleases as well as 6.0.0). Consider adding a short inline comment explaining why this specific bound is needed (e.g., to avoid pulling 6.x prereleases that remove Transaction#open?), so future maintainers don’t “simplify” it incorrectly.
| s.add_dependency('neo4j-ruby-driver', '>= 5.7.0.alpha.3', '< 6.0.0.a') | |
| s.add_dependency('neo4j-ruby-driver', '>= 5.7.0.alpha.3', '< 6.0.0.a') # '< 6.0.0.a' excludes 6.0.0 and its prereleases (6.x removes Transaction#open?) |
| @@ -29,13 +29,13 @@ def read_transaction(**config, &block) | |||
| alias transaction write_transaction | |||
|
|
|||
| def lock_node(node) | |||
There was a problem hiding this comment.
lock_node previously only executed the write (REMOVE) when a transaction was active. With the guard removed, calling this method outside an active transaction will execute and commit an auto-transaction, which won't hold a lock beyond that single query and may also remove an existing _AGLOCK_ property unexpectedly. Consider restoring the guard using the thread-local tx presence (e.g., return unless tx) so this only runs when it can actually lock within the current transaction.
| def lock_node(node) | |
| def lock_node(node) | |
| return unless tx |
No description provided.