Reimplement LogStash::String setting in Java #16576

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

      end
    end
    context 'to an invalid value not in the allow-list' do
      let(:possible_strings) { %w(this that)}
      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

elastic-sonarqube · 2025-01-24T15:10:23Z

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

elasticmachine · 2025-01-24T15:23:42Z

💚 Build Succeeded

Buildkite Build
Commit: c212855

History

💚 Build #2124 succeeded 6380a32
💚 Build #2086 succeeded 605459c
💔 Build #2085 failed 187af42
💚 Build #1846 succeeded dcae6f5
💔 Build #1842 failed 180d597
💛 Build #1841 was flaky 91e2048

cc @andsel

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

@logstashmachine backport 8.17

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
8.16: #16948

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

@andsel please also backport to 8.x

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

Renamed NullableString to SettingNullableString

c212855

andsel merged commit 03b11e9 into elastic:main Jan 24, 2025
6 checks passed

andsel mentioned this pull request Jan 24, 2025

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

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement LogStash::String setting in Java #16576

Reimplement LogStash::String setting in Java #16576

andsel commented Oct 17, 2024 •

edited

Loading

andsel Nov 12, 2024

elastic-sonarqube bot commented Nov 13, 2024

kaisecheng left a comment

kaisecheng Jan 22, 2025

andsel Jan 23, 2025

kaisecheng Jan 23, 2025

andsel Jan 24, 2025 •

edited

Loading

kaisecheng Jan 24, 2025

andsel Jan 24, 2025

andsel Jan 24, 2025

kaisecheng left a comment

elastic-sonarqube bot commented Jan 24, 2025

elasticmachine commented Jan 24, 2025

andsel commented Jan 24, 2025

andsel commented Jan 24, 2025

andsel commented Jan 24, 2025

kaisecheng commented Jan 24, 2025

Reimplement LogStash::String setting in Java #16576

Reimplement LogStash::String setting in Java #16576

Conversation

andsel commented Oct 17, 2024 • edited Loading

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

andsel Nov 12, 2024

Choose a reason for hiding this comment

elastic-sonarqube bot commented Nov 13, 2024

Quality Gate passed

kaisecheng left a comment

Choose a reason for hiding this comment

kaisecheng Jan 22, 2025

Choose a reason for hiding this comment

andsel Jan 23, 2025

Choose a reason for hiding this comment

kaisecheng Jan 23, 2025

Choose a reason for hiding this comment

andsel Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

kaisecheng Jan 24, 2025

Choose a reason for hiding this comment

andsel Jan 24, 2025

Choose a reason for hiding this comment

andsel Jan 24, 2025

Choose a reason for hiding this comment

kaisecheng left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Jan 24, 2025

Quality Gate passed

elasticmachine commented Jan 24, 2025

💚 Build Succeeded

History

andsel commented Jan 24, 2025

andsel commented Jan 24, 2025

andsel commented Jan 24, 2025

kaisecheng commented Jan 24, 2025

andsel commented Oct 17, 2024 •

edited

Loading

andsel Jan 24, 2025 •

edited

Loading