From d16a082c91e31b9e9e8beea180dca8e8e6d371c7 Mon Sep 17 00:00:00 2001 From: andsel Date: Mon, 23 Feb 2026 15:10:39 +0100 Subject: [PATCH 1/2] Translated ArrayCoearcible to Java, doesn't remove the orig Ruby class because there are some subclasses, easier to remove when also the subclasses are moved. Updated all instantiations to use Java classes and not Rubyt ones (so use Java's String class and not RubyString, etc). Impemented equality methods on the setting, used in Ruby tests. These requires to compare Java's List instances, because RubyArray, despite implementing Java's List interfacem does a hard type check on RubyArray itself. --- logstash-core/lib/logstash/settings.rb | 2 + .../api/commands/default_metadata_spec.rb | 2 +- .../logstash/settings/array_coercible_spec.rb | 14 +-- logstash-core/spec/logstash/settings_spec.rb | 2 +- .../settings/ArrayCoercibleSetting.java | 106 ++++++++++++++++++ .../settings/ArrayCoercibleSettingTest.java | 91 +++++++++++++++ x-pack/lib/config_management/extension.rb | 6 +- 7 files changed, 211 insertions(+), 12 deletions(-) create mode 100644 logstash-core/src/main/java/org/logstash/settings/ArrayCoercibleSetting.java create mode 100644 logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java diff --git a/logstash-core/lib/logstash/settings.rb b/logstash-core/lib/logstash/settings.rb index b24bd6d2692..f9d74df9845 100644 --- a/logstash-core/lib/logstash/settings.rb +++ b/logstash-core/lib/logstash/settings.rb @@ -458,6 +458,8 @@ def validate(value) java_import org.logstash.settings.TimeValueSetting + java_import org.logstash.settings.ArrayCoercibleSetting + class ArrayCoercible < Coercible def initialize(name, klass, default, strict = true, &validator_proc) @element_class = klass diff --git a/logstash-core/spec/logstash/api/commands/default_metadata_spec.rb b/logstash-core/spec/logstash/api/commands/default_metadata_spec.rb index 9fe73b1e99f..7d0f0f64e1c 100644 --- a/logstash-core/spec/logstash/api/commands/default_metadata_spec.rb +++ b/logstash-core/spec/logstash/api/commands/default_metadata_spec.rb @@ -34,7 +34,7 @@ def registerIfNot(setting) before :all do registerIfNot(LogStash::Setting::BooleanSetting.new("xpack.monitoring.enabled", false)) - registerIfNot(LogStash::Setting::ArrayCoercible.new("xpack.monitoring.elasticsearch.hosts", String, ["http://localhost:9200"])) + registerIfNot(LogStash::Setting::ArrayCoercibleSetting.new("xpack.monitoring.elasticsearch.hosts", java.lang.String.java_class, ["http://localhost:9200"])) registerIfNot(LogStash::Setting::NullableStringSetting.new("xpack.monitoring.elasticsearch.username", "logstash_TEST system")) registerIfNot(LogStash::Setting::NullableStringSetting.new("xpack.monitoring.elasticsearch.username", "logstash_TEST system")) end diff --git a/logstash-core/spec/logstash/settings/array_coercible_spec.rb b/logstash-core/spec/logstash/settings/array_coercible_spec.rb index db0999f1095..c73e3bfc3f6 100644 --- a/logstash-core/spec/logstash/settings/array_coercible_spec.rb +++ b/logstash-core/spec/logstash/settings/array_coercible_spec.rb @@ -18,10 +18,10 @@ require "spec_helper" require "logstash/settings" -describe LogStash::Setting::ArrayCoercible do +describe LogStash::Setting::ArrayCoercibleSetting do subject { described_class.new("option", element_class, value) } let(:value) { [] } - let(:element_class) { Object } + let(:element_class) { java.lang.Object.java_class } context "when given a non array value" do let(:value) { "test" } @@ -43,16 +43,16 @@ describe "initialization" do subject { described_class } - let(:element_class) { Integer } + let(:element_class) { java.lang.Integer.java_class } context "when given values of incorrect element class" do let(:value) { "test" } it "will raise an exception" do - expect { described_class.new("option", element_class, value) }.to raise_error(ArgumentError) + expect { described_class.new("option", element_class, value) }.to raise_error(java.lang.IllegalArgumentException) end end context "when given values of correct element class" do - let(:value) { 1 } + let(:value) { java.lang.Integer.new(1) } it "will not raise an exception" do expect { described_class.new("option", element_class, value) }.not_to raise_error @@ -63,9 +63,9 @@ describe "#==" do context "when comparing two settings" do let(:setting_1) { described_class.new("option_1", element_class_1, value_1) } - let(:element_class_1) { String } + let(:element_class_1) { java.lang.String.java_class } let(:setting_2) { described_class.new("option_1", element_class_2, value_2) } - let(:element_class_2) { String } + let(:element_class_2) { java.lang.String.java_class } context "where one was given a non array value" do let(:value_1) { "a string" } diff --git a/logstash-core/spec/logstash/settings_spec.rb b/logstash-core/spec/logstash/settings_spec.rb index d5bee93f1f4..dc782b88d77 100644 --- a/logstash-core/spec/logstash/settings_spec.rb +++ b/logstash-core/spec/logstash/settings_spec.rb @@ -325,7 +325,7 @@ subject do settings = described_class.new - settings.register(LogStash::Setting::ArrayCoercible.new("host", String, [])) + settings.register(LogStash::Setting::ArrayCoercibleSetting.new("host", java.lang.String.java_class, [])) settings end diff --git a/logstash-core/src/main/java/org/logstash/settings/ArrayCoercibleSetting.java b/logstash-core/src/main/java/org/logstash/settings/ArrayCoercibleSetting.java new file mode 100644 index 00000000000..e61c36bfbe0 --- /dev/null +++ b/logstash-core/src/main/java/org/logstash/settings/ArrayCoercibleSetting.java @@ -0,0 +1,106 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.logstash.settings; + +import java.util.*; +import java.util.function.Predicate; + +public class ArrayCoercibleSetting extends Coercible { + + private final Class elementClass; + + @SuppressWarnings("this-escape") + public ArrayCoercibleSetting(String name, Class elementClass, Object defaultValue, boolean strict, + Predicate validator) { + super(name, defaultValue, false, validator); + this.elementClass = elementClass; + + if (strict) { + List coercedDefault = coerce(defaultValue); + validate(coercedDefault); + this.defaultValue = coercedDefault; + } else { + this.defaultValue = defaultValue; + } + } + + public ArrayCoercibleSetting(String name, Class elementClass, Object defaultValue, boolean strict) { + this(name, elementClass, defaultValue, strict, noValidator()); + } + + public ArrayCoercibleSetting(String name, Class elementClass, Object defaultValue) { + this(name, elementClass, defaultValue, true); + } + + @Override + public List coerce(Object value) { + if (value == null) { + return Collections.emptyList(); + } + if (value instanceof List) { + // We need to move away from RubyArray instances, because they equals method does a strong type checking. + // See https://github.com/jruby/jruby/blob/9.4.14.0/core/src/main/java/org/jruby/RubyArray.java#L5988-L5994 + // If a RubyArray is compared to something else that's not a RubyArray, return false, but this doesn't + // take care of cases when it's compared to a List, which is the case for our settings. + // So we need to create a new ArrayList with the same content. + return new ArrayList<>((List) value); + } + if (value instanceof Object[]) { + return Arrays.asList((Object[]) value); + } + return Collections.singletonList(value); + } + + @Override + public void validate(Object input) throws IllegalArgumentException { + super.validate(input); + + if (input == null) { + return; + } + + List inputList = (List) input; + for (Object element : inputList) { + if (!elementClass.isInstance(element)) { + throw new IllegalArgumentException( + String.format("Values of setting \"%s\" must be %s. Received: %s (%s)", + getName(), elementClass.getSimpleName(), element, + element != null ? element.getClass().getSimpleName() : "null")); + } + } + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + ArrayCoercibleSetting that = (ArrayCoercibleSetting) o; + return this.value().equals(that.value()); + } + + @Override + public int hashCode() { + return Objects.hashCode(this.value()); + } +} diff --git a/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java b/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java new file mode 100644 index 00000000000..2551d312fd2 --- /dev/null +++ b/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java @@ -0,0 +1,91 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.logstash.settings; + +import org.junit.Test; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThrows; + +public class ArrayCoercibleSettingTest { + + @SuppressWarnings("unchecked") + @Test + public void givenNonArrayValueWhenCoercedThenConvertedToSingleElementArray() { + ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", String.class, Collections.emptyList(), false); + + sut.set("test"); + + assertThat((List) sut.value(), contains("test")); + } + + @Test + public void givenArrayValueWhenCoercedThenNotModified() { + List initialValue = List.of("test"); + ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", String.class, initialValue, true); + + assertThat(sut.value(), equalTo(initialValue)); + } + + @Test + public void givenValuesOfIncorrectElementClassWhenInitializedThenThrowsException() { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> { + new ArrayCoercibleSetting("option", Integer.class, Collections.singletonList("test")); + }); + + assertThat(ex.getMessage(), containsString("Values of setting \"option\" must be Integer")); + } + + @SuppressWarnings("unchecked") + @Test + public void givenValuesOfCorrectElementClassWhenInitializedThenNoException() { + ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", Integer.class, Collections.singletonList(1)); + + assertThat((List) sut.value(), contains(1)); + } + + @SuppressWarnings("unchecked") + @Test + public void givenNullValueWhenCoercedThenReturnsEmptyList() { + ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", String.class, Collections.emptyList(), false); + + sut.set(null); + + assertThat((List) sut.value(), empty()); + } + + @Test + public void givenValuesOfIncorrectElementClassWhenSetThenThrowsException() { + ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", Integer.class, Collections.emptyList()); + + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> { + sut.set(Collections.singletonList("not an integer")); + }); + + assertThat(ex.getMessage(), containsString("Values of setting \"option\" must be Integer")); + } +} diff --git a/x-pack/lib/config_management/extension.rb b/x-pack/lib/config_management/extension.rb index 1f5c6844248..844e5ceeee5 100644 --- a/x-pack/lib/config_management/extension.rb +++ b/x-pack/lib/config_management/extension.rb @@ -23,10 +23,10 @@ def additionals_settings(settings) settings.register(LogStash::Setting::BooleanSetting.new("xpack.management.enabled", false)) settings.register(LogStash::Setting::TimeValueSetting.new("xpack.management.logstash.poll_interval", "5s")) - settings.register(LogStash::Setting::ArrayCoercible.new("xpack.management.pipeline.id", String, ["main"])) + settings.register(LogStash::Setting::ArrayCoercibleSetting.new("xpack.management.pipeline.id", java.lang.String.java_class, ["main"])) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.username", "logstash_system")) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.password")) - settings.register(LogStash::Setting::ArrayCoercible.new("xpack.management.elasticsearch.hosts", String, ["https://localhost:9200"])) + settings.register(LogStash::Setting::ArrayCoercibleSetting.new("xpack.management.elasticsearch.hosts", java.lang.String.java_class, ["https://localhost:9200"])) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.cloud_id")) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.cloud_auth")) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.api_key")) @@ -39,7 +39,7 @@ def additionals_settings(settings) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.ssl.keystore.password")) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.ssl.certificate")) settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.ssl.key")) - settings.register(LogStash::Setting::ArrayCoercible.new("xpack.management.elasticsearch.ssl.cipher_suites", String, [])) + settings.register(LogStash::Setting::ArrayCoercibleSetting.new("xpack.management.elasticsearch.ssl.cipher_suites", java.lang.String.java_class, [])) settings.register(LogStash::Setting::StringSetting.new("xpack.management.elasticsearch.ssl.verification_mode", "full", true, %w[none certificate full])) settings.register(LogStash::Setting::BooleanSetting.new("xpack.management.elasticsearch.sniffing", false)) rescue => e From aa75d09d92eb9359b55bfe58cb00dbd15e66251c Mon Sep 17 00:00:00 2001 From: andsel Date: Fri, 27 Feb 2026 14:07:42 +0100 Subject: [PATCH 2/2] [Test] added JUnit tests to make the coverage in par with Ruby spec --- .../settings/ArrayCoercibleSettingTest.java | 71 ++++++++++++++++--- 1 file changed, 62 insertions(+), 9 deletions(-) diff --git a/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java b/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java index 2551d312fd2..2961fa2b819 100644 --- a/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java +++ b/logstash-core/src/test/java/org/logstash/settings/ArrayCoercibleSettingTest.java @@ -20,7 +20,6 @@ import org.junit.Test; -import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -28,7 +27,8 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; -import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThrows; public class ArrayCoercibleSettingTest { @@ -48,7 +48,7 @@ public void givenArrayValueWhenCoercedThenNotModified() { List initialValue = List.of("test"); ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", String.class, initialValue, true); - assertThat(sut.value(), equalTo(initialValue)); + assertEquals(initialValue, sut.value()); } @Test @@ -79,13 +79,66 @@ public void givenNullValueWhenCoercedThenReturnsEmptyList() { } @Test - public void givenValuesOfIncorrectElementClassWhenSetThenThrowsException() { - ArrayCoercibleSetting sut = new ArrayCoercibleSetting("option", Integer.class, Collections.emptyList()); + public void givenTwoSettingsWithSameNonArrayValueThenAreEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, "a string"); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, "a string"); - IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> { - sut.set(Collections.singletonList("not an integer")); - }); + assertEquals(setting1, setting2); + } - assertThat(ex.getMessage(), containsString("Values of setting \"option\" must be Integer")); + @Test + public void givenTwoSettingsOneNonArrayAndOneArrayWithSameValueThenAreEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, "a string"); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + + assertEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsWithDifferentNonArrayValuesThenAreNotEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, "a string"); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, "a different string"); + + assertNotEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsWithNonArrayAndDifferentValueInArrayThenAreNotEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, "a string"); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, List.of("a different string")); + + assertNotEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsBothHaveSameArrayValueThenAreEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + + assertEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsArrayAndNonArrayWithSameValueThenAreEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, "a string"); + + assertEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsWithDifferentArrayValuesThenAreNotEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, List.of("a different string")); + + assertNotEquals(setting1, setting2); + } + + @Test + public void givenTwoSettingsWithArrayAndDifferentNonArrayValueThenAreNotEqual() { + ArrayCoercibleSetting setting1 = new ArrayCoercibleSetting("option_1", String.class, List.of("a string")); + ArrayCoercibleSetting setting2 = new ArrayCoercibleSetting("option_1", String.class, "a different string"); + + assertNotEquals(setting1, setting2); } }