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

Add generic interface for loading service providers from plugins #88082

Merged
merged 15 commits into from
Jun 30, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
Expand Down Expand Up @@ -279,7 +281,6 @@ private Map<String, LoadedPlugin> loadBundles(Set<PluginBundle> bundles) {

// package-private for test visibility
static void loadExtensions(Collection<LoadedPlugin> plugins) {

Map<String, List<Plugin>> extendingPluginsByName = plugins.stream()
.flatMap(t -> t.descriptor().getExtendedPlugins().stream().map(extendedPlugin -> Tuple.tuple(extendedPlugin, t.instance())))
.collect(Collectors.groupingBy(Tuple::v1, Collectors.mapping(Tuple::v2, Collectors.toList())));
Expand All @@ -293,6 +294,33 @@ static void loadExtensions(Collection<LoadedPlugin> plugins) {
}
}

// package private for testing
<T> Iterator<T> providersIterator(Class<T> service, ClassLoader classLoader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be folded into loadServiceProviders now that the tests don't mock it?

return ServiceLoader.load(service, classLoader).iterator();
}

/**
* SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers
* from plugins/modules.
*
* <pre>
* For example:
*
* var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a <pre> block?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think only the code should be in the <pre>?

* </pre>
* @param service A templated service class to look for providers in plugins
* @return an immutable {@link List} of discovered providers in the plugins/modules
*/
public <T> List<? extends T> loadServiceProviders(Class<T> service) {
List<T> result = new ArrayList<>();

for (LoadedPlugin pluginTuple : plugins()) {
providersIterator(service, pluginTuple.loader()).forEachRemaining(c -> result.add(c));
}

return Collections.unmodifiableList(result);
}

private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, List<Plugin> extendingPlugins) {
ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,6 @@ grant codeBase "${codebase.netty-transport}" {
grant {
permission java.net.SocketPermission "127.0.0.1", "connect, resolve";
permission java.nio.file.LinkPermission "symbolic";
// needed for PluginsServiceTests
permission java.lang.RuntimePermission "createClassLoader";
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,14 @@
import org.apache.lucene.util.Constants;
import org.elasticsearch.Version;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.core.Strings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.operator.OperatorHandler;
import org.elasticsearch.operator.OperatorHandlerProvider;
import org.elasticsearch.operator.TransformState;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;
Expand All @@ -27,6 +32,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Set;
Expand All @@ -38,6 +44,9 @@
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.sameInstance;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;

@LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
public class PluginsServiceTests extends ESTestCase {
Expand Down Expand Up @@ -613,6 +622,66 @@ public void testThrowingConstructor() {
assertThat(e.getCause().getCause(), hasToString(containsString("test constructor failure")));
}

public void testLoadServiceProviders() throws ClassNotFoundException {
FakeClassLoader fakeClassLoader = new FakeClassLoader();
@SuppressWarnings("unchecked")
Class<? extends Plugin> fakePluginClass = (Class<? extends Plugin>) fakeClassLoader.findClass(FakePlugin.class.getName());

FakeClassLoader fakeClassLoader1 = new FakeClassLoader();
@SuppressWarnings("unchecked")
Class<? extends Plugin> fakePluginClass1 = (Class<? extends Plugin>) fakeClassLoader1.findClass(FakePlugin.class.getName());

assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader()));

OperatorHandlerProvider provider1 = () -> List.of(new OperatorHandler<Integer>() {
@Override
public String name() {
return "integer";
}

@Override
public TransformState transform(Object source, TransformState prevState) {
return prevState;
}
});

OperatorHandlerProvider provider2 = () -> List.of(new OperatorHandler<String>() {
@Override
public String name() {
return "string";
}

@Override
public TransformState transform(Object source, TransformState prevState) {
return prevState;
}
});
PluginsService service = spy(newMockPluginsService(List.of(fakePluginClass, fakePluginClass1)));

doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are dealing with different classloaders, we should probably test more real-world situations. Can the test use real classloaders instead of mocking them? We have utilities for creating dummy jars with module infos, classes, etc. It seems doable to actually test the service loading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought back the real classloader test version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see mocking? To clarify what I was suggesting: dynamically build a dummy plugin jar and zip (with the utilities I mentioned) so the providersIterator method can actually be tested with the real ServiceLoader

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, okay, I'll go further in building dummy plugins. This way I think I probably won't need to add extra permissions for the test framework.

doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));

List<? extends OperatorHandlerProvider> providers = service.loadServiceProviders(OperatorHandlerProvider.class);
assertEquals(2, providers.size());
List<String> handlers = providers.stream().map(p -> p.handlers()).flatMap(l -> l.stream()).map(h -> h.name()).toList();

assertThat(handlers, containsInAnyOrder("string", "integer"));

doReturn(Collections.emptyList().iterator()).when(service)
.providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));

assertEquals(1, service.loadServiceProviders(OperatorHandlerProvider.class).size());

doReturn(Collections.emptyList().iterator()).when(service)
.providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
doReturn(Collections.emptyList().iterator()).when(service)
.providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));

assertEquals(0, service.loadServiceProviders(OperatorHandlerProvider.class).size());

}

private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin {
private List<TestExtensionPoint> extensions;

Expand Down Expand Up @@ -652,4 +721,23 @@ public ThrowingConstructorExtension() {
throw new IllegalArgumentException("test constructor failure");
}
}

static class FakeClassLoader extends ClassLoader {
@Override
public Class<?> findClass(String name) throws ClassNotFoundException {
byte[] classBytes = fromFile(getClass().getClassLoader(), name);
return defineClass(name, classBytes, 0, classBytes.length);
}

private static byte[] fromFile(ClassLoader realLoader, String fileName) {
try {
InputStream input = realLoader.getResourceAsStream(
Strings.format("%s.class", fileName.replace(".", PathUtils.get(".").getFileSystem().getSeparator()))
);
return input.readAllBytes();
} catch (Exception x) {
throw new IllegalStateException("Error loading class", x);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public MockPluginsService(Settings settings, Environment environment, Collection
if (logger.isTraceEnabled()) {
logger.trace("plugin loaded from classpath [{}]", pluginInfo);
}
pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin));
pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin, pluginClass.getClassLoader(), ModuleLayer.boot()));
}

this.classpathPlugins = List.copyOf(pluginsLoaded);
Expand Down