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

Replace "blacklist/whitelist" terminology in Java APIs #1683

Closed
tlfeng opened this issue Dec 9, 2021 · 4 comments · Fixed by #3350
Closed

Replace "blacklist/whitelist" terminology in Java APIs #1683

tlfeng opened this issue Dec 9, 2021 · 4 comments · Fixed by #3350
Assignees
Labels
>breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request v3.0.0 Issues and PRs related to version 3.0.0

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Dec 9, 2021

Is your feature request related to a problem? Please describe.
Replace the "blacklist/whitelist" terminology in all Java APIs, including field, method, class, and package names.
The "Java APIs" refers to those packaged in Java libraries and are published to Maven (https://search.maven.org/search?q=g:org.opensearch https://mvnrepository.com/artifact/org.opensearch)

Impact:
All plugins, clients and tools that use OpenSearch Java APIs from OpenSearch Java libraries which contain non-inclusive terminologies have to make corresponding changes to call new APIs, if they want to upgrade the dependency to a future major version of OpenSearch.

A part of #1483

Describe the solution you'd like
Replace the "blacklist/whitelist" terminology with "denylist/allowlist" in all Java APIs that are exposed in Java libraries.

Describe alternatives you've considered
None.

Additional context
Locations of "blacklist/whitelist" in API of Java library:
1 Painless SPI - multiple classes with name “Whitelist”
Code: https://github.com/opensearch-project/OpenSearch/tree/1.0.0/modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi
Javadoc: https://opensearch.org/javadocs/1.0.0/OpenSearch/modules/lang-painless/spi/build/docs/javadoc/org/opensearch/painless/spi/package-summary.html

2 Subclasses of the Painless SPI classes: ProcessorsWhitelistExtension
Code: https://github.com/opensearch-project/OpenSearch/blob/2.0.0/modules/ingest-common/src/main/java/org/opensearch/ingest/common/ProcessorsWhitelistExtension.java

The regex to filter the lines with public or protected class definition contains "whitelist" terminology:
(public|protected)(.)+(class|interface)\s(\w)*Whitelist

List of the classes need renaming:
In package org.opensearch.painless.spi

final class Whitelist
final class WhitelistClass
class WhitelistClassBinding
final class WhitelistConstructor
class WhitelistField
class WhitelistInstanceBinding
final class WhitelistLoader
class WhitelistMethod
interface WhitelistAnnotationParser

In package org.opensearch.ingest.common:

class ProcessorsWhitelistExtension

LIst of the public method and variables need renaming:
In final class Whitelist:

public static final List<Whitelist> BASE_WHITELISTS
public final List<WhitelistClass> whitelistClasses
public final List<WhitelistMethod> whitelistImportedMethods
public final List<WhitelistClassBinding> whitelistClassBindings
public final List<WhitelistInstanceBinding> whitelistInstanceBindings

In final class WhitelistClass:

public final List<WhitelistConstructor> whitelistConstructors
public final List<WhitelistMethod> whitelistMethods
public final List<WhitelistField> whitelistFields

In interface PainlessExtension:

Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists();

There are some methods whose return type are objects of classes contain Whitelist, so the backwards compatibility needs to be considered:
In final class WhitelistLoader:

public static Whitelist loadFromResourceFiles(Class<?> resource, String... filepaths)
public static Whitelist loadFromResourceFiles(Class<?> resource, Map<String, WhitelistAnnotationParser> parsers, String... filepaths)

In interface WhitelistAnnotationParser:

Map<String, WhitelistAnnotationParser> BASE_ANNOTATION_PARSERS
@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request untriaged >breaking Identifies a breaking change. and removed untriaged labels Dec 9, 2021
@tlfeng tlfeng added the v3.0.0 Issues and PRs related to version 3.0.0 label May 17, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 22, 2022

I decide to add back the old usages of the public classes or interfaces that contains whitelist in the name, and deprecate them before directly replacing them, while keeping the new usages with allowlist name that made by PR #3350.
So that we can have a smoother path to let developers migrate to use classes with allowlist in the name.

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 3, 2022

There is one usage of the impacted classes or methods in opensearch-project organization:
https://github.com/opensearch-project/alerting/blob/2.1.0.0/alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt#L115
which overriding the method Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists()

@andrross
Copy link
Member

andrross commented Aug 3, 2022

There is one usage of the impacted classes or methods in opensearch-project organization

This appears to be used by the k-NN plugin as well

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 9, 2022

After an incomplete attempt to provide alternative allowlist APIs as well as keeping the existing whitelist APIs in PR #4101, I decided not to deprecate the whitelist APIs before replacing them with allowlist APIs in a future major version.
We will post a plan for the version of removing whitelist APIs. Developers can keep using the existing whitelist APIs till the further notice.

The impact of the plugin developers is likely be minor, if making as breaking change instead of deprecated in advance.
There are 2 plugins (alerting and k-NN) in opensearch-project organization using the whitelist Java APIs, and the affected APIs of them are:

 public Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists()
 -> public Map<ScriptContext<?>, List<Allowlist>> getContextAllowlists()
 
org.opensearch.painless.spi.Whitelist -> Allowlist
org.opensearch.painless.spi.WhitelistLoader -> AllowlistLoader

The detailed reason not to deprecate whitelist APIs in 2.x version:
There is a public abstract method that can be implemented by plugins.

Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists();

I found it's hard to support the return type Map<ScriptContext<?>, List<Whitelist>>, while provide alternative of Map<ScriptContext<?>, List<Allowlist>>, because there will be type casting problem in a few APIs that also use the same return type.

For example, the return type Map<ScriptContext<?>, List<Whitelist>> can be used by a native module PainlessPlugin (or lang-painless), such as in a constructor of a class:

public PainlessScriptEngine(Settings settings, Map<ScriptContext<?>, List<Whitelist>> contexts) {

I have tried to make Whitelist as a subclass of Allowlist class, and provide alternative method to return Map<ScriptContext<?>, List<Allowlist>> in commit tlfeng@539d78d, like below:

/** @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #getContextAllowlists()} */
@Deprecated
default Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists() {
  throw new UnsupportedOperationException("Must be overridden");
}

// TODO: Remove the default implementation after removing the deprecated getContextWhitelists()
default Map<ScriptContext<?>, List<Allowlist>> getContextAllowlists() {
return getContextWhitelists().entrySet()
  stream()
  .collect(Collectors.toMap(k -> k.getKey(), v -> v.getValue().stream().map(e -> (Allowlist) e).collect(Collectors.toList())));
}

Java doesn't support me to provide 2 constructors to receive either Map<ScriptContext<?>, List<Whitelist>> or Map<ScriptContext<?>, List<Allowlist>>.
I don't want to change the parameter type to a wider bound class, because that will cause more problem for the implementation inside the constructor.

With the above problems, it's hard to keep the backwards compatibility, while providing alternative new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants