Reimplement LogStash::String setting in Java by andsel · Pull Request #16576 · elastic/logstash

andsel · 2024-10-17T09:30:57Z

Release notes

[rn:skip]

What does this PR do?

Reimplements LogStash::Setting::String Ruby setting class into the org.logstash.settings.StringSetting and exposes it through java_import as LogStash::Setting::StringSetting.
Updates the rspec tests in two ways:

logging mock is now converted to real Log4J appender that spy log line that are later verified
verifies java.lang.IllegalArgumentException instead of ArgumentError is thrown because the kind of exception thrown by Java code, during verification.

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

As a developer I want use Java classes to shape the configuration settings.

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

Implements the other constructors, with 2 and 3 params and spread the use
- Execute SPEC_OPTS="-fd -P logstash-core/spec/logstash/runner_spec.rb" ./gradlew --rerun-tasks :logstash-core:rubyTests there is a problem in reading the deprecation logger messages in
  
  logstash/logstash-core/spec/logstash/runner_spec.rb
  
  Line 269 in 467ab3f
  
  expect(deprecation_logger_stub).to receive(:deprecated).with(a_string_including "`http.host` is a deprecated alias for `api.http.host`")
  
  . Suggestion mock the Appender and reconfigure logs?
Check the code for other instantiations of LogStash::String.new
Translate RSpec tests to JUnit if there are or cover with some testing
~~[ ] Rename StringSetting to String and use explicit java.lang.String around~~ converting means to make explicit reference to java.lang.String in all places inside the package org.logstash.settings.* .

How to test this PR locally

run of CI is one good test.
run Logstash instance, it uses plenty of string settings.
switch some string setting to an invalid value and verify it reports the error. For example tinker with config.field_reference.escape_style in config/logstash.yml

Related issues

andsel · 2024-11-12T15:52:50Z

logstash-core/spec/logstash/settings/nullable_spec.rb

      let(:candidate_value) { "another" } # wrong type, expects String
      it 'is an invalid setting' do
-        expect { nullable_setting.validate_value }.to raise_error(ArgumentError, a_string_including("Invalid value"))
+        expect { nullable_setting.validate_value }.to raise_error(java.lang.IllegalArgumentException, a_string_including("Invalid value"))


Thrown by https://github.com/elastic/logstash/pull/16576/files#diff-fa84c030bfae07fb6da69b838c06f4448109e6fe35b66e23414d77dae63bb6ffR53

…stead of Ruby one

…ingWithDeprecatedAlias spec test.

…when it closes the logger context

Reworked log capture in setting_with_deprecated_alias_spec to use Logger's addAppender and a spy appender implementation. This leverages the idea to don't reconfigure the Log4J with a custom build configuration, but simply adding an appender that could spy the appended lines to a logger.

…e also in runner_spec, removing the programmatic configuration way

…eferences

elastic-sonarqube · 2024-11-13T08:58:03Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

kaisecheng

I have a minor suggestion on renaming StringSetting. I like your way to test with log appender. I will do more tests tmr.

kaisecheng · 2025-01-22T23:01:26Z

logstash-core/src/main/java/org/logstash/settings/StringSetting.java

