Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Re-)Add coalesceToEmpty method to o.o.common.Strings util class #5664

Closed
dbwiddis opened this issue Dec 30, 2022 · 2 comments
Closed

(Re-)Add coalesceToEmpty method to o.o.common.Strings util class #5664

dbwiddis opened this issue Dec 30, 2022 · 2 comments
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request Plugins

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Dec 30, 2022

Is your feature request related to a problem? Please describe.

A relatively common use case during REST handling is to replace a null String with an empty String:

public static String coalesceToEmpty(@Nullable String s) {
    return s == null ? "" : s;
}

Such a method exists in ml-commons and anomaly-detection plugins and is being added to job-scheduler.

This exists in the current release (2.4.1) of the Strings class here and is referenced by the alerting plugin but was removed in #5288 as "unused". (This broke the AD plugin which prompted re-adding its own method.)

Describe the solution you'd like

Don't repeat code. Add this method back to the Strings class.

Additionally, I'm curious why this util class is marked "internal" as it's useful for dependent classes to use (which at least the alerting plugin does). The removal was not marked deprecated nor mentioned in the change log (likely because of its "internal" usage) but there is a lot of use of other methods in this class.

Describe alternatives you've considered

It may be a better idea to create a util class with all the RestHandlerUtils in the above-linked plugin packages.

Additional context

The method appears to have originated in com.google.common.base.Strings and was moved to the Strings class (in open-source ES pre-fork) to remove that external dependency.

If we want to remove "internal" util classes/methods like this, we should consider either using external dependencies or creating a separate supported (API) project with util classes to provide the same functionality without duplicating code.

@dbwiddis dbwiddis added enhancement Enhancement or improvement to existing feature or request untriaged labels Dec 30, 2022
@dblock
Copy link
Member

dblock commented Jan 2, 2023

I think we can all agree that coalesceToEmpty is not "OpenSearch-specific business logic", thus isn't not an API I think OpenSearch should expose. However, having helpers to make life easier for plugin authors may make sense if we wanted to continue building tightly-coupled plugins. Introducing RestHandlerUtils would be one way to do it. Which tells me that the right place for this method was indeed in com.google.common.base.Strings, a helper library whose only purpose in life was to do things about strings, aka coalesceToEmpty was its business logic. We just didn't want the extra dependency for such a silly method.

If we go all in with helpers, the risk is that RestHandlerUtils grows out of control and becomes common-utils, a kitchen sink of unrelated functions.

All that to say that I think putting this method back into this project and unmarking it from internal is harmless, but that I hope a plugin an extension author will not care whether we do or don't.

@dbwiddis
Copy link
Member Author

dbwiddis commented Jan 9, 2023

Closing this request as I have decided it's the wrong answer to a real problem.

I'll consider a future request for an API RestHandlerUtils class.

@dbwiddis dbwiddis closed this as completed Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request Plugins
Projects
None yet
Development

No branches or pull requests

4 participants