[Spacetime] Reimplement config Setting classe in java

[andsel]

Release notes

[rn:skip]

What does this PR do?

Reimplement the root Ruby Setting class in Java and use it from the Ruby one moving the original Ruby class to a shell wrapping the Java instance.
In particular create a new symmetric hierarchy (at the time just for Setting, Coercible and Boolean classes) to the Ruby one, moving also the feature for setting deprecation. In this way the new org.logstash.settings.Boolean is syntatically and semantically equivalent to the old Ruby Boolean class, which replaces.

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

This PR allows to move the Ruby Setting subclasses one at a time without breaking existing code.

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

• Fix the Ruby side of hierarchy to have green tests

How to test this PR locally

The test consist in switching on and off a boolean setting and check the selection is respected. Consider the api.enabled and its deprecated version named http.enabled, tinker with it in config/logstash.yml and as command line switch --api.enabled true|false. Check with curl.

1. set to false api.enabled in logstash.yml
2. run Logstash. with bin/logstash -e "input{stdin{}} output{stdout{}}"
3. verify with

curl -v "http://localhost:9600"

4. Use the command line switch

bin/logstash -e "input{stdin{}} output{stdout{}}" --api.enabled false 

Related issues

• Closes [WIP] Feature/move settings ruby to java #12531
• Closes [WIP]Rought idea of translating Ruby setting into Java using a Ruby shell to immutable Java implementation #12566

Use cases

Screenshots

Logs

[elastic-sonarqube]

Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

[andsel]

Note for reviewer:

nullifying the default value is already done by the deprecate method when create the new instance of the Java Setting class.

[yaauie]

This looks like an excellent step in the right direction.

From it, I see new opportunities (and also a reminder of some of my own frustrations with the existing implementation).

I like the introduction of the settings builder, and hope that we can eventually migrate from the many-positional-parameters settings declarations in environment.rb to something that is more readable, using the builder pattern.

[yaauie]

While it is likely out of scope for this PR, we really have two independent needs for settings: (1) applying user input during process and pipeline start-up and (2) working with the effective values at runtime.

The existing implementation (and therefore the one that you ported) provides objects that address both needs, which means that we have access to mutating explicitly-given settings in places where we have no need to do so.

At some point I would like to be able to have a minimal read-only view, or at least an interface that exposes such, so that we can minimize the risk of explicitly-given settings being overridden by developer error.

Something like:

package co.elastic.logstash.api;

import java.util.function.Supplier;
import java.util.function.UnaryOperator;

public interface Setting<TYPE> {
    /**
     * @return the canonical dot-notation name of this setting
     */
    String getName();

    /**
     * @param valueMapper a mapper that receives the <em>effective</em> value and an
     *                    indication if that is an explicitly-given value, and returns
     *                    a possibly-modified result
     * @return the result of applying the <em>effective</em> value and a flag to the {@code valueMapper}.
     */
    TYPE value(ValueMapper<TYPE> valueMapper);
    
    @FunctionalInterface
    interface ValueMapper<TYPE> {
        TYPE apply(TYPE effectiveValue, boolean isExplicit);
    }
    
    /**
     * @return the explicitly-given value, if set; otherwise the default value
     */
    default TYPE value() {
        return value(((effectiveValue, isExplicit) -> effectiveValue));
    }

    /**
     * @param defaultValueMapper a mapper with which to transform or replace the default value
     * @return the explicitly-given value, if set; otherwise the result of providing
     *         the <em>default</em> value to the {@code defaultValueMapper}.
     */
    default TYPE value(final UnaryOperator<TYPE> defaultValueMapper) {
        return value((effectiveValue, isExplicit) -> isExplicit ? effectiveValue : defaultValueMapper.apply(effectiveValue));
    };

    /**
     * @param defaultValueSupplier a supplier that <em>replaces</em> the default value
     * @return the explicitly-given value, if set; otherwise a default value provided by
     *         the {@code defaultValueSupplier}, <em>ignoring</em> the setting's default.
     */
    default TYPE value(final Supplier<TYPE> defaultValueSupplier) {
        return value((ignoredDefault) -> defaultValueSupplier.get());
    }
}

