logstash icon indicating copy to clipboard operation
logstash copied to clipboard

Avoid SuppressWarnings of "this-escape" in Java Setting hierarchy

Open andsel opened this issue 1 year ago • 3 comments

With #16496 and commit 5ca068693a6cbec28bf794cb1cf6f560c5053351 in #16490 were introduced some @SupressWarning annotations to avoid errors with JDK 21 like:

/buildkite/builds/bk-agent-prod-k8s-1728654414425952824/elastic/logstash-pull-request-pipeline/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java:105: warning: [this-escape] possible 'this' escape before subclass is fully initialized
--
  | validate(defaultValue);
  | ^pileJava
  | error: warnings found and -Werror specified
  | 1 error
  | 1 warning
  |  
  | > Task :logstash-core:compileJava FAILED
  |  
  | FAILURE: Build failed with an exception.


As per comment https://github.com/elastic/logstash/pull/16490#pullrequestreview-2360999039 this issue is to ask for removal of such annotations and leverage static method instead of calling instance methods on non-completely initialised instances.

andsel avatar Oct 11 '24 13:10 andsel

Shaped the rough idea in #16543 but still coerce method is harder to avoid https://github.com/elastic/logstash/blob/6064587bc48d2e542cf7be7022f4e7cbcd661afa/logstash-core/src/main/java/org/logstash/settings/Coercible.java#L31

andsel avatar Oct 11 '24 14:10 andsel

Hey @andsel @robbavey, dusted off this JDK 21 nit while auditing build warnings—smart call to ditch those @SuppressWarnings for cleaner hierarchy init. Skimmed BaseSetting.java and the Coercible interface; the this-escape pops because we're invoking instance validate/coerce pre-super() full init, but yeah, coercing defaults in a static wrapper feels like the play. Quick root & hurdles:

  • Core issue: Subclass constructors call super(defaultValue) → triggers validate(defaultValue) on half-baked this. Static refactor sidesteps it without losing type safety.
  • Sticky bit in #16543: Coercible.coerce(defaultValue)—it's instance-bound, but we could hoist to a static util (e.g., Coercible.coerceStatic(Class<T> clazz, Object raw)) with reflection or generics magic, or make coerce a static default method in the interface (Java 8+ friendly).

Fix sketch if picking up:

  • Refactor BaseSetting ctor to static factory: static <T> Setting<T> create(...) { validateStatic(raw); return new Subclass(...); }.
  • For Coercible: Add static overload, migrate callers (e.g., in StringSetting), add specs for init flows.
  • Bonus: Bump min JDK in CI to 21, gate warnings-as-errors.

#16543's a great skeleton—any thoughts on the coerce refactor? Happy to revive it with a branch/tests. Can you assign this to me?

0xShubhamSolanki avatar Oct 27 '25 19:10 0xShubhamSolanki

Hi @0xShubhamSolanki thanks a lot to chime in!

If you would you can provide a new PR with the solution you are proposing, so that we can think about it, summarizing in the PR which are the changes you are proposing.

My idea was to completely migrate the settings hierarchy from Ruby to Java and then fix this issue, to have full context, but I don't expect any significant changes in the super - sub constructors call chains to pop in. So if you mind to shape a solution for this I would me more than happy to review it!

andsel avatar Dec 03 '25 09:12 andsel