+import java.util.Collections;
+import java.util.List;
+
+public class StringSetting extends BaseSetting<String> {


The setting classes have migrated Boolean, Coercible and NullableString to java. Do you think we can keep this class as String instead of StringSetting?

I tried to do that but then every instance of Java String needs to be referenced by its full classname.
In this case I think that having the suffix Setting make more explicit in the codebase to which class we are referring.
For Boolean was easier and less impacting. I think that for Logstash settings which names collides with Java standard type we could use the Setting postfix.
WDYT?

oh... naming is hard. Could we use SString ? The first S stands for Setting.
StringSetting sounds like referring a different thing comparing to Boolean.
😮‍💨 I can accept StringSetting if you think it is more comfortable

Could it worth to rename to SettingString so that the type part is last and maybe recall more that this class is closely related to the string type, like Boolean ?
I think that the issue is the original naming, type names has strong meaning in programming languages, so I would have used something else, maybe including the Setting suffix in every class.
WDYT?

Having a prefix for every class would be good, eg SettingBoolean, SettingString, SettingNullableString

Ok I rename StringSetting to SettingString for this PR then I'll create a follow up PR for the others

Tracked in #16946

kaisecheng

Tested locally and it works as expected. I hope we can find another name for Logstash::Setting::String

andsel · 2025-01-24T15:57:19Z

@logstashmachine backport 8.16

Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification.

andsel · 2025-01-24T16:22:18Z

8.17: ~~#16947~~ #16960
8.16: ~~#16948~~ #16961

kaisecheng · 2025-01-24T18:16:03Z

@andsel please also backport to 8.x

andsel · 2025-01-27T08:06:43Z

@logstashmachine backport 8.x

Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification.

andsel · 2025-01-27T14:23:50Z

8.x backport #16959

…16959) Non clean backport of #16576 ---- Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification. * Fixed the rename of NullableString to SettingNullableString * Fixed runner test to use real spy logger from Java Settings instead of mock test double

…16959) Non clean backport of #16576 ---- Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification. * Fixed the rename of NullableString to SettingNullableString * Fixed runner test to use real spy logger from Java Settings instead of mock test double (cherry picked from commit a0378c0)