We could then implement this interface here in the base-class so that all of our implementations could be used as read-only settings.

[andsel]

That would be nice, I think it's would better addressed in a follow up PR. The scope of this PR is to try to do a 1:1 move as mush as possible keeping the existing API.
Once the full setting hierarchy is moved to Java, with unit tests also, we can do such API refactoring easier.
The base idea, separating the writer from the reader, which is inspired by single-responsibility principle, is a great suggestion 👍
Let me know if you push to have such change in this PR.

[andsel]

@yaauie this is ready for another review :-)

[yaauie]

I found an issue with how deprecated aliases are handled, in that they do not coerce the input because the DeprecatedAlias is instantiated with a fresh Setting that doesn't carry any of the type-specific behaviour of the original setting. I've left some suggestions around how we can handle that.

There is also an issue where if a canonical setting and its deprecated counterpart are set, we no longer get an error:

╭─{ rye@perhaps:~/src/elastic/logstash@main (feature/reimplement_config_classes_in_java ✘) }
╰─● (echo 'api.enabled: true'"\n"'http.enabled: true' > config/logstash.yml) && bin/logstash -e 'input { generator { count => 1 } }' | grep API 
[2024-07-11T22:07:04,841][INFO ][logstash.agent           ] Successfully started Logstash API endpoint {:port=>9600, :ssl_enabled=>false}
[success (00:00:07)]                                                                                                                                                                                       

╭─{ rye@perhaps:~/src/elastic/scratch/202407-settings-refactor/logstash-8.13.2 }
╰─○ (echo 'api.enabled: true'"\n"'http.enabled: true' > config/logstash.yml) && bin/logstash -e 'input { generator { count => 1 } }' | grep API
Your settings are invalid. Reason: Both `api.enabled` and its deprecated alias `http.enabled` have been set. Please only set `api.enabled`
[error: 1 (00:00:06)]

[yaauie]

While <T> Coercible<T>#coerce(Object) will take any value, void Coercible#set(T) effectively requires that the caller uses this to pre-coerce whatever value it is working with.

I think this can be solved in one of several ways:

• change signature of Setting#set to receive an Object, and make its implementation do a class cast before actually setting the value. The downside of this is that we defer compile-time type-safety (that we aren't using anyway) to be runtime class cast exception. If we introduced a method with a different erasure (like Setting#trySet(Object) or similar), we would need to use it wherever we don't have the right type so leaving the type-safe Setting#set(T) retains little value.
• change Setting<T> to be something like Setting<VALUE,ACCEPT> (and map the types in its methods to either the value-type or a type that is accepted for setting/validating/coercing), which would allow coercible to be defined as Coercible<T> extends Setting<T,Object>. This would mean that any callers that only need read-access would need to declare type-related information that they don't care about unless declaring a concrete Setting type that does it for them.

[andsel]

The motivation for the void set(T value) was to express more clearly what set expect, a sort of contract, which in my expectations, once we have all the Settings re-implemented in Java, would make evident some type-related problems in the Setting hierarchy.
One of the thing that I tried to do is to keep a syntactic compatibility of new Java classes with previous Ruby classes, to limit changes to existing code.

If I understand correctly your second proposal consist in defining 2 type params, one for the "read value" (VALUE) so that a Boolean setting would use Boolean (java type object) and is used in every "read" Setting's method.
The ACCEPT is instead the data type used to set the value, so for example could say that Boolean setting accepts and decodes String value like

class Boolean extends Setting<Boolean, String>

That would be a nice idea, but as of today the set is invoked only from Ruby code and maybe that is an overkill.

I would choose the path to use Object as param type of the set method, but this creates other problems:

• the validate method in Setting's root class should also accept an Object type: code ref
• also the value field in Setting root at that point miss the type and become an Object : code ref

I understand the actual proposition is less then optimal, but moving to Object would force to loose the type in many parts.

WDYT? Do you foresee a problem in keeping the type also in coerced set?

