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

[yaauie]

Release notes

• Fixes an issue introduced in 8.16.4, 8.17.2, and 8.18.0 where null values were incorrectly accepted for non-nullable settings.

What does this PR do?

Ensures that SettingString is correctly non-nullable and SettingNullableString is nullable.

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

Improves error messages and reliability of behaviour when Logstash is misconfigured.

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

How to test this PR locally

With Logstash 8.15.5, set a non-nullable string setting to null in the logstash.yml and run Logstash, observing that it refuses to start and gives guidance about the wrong setting

(cd logstash-8.15.5 && echo 'keystore.file: null' > config/logstash.yml && bin/logstash -e 'input { generator { count => 1 } }')

Your settings are invalid. Reason: Setting "keystore.file" must be a String. Received:  (NilClass)
[2025-04-08T21:51:30,611][FATAL][org.logstash.Logstash    ] Logstash stopped processing because of an error: (SystemExit) exit

Repeat with Logstash 8.17.4 and observe that runtime errors that are less clear occur later

(cd logstash-8.17.4 && echo 'keystore.file: null' > config/logstash.yml && bin/logstash -e 'input { generator { count => 1 } }')

[2025-04-08T22:46:34,161][ERROR][logstash.agent           ] Failed to execute action {:action=>LogStash::PipelineAction::Create/pipeline_id:main, :exception=>"TypeError", :message=>"nil is not a string", :backtrace=>["org/logstash/execution/AbstractPipelineExt.java:293:in `initialize'", "org/logstash/execution/AbstractPipelineExt.java:227:in `initialize'", "/Users/rye/src/elastic/scratch/logstash-8.17.4/logstash-core/lib/logstash/java_pipeline.rb:47:in `initialize'", "org/jruby/RubyClass.java:949:in `new'", "/Users/rye/src/elastic/scratch/logstash-8.17.4/logstash-core/lib/logstash/pipeline_action/create.rb:50:in `execute'", "/Users/rye/src/elastic/scratch/logstash-8.17.4/logstash-core/lib/logstash/agent.rb:420:in `block in converge_state'"]}
[2025-04-08T22:46:34,188][INFO ][logstash.agent           ] Successfully started Logstash API endpoint {:port=>9600, :ssl_enabled=>false}
[2025-04-08T22:46:34,192][INFO ][logstash.runner          ] Logstash shut down.
[2025-04-08T22:46:34,194][FATAL][org.logstash.Logstash    ] Logstash stopped processing because of an error: (SystemExit) exit

Repeat with this patch, and observe the restored behaviour:

echo 'keystore.file: null' > config/logstash.yml && bin/logstash -e 'input { generator { count => 1 } }'

Your settings are invalid. Reason: Setting "keystore.file" must be a String. Received:  (NilClass)
[2025-04-08T22:49:04,092][FATAL][org.logstash.Logstash    ] Logstash stopped processing because of an error: (SystemExit) exit

Related issues

Caused-By: #16576

[mergify]

This pull request does not have a backport label. Could you fix it @yaauie? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
• backport-8.x is the label to automatically backport to the 8.x branch.
• If no backport is necessary, please add the backport-skip label

[elastic-sonarqube]

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]

💚 Build Succeeded

• Buildkite Build
• Commit: 86af7b0

[andsel]

I would propose to share common test code in inner abstract classes, that contains all the shared test methods, like:

diff --git a/logstash-core/src/test/java/org/logstash/settings/SettingNullableStringTest.java b/logstash-core/src/test/java/org/logstash/settings/SettingNullableStringTest.java
index aaa3cee7c..38b585537 100644
--- a/logstash-core/src/test/java/org/logstash/settings/SettingNullableStringTest.java
+++ b/logstash-core/src/test/java/org/logstash/settings/SettingNullableStringTest.java
@@ -14,58 +14,46 @@ import static org.junit.Assert.*;
 @RunWith(Enclosed.class)
 public class SettingNullableStringTest {
 
-    public static class WithValueConstraintCase{
-        private static final List<String> POSSIBLE_VALUES = List.of("a", "b", "c");
-        private SettingString sut;
-
-        @Before
-        public void setUp() throws Exception {
-            sut = new SettingNullableString("mytext", POSSIBLE_VALUES.iterator().next(), true, POSSIBLE_VALUES);
-        }
-
-        @Test
-        public void whenSetConstrainedToValueNotPresentInPossibleValuesThenThrowAHelpfulError() {
-            IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> {
-                sut.set("d");
-            });
-            assertThat(ex.getMessage(), containsString("Invalid value \"mytext: d\""));
-        }
+    private abstract static class ValuesSharedChecks {
+        protected SettingString sut;
 
         @Test
-        public void whenSetConstrainedToValuePresentInPossibleValuesThenSetValue() {
+        public void whenSetToNonNullValueThenSetValue() {
             sut.set("a");
 
             assertEquals("a", sut.value());
         }
 
         @Test
-        public void whenSetConstrainedToNullThenSetValue() {
+        public void whenSetToNullThenSetValue() {
             sut.set(null);
 
             assertNull(sut.value());
         }
     }
 
-    public static class WithoutValueConstraintCase {
-        private SettingString sut;
+    public static class WithValueConstraintCase extends ValuesSharedChecks {
+        private static final List<String> POSSIBLE_VALUES = List.of("a", "b", "c");
 
         @Before
         public void setUp() throws Exception {
-            sut = new SettingNullableString("mytext", "foo", true);
+            sut = new SettingNullableString("mytext", POSSIBLE_VALUES.iterator().next(), true, POSSIBLE_VALUES);
         }
 
         @Test
-        public void whenSetUnconstrainedToNonNullValueThenSetValue() {
-            sut.set("a");
-
-            assertEquals("a", sut.value());
+        public void whenSetConstrainedToValueNotPresentInPossibleValuesThenThrowAHelpfulError() {
+            IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> {
+                sut.set("d");
+            });
+            assertThat(ex.getMessage(), containsString("Invalid value \"mytext: d\""));
         }
+    }
 
-        @Test
-        public void whenSetUnconstrainedToNullThenSetValue() {
-            sut.set(null);
+    public static class WithoutValueConstraintCase extends ValuesSharedChecks {
 
-            assertNull(sut.value());
+        @Before
+        public void setUp() throws Exception {
+            sut = new SettingNullableString("mytext", "foo", true);
         }
     }
 }
\ No newline at end of file
diff --git a/logstash-core/src/test/java/org/logstash/settings/SettingStringTest.java b/logstash-core/src/test/java/org/logstash/settings/SettingStringTest.java
index 4661bcbbe..f740405e6 100644
--- a/logstash-core/src/test/java/org/logstash/settings/SettingStringTest.java
+++ b/logstash-core/src/test/java/org/logstash/settings/SettingStringTest.java
@@ -33,32 +33,18 @@ import static org.junit.Assert.assertThrows;
 @RunWith(Enclosed.class)
 public class SettingStringTest {
 
-    public static class WithValueConstraintCase {
-        private static final List<String> POSSIBLE_VALUES = List.of("a", "b", "c");
-        private SettingString sut;
-
-        @Before
-        public void setUp() throws Exception {
-            sut = new SettingString("mytext", POSSIBLE_VALUES.iterator().next(), true, POSSIBLE_VALUES);
-        }
-
-        @Test
-        public void whenSetValueNotPresentInPossibleValuesThenThrowAHelpfulError() {
-            IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> {
-                sut.set("d");
-            });
-            assertThat(ex.getMessage(), containsString("Invalid value \"mytext: d\""));
-        }
+    private abstract static class ValuesSharedChecks {
+        protected SettingString sut;
 
         @Test
-        public void whenSetConstrainedToValuePresentInPossibleValuesThenSetValue() {
+        public void whenSetToValuePresentInPossibleValuesThenSetValue() {
             sut.set("a");
 
             assertEquals("a", sut.value());
         }
 
         @Test
-        public void whenSetConstrainedToNullThenThrowAHelpfulError() {
+        public void whenSetToNullThenThrowAHelpfulError() {
             IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> {
                 sut.set(null);
             });
@@ -66,27 +52,28 @@ public class SettingStringTest {
         }
     }
 
-    public static class WithoutValueConstraintCase {
-        private SettingString sut;
+    public static class WithValueConstraintCase extends ValuesSharedChecks {
+        private static final List<String> POSSIBLE_VALUES = List.of("a", "b", "c");
 
         @Before
         public void setUp() throws Exception {
-            sut = new SettingString("mytext", "foo", true);
-        }
-
-        @Test
-        public void whenSetUnconstrainedToNonNullValueThenSetValue() {
-            sut.set("a");
-
-            assertEquals("a", sut.value());
+            sut = new SettingString("mytext", POSSIBLE_VALUES.iterator().next(), true, POSSIBLE_VALUES);
         }
 
         @Test
-        public void whenSetUnconstrainedToNullThenThrowAHelpfulError() {
+        public void whenSetValueNotPresentInPossibleValuesThenThrowAHelpfulError() {
             IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> {
-                sut.set(null);
+                sut.set("d");
             });
-            assertThat(ex.getMessage(), containsString("Setting \"mytext\" must be a String"));
+            assertThat(ex.getMessage(), containsString("Invalid value \"mytext: d\""));
+        }
+    }
+
+    public static class WithoutValueConstraintCase extends ValuesSharedChecks {
+
+        @Before
+        public void setUp() throws Exception {
+            sut = new SettingString("mytext", "foo", true);
         }
     }
 }
\ No newline at end of file

But present two possible issues:

1. we have test method with duplicated names and doesn't provide the full context when reading the test name
2. in case of error in one of the shared tests we have to check the stack trace to understand the source of the error:

org.junit.ComparisonFailure: expected:<[b]> but was:<[a]>
	at org.junit.Assert.assertEquals(Assert.java:117)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at org.logstash.settings.SettingStringTest$ValuesSharedChecks.whenSetToValuePresentInPossibleValuesThenSetValue(SettingStringTest.java:43)
	at org.logstash.settings.SettingStringTest$WithoutValueConstraintCase.whenSetToValuePresentInPossibleValuesThenSetValue(SettingStringTest.java:72)	

[andsel]

Left a comment to avoid code duplication, but feel free to skip it because potentially introduce some unclear error reports.

LGTM

[yaauie]

I would propose to share common test code in inner abstract classes, that contains all the shared test methods, like:

[...]

But present two possible issues:

1. we have test method with duplicated names and doesn't provide the full context when reading the test name
2. in case of error in one of the shared tests we have to check the stack trace to understand the source of the error:

I agree on both fronts. My first pass used shared abstract base class, but I elected for duplication because in the end it made the test output more clear.

If we were using Junit Jupiter, we could use @Nested which handles both of these concerns significantly better, but we're constrained to use things like @RunWith(Enclosed.class) in Junit4.