…16576) (elastic#16959) Non clean backport of elastic#16576 ---- Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification. * Fixed the rename of NullableString to SettingNullableString * Fixed runner test to use real spy logger from Java Settings instead of mock test double

…16959) (#16960) Clean backport of #16959 from 8.x to 8.17 ---- Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification. * Fixed the rename of NullableString to SettingNullableString * Fixed runner test to use real spy logger from Java Settings instead of mock test double (cherry picked from commit a0378c0) Co-authored-by: Andrea Selva <selva.andre@gmail.com>

…16959) (#16961) Non clean backport of #16959 from 8.x to 8.16 (which is a non clean backport of #16576 to 8.x) Changes respect to 8.x: - in 8.x the `with_deprecated_alias` receives also a parameter specifying the version where the setting is removed. In this case we removed the extra parameter to the method https://github.com/elastic/logstash/blob/a0378c05cbcc8eda3ec0b2de286fb9ebde97df96/logstash-core/lib/logstash/environment.rb#L79 - fixed the test removing the check for the remove version https://github.com/elastic/logstash/blob/a0378c05cbcc8eda3ec0b2de286fb9ebde97df96/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb#L157 ---- Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification. * Fixed the rename of NullableString to SettingNullableString * Fixed runner test to use real spy logger from Java Settings instead of mock test double

Reimplements `LogStash::Setting::String` Ruby setting class into the `org.logstash.settings.SettingString` and exposes it through `java_import` as `LogStash::Setting::SettingString`. Updates the rspec tests in two ways: - logging mock is now converted to real Log4J appender that spy log line that are later verified - verifies `java.lang.IllegalArgumentException` instead of `ArgumentError` is thrown because the kind of exception thrown by Java code, during verification.

andsel self-assigned this Oct 17, 2024

jsvd added the status:work-in-progress label Oct 17, 2024

andsel force-pushed the feature/string_setting_to_java branch from f1abaf3 to 8ffcede Compare October 29, 2024 18:18

andsel force-pushed the feature/string_setting_to_java branch from c3bcbd5 to 0602b98 Compare November 12, 2024 10:10

andsel changed the title ~~Reimplemented LogStash::String setting in Java~~ Reimplement LogStash::String setting in Java Nov 12, 2024

andsel added the enhancement label Nov 12, 2024

andsel commented Nov 12, 2024

View reviewed changes

andsel added 12 commits November 12, 2024 17:00

Reimplemented LogStash::String setting in Java

b8f24c5

[WIP] Introduce StringSetting constructor that accepts 2 arguments

75ca3f8

Added String constructor with 2 arguments

0c311c7

Fixed nullable specs after the usage of the new Java StringSetting in…

35d3d93

…stead of Ruby one

Moved from logger double to real log spy using log4j appender in Sett…

2db9858

…ingWithDeprecatedAlias spec test.

[test] Updated remaining Logstash core test to use StringSetting

07e7325

Force to use freach Log4j configuration on each test, in particaular …

d9bdccb

…when it closes the logger context

Moved log appender spy class into spec_helper, used the same tecnhiqu…

5131596

…e also in runner_spec, removing the programmatic configuration way

Implemented NullableString in Java an updates latest String setting r…

2a13975

…eferences

Minor, removed commented code

61540ee

Mirrored rspec test for StringSetting into JUnit

180d597

andsel force-pushed the feature/string_setting_to_java branch from 91e2048 to 180d597 Compare November 12, 2024 16:04

Fixed failed test

dcae6f5

andsel marked this pull request as ready for review November 13, 2024 09:16

andsel linked an issue Nov 13, 2024 that may be closed by this pull request

Port the String Setting implementation from Ruby to Java #16672

Closed

andsel added 2 commits January 20, 2025 11:29

Merge branch 'main' into feature/string_setting_to_java

187af42

Fixed reference to stirng Java class after merge

605459c

kaisecheng self-requested a review January 22, 2025 17:02

kaisecheng reviewed Jan 22, 2025

View reviewed changes

kaisecheng approved these changes Jan 23, 2025

View reviewed changes

Renamed StringSetting to SettingString

6380a32

andsel mentioned this pull request Jan 24, 2025

[Backport 8.16] Reimplement LogStash::String setting in Java (#16576) #16948

Closed

5 tasks

andsel mentioned this pull request Jan 27, 2025

[Backport 8.x] Reimplement LogStash::String setting in Java (#16576) #16959

Merged

github-actions bot mentioned this pull request Jan 27, 2025

Backport PR #16959 to 8.17: [Backport 8.x] Reimplement LogStash::String setting in Java (#16576) #16960

Merged

andsel added the v8.17.2 label Jan 27, 2025

andsel mentioned this pull request Jan 27, 2025

[Backport 8.16] Reimplement LogStash::String setting in Java (#16559) #16961

Merged

5 tasks

andsel added the v8.16.4 label Jan 27, 2025

This was referenced Feb 26, 2025

Reimplement LogStash::Numeric setting in Java #17127

Merged

[Meta] Logstash Setting hierarchy move to Java #17157

Open

mergify bot mentioned this pull request Mar 6, 2025

[8.x] Reimplement LogStash::Numeric setting in Java (backport #17127) #17273

Merged

3 tasks

yaauie mentioned this pull request Apr 8, 2025

setting: enforce non-nullable string (restore 8.15.x behavior) #17522

Merged

3 tasks

andsel mentioned this pull request Sep 16, 2025

Move Port and PortRange Ruby settings to Java #17964

Merged

5 tasks

Conversation

andsel commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

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

Checklist

Author's Checklist

How to test this PR locally

Related issues

Uh oh!

andsel Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

elastic-sonarqube bot commented Nov 13, 2024

Quality Gate passed

Uh oh!

kaisecheng left a comment

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

andsel Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

andsel Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

andsel Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

andsel Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

kaisecheng left a comment

Choose a reason for hiding this comment

Uh oh!

andsel commented Jan 24, 2025

Uh oh!

andsel commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaisecheng commented Jan 24, 2025

Uh oh!

andsel commented Jan 27, 2025

Uh oh!

andsel commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andsel commented Oct 17, 2024 •

edited

Loading

andsel Jan 24, 2025 •

edited

Loading

andsel commented Jan 24, 2025 •

edited

Loading