[andsel]

@yaauie please could you review my last 🔝 comment 🙏

[yaauie]

As defined here, any caller of this Coercible<T> (or its subclasses) will (a) need to know that the setting is Coercible, and (b) will need to use T Coercible<T>#coerce(Object) to transform the Object value into an instance of T that can be accepted by Coercible<T>#set(T). This significantly breaks the encapsulation.

A middle-ground could be:

• (A) renaming Setting<T>#set(T) -> Setting<T>#setSafely(T) and introducing Setting<T>#set(Object) with a default implementation that merely does a class-cast (therefore being exceptional if given the wrong type). In this way the caller can use one method if they know that they are working with the expected type, and the other to rely on coercion without needing to introspect the setting that they are working with.
• (B) Implementing both Coercible<T>#set(Object) and Coercible<T>#setSafely(T) to use Coercible<T>coerce(Object), and having a private shared Coercible<T>#set(T) that performs the always-on validation but doesn't re-coerce the value

I whipped up a quick diff representing the above here.

[andsel]

Thank's for the suggestion I've completely missed that forcing the client of CoercibleSetting.set to know upfront the type of the value to set is a violation of the encapsulation.
I've integrated you suggestion, the only observation about point B

having a private shared Coercible#set(T)

We can't have it because would conflict with set(Object) due to type erasure, we could override methods only if they have different argument lists, but for the JVM set(Object val) and set(T val) are the same method.

[yaauie]

🤦🏼 A previous version of the diff had a private shared Coercible<T>#setInternal(T) to handle the validation etc.:

    @Override
    public void setSafely(T value) {
        this.setInternal(coerce(value));
    }

    @Override
    public void set(Object newValue) {
        this.setInternal(coerce(newValue));
    }

    private void setInternal(T coercedValue) {
        validate(coercedValue);
        super.setSafely(coercedValue);
    }

The edited version I ended up posting merely inverted the responsibilities of Coercible<T>#set(Object) and Coercible<T>#setSafely(T) to avoid double-coercing.

    @Override
    public void set(Object value) {
        T coercedValue = coerce(value);
        validate(coercedValue);
        super.setSafely(coercedValue);
    }

    @Override
    public void setSafely(T value) {
        this.set(value);
    }

