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.

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

Jump to bottom

Updates for JRuby 9.4 #125

Merged
merged 11 commits into from
Jun 15, 2023
Merged

Updates for JRuby 9.4 #125

merged 11 commits into from
Jun 15, 2023

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Apr 18, 2023 •
edited
Loading

Release notes

Fix: adaptations for JRuby 9.4

What does this PR do?

This PR laid down adaptations to run this plugin under JRuby 9.4 (Ruby 3.1)

List of changes:

  • in a method with var args the hash map has to be defined when passed as argument, d19fc58 Ruby 3.0
  • Ruby 3.1 comes with Psych 4 which switched the alias of load from unsafe_load to safe_load reference. Created a wrapper method to behave consistently across different versions of JRuby.
  • fixed a stubbing error in tests: 72a2af5

Why is it important/What is the impact to the user?

Let the user to use this plugin also on future Logstash versions which ship JRuby >= 9.4

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • run tests locally with JRuby 9.3 against a Logstash with JRuby 9.3
  • run tests locally with JRuby 9.4 against a Logstash with JRuby 9.4
  • run the plugin on a Logstash that ships JRuby 9.4

How to test this PR locally

Related issues

@andsel andsel self-assigned this Apr 18, 2023
@andsel andsel marked this pull request as ready for review April 20, 2023 15:13
@andsel andsel mentioned this pull request May 2, 2023
Closed
andsel added 8 commits June 14, 2023 10:08
@andsel
Fix hash creation error from Ruby 3.0
ea119c8 
Check https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
@andsel
With Ruby 3.1 YAML.load uses a whitelist of classes that can be deser…
c3f2783 
…ialized. With this commit defines the list of permitted_classes to be used by the deserializer.
@andsel
Defined better the test stub behaviour, without this, it generated an…
72590bc 
… error like:

```
expected: ("HOME")
got: ("GEM_REQUIREMENT_LOGSTASH-CODEC-PLAIN")
Please stub a default value first if message might be received with other args as well
```
@andsel
Move YAML.load to YAML.unsafe_load
8dac74a 
With Ruby 3.1 and new version of Psych 4 (YAML parser) the `load` method
which was an alias for `unsafe_load` has been moved to be an alias for
`safe_load`.
This commit default the calls to old aliased method: `unsafe_load`.

Ref: https://www.ctrl.blog/entry/ruby-psych4.html
@andsel
Added dependency on Psych 4 to be able to run also on Logstash 7.17
2c93f94
@andsel
[skip ci] added comment to motivate the dependency on Psych 4
ecffaac
@andsel
Avoid to depend explicitly on Psych 4 and use the one bundled with th…
65a9ba3 
…e JRuby
@andsel
Bumped version 5.4.2
a2c56df
@andsel andsel requested a review from robbavey June 14, 2023 08:14
robbavey
robbavey requested changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, but can we please add unit tests for the value tracker?

@andsel
Copy link
Contributor Author

andsel commented Jun 15, 2023

@robbavey thank't for reviewing, integrated your suggestion with commit cf81c3e

@andsel andsel requested a review from robbavey June 15, 2023 09:03
robbavey
robbavey requested changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Thank you for adding a test! Can we add a couple more to make sure we cover read/write across all scenarios?

spec/plugin_mixins/jdbc/value_tracking_spec.rb Outdated

let(:yaml_date_source) { "--- !ruby/object:DateTime '2023-06-15 09:59:30.558000000 +02:00'\n" }

context "#load_yaml" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have tests that verify a few different scenarios - Time, DateTime, BigDecimal? I also wonder if we should have a test that uses the build_last_value_tracker(plugin) to do read and write these values too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, however I think that testing build_last_value_tracker is a little bit out of context of this PR. We can avoid to push 2 PRs, one for JRuby 9.4 updates and another to cover with test existing code, and commit everything to this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done:

  • with commit 743f919 #load_yaml has been covered for Time and BigDecimal cases
  • while with 9a640a7 the method #build_last_value_tracker has been exercised

@andsel andsel requested a review from robbavey June 15, 2023 14:48
robbavey
robbavey reviewed Jun 15, 2023
View reviewed changes
spec/plugin_mixins/jdbc/value_tracking_spec.rb
tracker.write

temp_file.rewind
v = ValueTracking.load_yaml(::File.read(temp_file.path))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use tracker.read here? (Or create a separate reader tracker, and use reader_tracker.read?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tracker doesn't have readmethod just write.
The read methods are present only in FileHandler, which is not exposed externally of the tracker. That's why I've used such code to read back the value.

@andsel andsel requested a review from robbavey June 15, 2023 15:17
robbavey
robbavey approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@andsel andsel merged commit c06e319 into logstash-plugins:main Jun 15, 2023
@lucabelluccini lucabelluccini mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbavey robbavey robbavey approved these changes

Assignees

@andsel andsel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the plugin compatible with JRuby 9.4 (Ruby 3.1)
3 participants
@andsel @robbavey @roaksoax