Skip to content

Fix notification config #254

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.

Sign up for GitHub

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

Draft
wants to merge 15 commits into
base: 5.0
Choose a base branch
from
Draft

Fix notification config #254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
34863fa
Fix notification_config
nsauk May 20, 2025
1ff959a
Merge branch 'refs/heads/5.0' into fix-notification-config
nsauk May 22, 2025
ee0b48d
Merge branch 'refs/heads/5.0' into fix-notification-config
nsauk May 22, 2025
c418ace
Fix specs
nsauk May 26, 2025
e497808
Merge branch '5.0' into fix-notification-config
nsauk May 26, 2025
72fd11d
Simplify check because of different codes among versions
nsauk May 26, 2025
9be65b6
Merge branch '5.0' into fix-notification-config
klobuczek May 27, 2025
16e1d45
Use public Java API
nsauk May 28, 2025
1d9575f
Disable specs for old server versions
nsauk May 28, 2025
44e7167
Add constants
nsauk May 28, 2025
bd6d4c9
Remove constants from jruby
nsauk May 28, 2025
47bcd2c
Refactor
nsauk May 28, 2025
421568d
Merge branch '5.0' into fix-notification-config
nsauk May 28, 2025
a37fcb5
Reduce changes
nsauk May 29, 2025
ca4d75b
Merge branch '5.0' into fix-notification-config
klobuczek Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jruby/neo4j/driver/ext/config_converter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def notification_config(minimum_severity: nil, disabled_categories: nil)
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) }
Copy link
Member

Choose a reason for hiding this comment

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

@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) }

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

&.map { |value| org.neo4j.driver.NotificationCategory.const_get(value.to_s.upcase) }
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Using NotificationClassification serves as well as a type of documentation.

&.then(&java.util.HashSet.method(:new)))
end

Expand Down
31 changes: 30 additions & 1 deletion spec/neo4j/driver/dev_manual_examples_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,15 @@

it { is_expected.to be true }
end

context 'Example 2.12. Notification config' do
let(:config) { { notification_config: { minimum_severity: :warning, disabled_categories: [:hint, :generic] } } }

it { is_expected.to be true }
end
end

context 'Example 2.12. Service unavailable' do
context 'Example 2.13. Service unavailable' do
def add_item(driver)
session = driver.session
session.write_transaction { |tx| tx.run('CREATE (a:Item)') }
Expand Down Expand Up @@ -293,4 +299,27 @@ def match_person_nodes(tx)
expect(add_employees('abc')).to eq(2)
end
end

context '5. Notification config' do
subject do
driver.session do |session|
result = session.run(
'MATCH p=shortestPath((:Person {name: $start})-[*]->(:Person {name: $end})) RETURN p',
start: 'Alice',
end: 'Bob'
)
result.consume.notifications.first&.code
end
end

context 'Example 5.1. Default severity and categories' do
it { is_expected.to eq('Neo.ClientNotification.Statement.UnboundedVariableLengthPattern') }
end

context 'Example 5.2. Custom severity and categories' do
let(:config) { { notification_config: { minimum_severity: :warning, disabled_categories: [:hint, :generic] } } }

it { is_expected.not_to be nil }
end
end
end
Loading