[elastic-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

[yaauie]

It looks like we are closing in on it, but we need to ensure that the deprecation logs actually get emitted when using a deprecated setting. Since the deprecation logger is configured after the settings are parsed (see #16339), that means we need to implement the hook to observe the value after settings have been fully-loaded, and emit the deprecation log there.

I was able to get it working:

diff --git a/config/log4j2.properties b/config/log4j2.properties
index 1c676fefe..92acf87be 100644
--- a/config/log4j2.properties
+++ b/config/log4j2.properties
@@ -154,7 +154,7 @@ appender.deprecation_rolling.policies.size.size = 100MB
 appender.deprecation_rolling.strategy.type = DefaultRolloverStrategy
 appender.deprecation_rolling.strategy.max = 30
 
-logger.deprecation.name = org.logstash.deprecation, deprecation
+logger.deprecation.name = org.logstash.deprecation
 logger.deprecation.level = WARN
 logger.deprecation.appenderRef.deprecation_rolling.ref = deprecation_plain_rolling
 logger.deprecation.additivity = false
diff --git a/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java b/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java
index c2fd1d45e..afe7b9afa 100644
--- a/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java
+++ b/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java
@@ -21,14 +21,16 @@ package org.logstash.settings;
 
 import co.elastic.logstash.api.DeprecationLogger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.logstash.log.DefaultDeprecationLogger;
 
 /**
  * A <code>DeprecatedAlias</code> provides a deprecated alias for a setting, and is meant
  * to be used exclusively through @see org.logstash.settings.SettingWithDeprecatedAlias#wrap()
  * */
-final class DeprecatedAlias<T> extends SettingDelegator<T> {
-    private final DeprecationLogger deprecationLogger = new DefaultDeprecationLogger(LogManager.getLogger(DeprecatedAlias.class));
+public final class DeprecatedAlias<T> extends SettingDelegator<T> {
+    private static final Logger LOGGER = LogManager.getLogger();
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DefaultDeprecationLogger(LOGGER);
 
     private SettingWithDeprecatedAlias<T> canonicalProxy;
 
@@ -37,16 +39,9 @@ final class DeprecatedAlias<T> extends SettingDelegator<T> {
         this.canonicalProxy = canonicalProxy;
     }
 
-    @Override
-    public void setSafely(T newValue) {
-        deprecationLogger.deprecated("The setting `{}` is a deprecated alias for `{}` and will be removed in a " +
-                "future release of Logstash. Please use {} instead", getName(), canonicalProxy.getName(), canonicalProxy.getName());
-        super.setSafely(newValue);
-    }
-
     @Override
     public T value() {
-        deprecationLogger.deprecated("The value of setting `{}` has been queried by its deprecated alias `{}`. " +
+        LOGGER.warn("The value of setting `{}` has been queried by its deprecated alias `{}`. " +
                 "Code should be updated to query `{}` instead", canonicalProxy.getName(), getName(), canonicalProxy.getName());
         return super.value();
     }
@@ -58,4 +53,11 @@ final class DeprecatedAlias<T> extends SettingDelegator<T> {
             getDelegate().validateValue();
         }
     }
+
+    public void observePostProcess() {
+        if (isSet()) {
+            DEPRECATION_LOGGER.deprecated("The setting `{}` is a deprecated alias for `{}` and will be removed in a " +
+                    "future release of Logstash. Please use `{}` instead", getName(), canonicalProxy.getName(), canonicalProxy.getName());
+        }
+    }
 }

Notably:

• the DeprecatedAlias<T>#observePostProcess() is only exposed to the ruby-caller if the DeprecatedAlias class and that method are both public
• the org.logstash.deprecated logger isn't routed to the deprecation log when the log4j2.configuration has logger.deprecation.name set to an array, but the other value is already covered by the subsequent config
• using DeprecatedAlias<T>#value() is an us-the-developers problem, not a user-of-logstash problem, so it should get routed to the regular logger as a warning (not to the deprecation logger).

[yaauie]

😩 I found one more issue with the message that is provided to the user when they provide an incoercible value to a boolean setting, and have provided a suggestion that fixes the issue.

Once that suggestion is applied this will have my approval.

[yaauie]

When we fail to coerce, our message contains the existing value, not the one we attempted to coerce:

╭─{ rye@perhaps:~/src/elastic/logstash@main (validate/feature/reimplement_config_classes_in_java ✘) }
╰─● (echo 'api.enabled: bananas' > config/logstash.yml) && bin/logstash --log.level=debug -e 'input { generator { count => 1 } }'
Using system java: /Users/rye/.jenv/shims/java
ERROR: Failed to load settings file from "path.settings". Aborting... path.setting=/Users/rye/src/elastic/logstash@main/config, exception=Java::JavaLang::IllegalArgumentException, message=>could not coerce true into a boolean
[FATAL] 2024-09-10 14:03:47.613 [main] Logstash - Logstash stopped processing because of an error: (SystemExit) exit
org.jruby.exceptions.SystemExit: (SystemExit) exit
        at org.jruby.RubyKernel.exit(org/jruby/RubyKernel.java:921) ~[jruby.jar:?]
        at org.jruby.RubyKernel.exit(org/jruby/RubyKernel.java:880) ~[jruby.jar:?]
        at Users.rye.src.elastic.logstash_at_40_main.lib.bootstrap.environment.<main>(/Users/rye/src/elastic/logstash@main/lib/bootstrap/environment.rb:90) ~[?:?]
[error: 1 (00:00:05)]                                                                                                                                                                                                                                                                                                                                                                                                            

Suggested change

	public java.lang.Boolean coerce(Object obj) {
	if (obj instanceof String) {
	switch((String) obj) {
	case "true": return true;
	case "false": return false;
	default: throw new IllegalArgumentException("could not coerce " + value() + " into a boolean");
	}
	}
	if (obj instanceof java.lang.Boolean) {
	return (java.lang.Boolean) obj;
	}
	throw new IllegalArgumentException("could not coerce " + value() + " into a boolean");
	}
	public java.lang.Boolean coerce(Object obj) {
	if (obj instanceof String) {
	switch((String) obj) {
	case "true": return true;
	case "false": return false;
	default: throw new IllegalArgumentException(coercionFailureMessage(obj));
	}
	}
	if (obj instanceof java.lang.Boolean) {
	return (java.lang.Boolean) obj;
	}
	throw new IllegalArgumentException(coercionFailureMessage(obj));
	}
	private String coercionFailureMessage(Object obj) {
	return String.format("Cannot coerce `%s` to boolean", obj);
	}

It would also likely be helpful if the crashing exception included which setting we were attempting coerce a value for:

Suggested change

	public java.lang.Boolean coerce(Object obj) {
	if (obj instanceof String) {
	switch((String) obj) {
	case "true": return true;
	case "false": return false;
	default: throw new IllegalArgumentException("could not coerce " + value() + " into a boolean");
	}
	}
	if (obj instanceof java.lang.Boolean) {
	return (java.lang.Boolean) obj;
	}
	throw new IllegalArgumentException("could not coerce " + value() + " into a boolean");
	}
	public java.lang.Boolean coerce(Object obj) {
	if (obj instanceof String) {
	switch((String) obj) {
	case "true": return true;
	case "false": return false;
	default: throw new IllegalArgumentException(coercionFailureMessage(obj));
	}
	}
	if (obj instanceof java.lang.Boolean) {
	return (java.lang.Boolean) obj;
	}
	throw new IllegalArgumentException(coercionFailureMessage(obj));
	}
	private String coercionFailureMessage(Object obj) {
	return String.format("Cannot coerce `%s` to boolean (%s)", obj, this.getName());
	}

╭─{ rye@perhaps:~/src/elastic/logstash@main (validate/feature/reimplement_config_classes_in_java ✘) }
╰─● (echo 'api.enabled: bananas' > config/logstash.yml) && bin/logstash --log.level=debug -e 'input { generator { count => 1 } }'
Using system java: /Users/rye/.jenv/shims/java
ERROR: Failed to load settings file from "path.settings". Aborting... path.setting=/Users/rye/src/elastic/logstash@main/config, exception=Java::JavaLang::IllegalArgumentException, message=>Cannot coerce `bananas` to boolean (api.enabled)
[FATAL] 2024-09-10 14:20:03.107 [main] Logstash - Logstash stopped processing because of an error: (SystemExit) exit
org.jruby.exceptions.SystemExit: (SystemExit) exit
        at org.jruby.RubyKernel.exit(org/jruby/RubyKernel.java:921) ~[jruby.jar:?]
        at org.jruby.RubyKernel.exit(org/jruby/RubyKernel.java:880) ~[jruby.jar:?]
        at Users.rye.src.elastic.logstash_at_40_main.lib.bootstrap.environment.<main>(/Users/rye/src/elastic/logstash@main/lib/bootstrap/environment.rb:90) ~[?:?]
[error: 1 (00:00:05)]                                                                                                                                                                                                                                                                                                                                                                                                            

[andsel]

Thank's @yaauie for catching it. I've updated with your suggestion plus covered with a unit test.

[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, but was flaky

• Buildkite Build
• Commit: 192410e

Failed CI Steps

• :rspec: Ruby unit tests
• :rspec: Ruby unit tests
• 🥼 x-pack unit tests

History

• 💚 Build #1415 succeeded 37b8713
• 💔 Build #1413 failed 08da681
• 💔 Build #1411 failed fa03765
• 💔 Build #1410 failed 187fbc7
• 💚 Build #1338 succeeded 9d7feaa
• 💛 Build #1334 was flaky 0958719

cc @andsel

[yaauie]

LGTM 🎉

[andsel]

@logstashmachine backport 8.x