Fix notification config #254
Conversation
nsauk
force-pushed
the
fix-notification-config
branch
2 times, most recently
from
a7fe304 to
050b547
Compare
nsauk
force-pushed
the
fix-notification-config
branch
from
050b547 to
34863fa
Compare
| value_of(org.neo4j.driver.internal.InternalNotificationSeverity, minimum_severity), | ||
| disabled_categories | ||
| &.map { |value| value_of(org.neo4j.driver.internal.InternalNotificationCategory, value) } | ||
| &.map { |value| org.neo4j.driver.NotificationCategory.const_get(value.to_s.upcase) } |
There was a problem hiding this comment.
We have been dealing with this issue the following way: https://github.com/neo4jrb/neo4j-ruby-driver/blob/5.0/ruby/neo4j/driver/access_mode.rb
to have some level of stronger typing compatible with java.
I could be swayed over but could end up with complex converting code when the configuration is deeply nested resulting in runtime errors as users would be using nested symbol hashes instead of higher level types.
There was a problem hiding this comment.
Using NotificationClassification serves as well as a type of documentation.
|
Converted to draft because the spec is incorrect |
| org.neo4j.driver.internal.InternalNotificationConfig.new( | ||
| value_of(org.neo4j.driver.internal.InternalNotificationSeverity, minimum_severity), | ||
| disabled_categories | ||
| &.map { |value| value_of(org.neo4j.driver.internal.InternalNotificationCategory, value) } |
There was a problem hiding this comment.
@nsauk could you check. It appears to me that the only change needed was this line:
&.map { |value| value_of(org.neo4j.driver.NotificationClassification, value) }
There was a problem hiding this comment.
Maybe, additionally move the .or_else(nil) from value_of method to the line 71 as the value_of of java enum, which NotificationClassification now is, does not return Optional.
There was a problem hiding this comment.
I rewrote the rest of this method using the Java driver's public API so we rely less on internals.
InternalNotificationSeverity works with value_of, but NotificationSeverity fails because it's an interface. As far as I can understand, we can't use the same approach for both (unless it's const_get).
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the notification configuration implementation by correcting the Java class references and handling of optional values. The changes ensure proper integration with Neo4j's notification filtering system for controlling warning/hint messages.
- Updates the
value_ofmethod to return Java Optional objects instead of immediately unwrapping them - Corrects the Java class reference from
InternalNotificationCategorytoNotificationClassification - Adds comprehensive test coverage for notification configuration scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| jruby/neo4j/driver/ext/config_converter.rb | Fixes notification config implementation by correcting Java class references and Optional handling |
| spec/neo4j/driver/dev_manual_examples_spec.rb | Adds test cases for default and custom notification configurations |
| .github/workflows/specs.yml | Updates Ruby and Neo4j versions in CI matrix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| end | ||
|
|
||
| context 'Example 2.12. Service unavailable' do | ||
| context 'Example 2.13. Service unavailable' do |
There was a problem hiding this comment.
The context numbering has been incremented to 2.13, but verify that the documentation or examples being referenced align with this new numbering scheme to maintain consistency.
Copilot uses AI. Check for mistakes.
2 participants
No description provided.