Skip to content

Commit

Permalink
fix: optional plugin dependencies not working correctly
Browse files Browse the repository at this point in the history
  • Loading branch information
guqing committed Nov 29, 2024
1 parent e5be856 commit 89d9023
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import run.halo.app.infra.utils.YamlUnstructuredLoader;
import run.halo.app.plugin.PluginConst;
import run.halo.app.plugin.PluginProperties;
import run.halo.app.plugin.PluginService;
import run.halo.app.plugin.SpringPluginManager;

/**
Expand All @@ -85,13 +86,16 @@ public class PluginReconciler implements Reconciler<Request> {

private final PluginProperties pluginProperties;

private final PluginService pluginService;

private Clock clock;

public PluginReconciler(ExtensionClient client, SpringPluginManager pluginManager,
PluginProperties pluginProperties) {
PluginProperties pluginProperties, PluginService pluginService) {
this.client = client;
this.pluginManager = pluginManager;
this.pluginProperties = pluginProperties;
this.pluginService = pluginService;
this.clock = Clock.systemUTC();
}

Expand Down Expand Up @@ -343,13 +347,12 @@ private Result enablePlugin(Plugin plugin) {
private Result disablePlugin(Plugin plugin) {
var pluginName = plugin.getMetadata().getName();
var status = plugin.getStatus();
if (pluginManager.getPlugin(pluginName) != null) {
var pluginWrapper = pluginManager.getPlugin(pluginName);
if (pluginWrapper != null) {
// check if the plugin has children
var dependents = pluginManager.getDependents(pluginName)
.stream()
.filter(pw -> PluginState.STARTED.equals(pw.getPluginState()))
.map(PluginWrapper::getPluginId)
.toList();
var dependents = pluginService.getRequiredDependencies(plugin,
pw -> PluginState.STARTED.equals(pw.getPluginState())
);
var conditions = status.getConditions();
if (!CollectionUtils.isEmpty(dependents)) {
removeConditionBy(conditions, ConditionType.READY);
Expand Down Expand Up @@ -515,15 +518,9 @@ private Result loadOrReload(Plugin plugin) {
}

// check dependencies before loading
var unresolvedParentPlugins = plugin.getSpec().getPluginDependencies().keySet()
.stream()
.filter(dependency -> {
var parentPlugin = pluginManager.getPlugin(dependency);
return parentPlugin == null
|| pluginManager.getUnresolvedPlugins().contains(parentPlugin);
})
.sorted()
.toList();
var unresolvedParentPlugins = pluginService.getRequiredDependencies(plugin,
pw -> pw == null || pluginManager.getUnresolvedPlugins().contains(pw)
);
if (!unresolvedParentPlugins.isEmpty()) {
// requeue if the parent plugin is not loaded yet.
removeConditionBy(conditions, ConditionType.INITIALIZED);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import lombok.extern.slf4j.Slf4j;
import org.pf4j.PluginRuntimeException;
import org.springframework.beans.factory.support.DefaultBeanNameGenerator;
import org.springframework.beans.factory.support.DefaultListableBeanFactory;
import org.springframework.boot.env.PropertySourceLoader;
import org.springframework.boot.env.YamlPropertySourceLoader;
import org.springframework.context.ApplicationContext;
Expand Down Expand Up @@ -65,13 +66,19 @@ public ApplicationContext create(String pluginId) {

var sw = new StopWatch("CreateApplicationContextFor" + pluginId);
sw.start("Create");
var context = new PluginApplicationContext(pluginId, pluginManager);

var pluginWrapper = pluginManager.getPlugin(pluginId);
var classLoader = pluginWrapper.getPluginClassLoader();

// create a custom bean factory to set the class loader
var beanFactory = new DefaultListableBeanFactory();
beanFactory.setBeanClassLoader(classLoader);

var context = new PluginApplicationContext(pluginId, pluginManager, beanFactory);
context.setBeanNameGenerator(DefaultBeanNameGenerator.INSTANCE);
context.registerShutdownHook();
context.setParent(pluginManager.getSharedContext());

var pluginWrapper = pluginManager.getPlugin(pluginId);
var classLoader = pluginWrapper.getPluginClassLoader();
var resourceLoader = new DefaultResourceLoader(classLoader);
context.setResourceLoader(resourceLoader);
sw.stop();
Expand All @@ -84,7 +91,6 @@ public ApplicationContext create(String pluginId) {
sw.stop();

sw.start("RegisterBeans");
var beanFactory = context.getBeanFactory();
beanFactory.registerSingleton("pluginWrapper", pluginWrapper);
context.registerBean(AggregatedRouterFunction.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.List;
import java.util.concurrent.locks.StampedLock;
import org.springframework.beans.factory.support.DefaultListableBeanFactory;
import org.springframework.context.ApplicationEvent;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.core.ResolvableType;
Expand All @@ -27,7 +28,9 @@ public class PluginApplicationContext extends AnnotationConfigApplicationContext

private final SpringPluginManager pluginManager;

public PluginApplicationContext(String pluginId, SpringPluginManager pluginManager) {
public PluginApplicationContext(String pluginId, SpringPluginManager pluginManager,
DefaultListableBeanFactory beanFactory) {
super(beanFactory);
this.pluginId = pluginId;
this.pluginManager = pluginManager;
}
Expand Down
13 changes: 13 additions & 0 deletions application/src/main/java/run/halo/app/plugin/PluginService.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package run.halo.app.plugin;

import java.nio.file.Path;
import java.util.List;
import java.util.function.Predicate;
import org.pf4j.PluginWrapper;
import org.springframework.core.io.Resource;
import org.springframework.core.io.buffer.DataBuffer;
import org.springframework.web.server.ServerWebInputException;
Expand Down Expand Up @@ -102,4 +105,14 @@ public interface PluginService {
*/
Mono<Plugin> changeState(String pluginName, boolean requestToEnable, boolean wait);

/**
* Gets required dependencies of the given plugin.
*
* @param plugin the plugin to get dependencies from
* {@link Plugin.PluginSpec#getPluginDependencies()}
* @param predicate the predicate to filter by {@link PluginWrapper},such as enabled or disabled
* @return plugin names of required dependencies
*/
List<String> getRequiredDependencies(Plugin plugin,
Predicate<PluginWrapper> predicate);
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@
import java.util.List;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.pf4j.DependencyResolver;
import org.pf4j.PluginDependency;
import org.pf4j.PluginDescriptor;
import org.pf4j.PluginWrapper;
import org.reactivestreams.Publisher;
Expand Down Expand Up @@ -269,11 +271,11 @@ public Flux<DataBuffer> uglifyJsBundle() {
.sort(Comparator.comparing(PluginWrapper::getPluginId))
.flatMapSequential(pluginWrapper -> {
var pluginId = pluginWrapper.getPluginId();
return Mono.<Resource>fromSupplier(
() -> BundleResourceUtils.getJsBundleResource(
return Mono.fromSupplier(() -> BundleResourceUtils.getJsBundleResource(
pluginManager, pluginId, BundleResourceUtils.JS_BUNDLE
)
)
.cast(Resource.class)
.filter(Resource::isReadable)
.flatMapMany(resource -> {
var head = Mono.<DataBuffer>fromSupplier(
Expand All @@ -297,9 +299,10 @@ public Flux<DataBuffer> uglifyCssBundle() {
.flatMapSequential(pluginWrapper -> {
var pluginId = pluginWrapper.getPluginId();
var dataBufferFactory = DefaultDataBufferFactory.sharedInstance;
return Mono.<Resource>fromSupplier(() -> BundleResourceUtils.getJsBundleResource(
return Mono.fromSupplier(() -> BundleResourceUtils.getJsBundleResource(
pluginManager, pluginId, BundleResourceUtils.CSS_BUNDLE
))
.cast(Resource.class)
.filter(Resource::isReadable)
.flatMapMany(resource -> {
var head = Mono.<DataBuffer>fromSupplier(() -> dataBufferFactory.wrap(
Expand Down Expand Up @@ -344,28 +347,19 @@ public Mono<Plugin> changeState(String pluginName, boolean requestToEnable, bool
// preflight check
if (requestToEnable) {
// make sure the dependencies are enabled
var dependencies = plugin.getSpec().getPluginDependencies().keySet();
var notStartedDependencies = dependencies.stream()
.filter(dependency -> {
var pluginWrapper = pluginManager.getPlugin(dependency);
return pluginWrapper == null
|| !Objects.equals(STARTED, pluginWrapper.getPluginState());
})
.toList();
var notStartedDependencies = getRequiredDependencies(plugin,
pw -> pw == null || !Objects.equals(STARTED, pw.getPluginState())
);
if (!CollectionUtils.isEmpty(notStartedDependencies)) {
return Mono.error(
new PluginDependenciesNotEnabledException(notStartedDependencies)
);
}
} else {
// make sure the dependents are disabled
var dependents = pluginManager.getDependents(pluginName);
var notDisabledDependents = dependents.stream()
.filter(
dependent -> Objects.equals(STARTED, dependent.getPluginState())
)
.map(PluginWrapper::getPluginId)
.toList();
var notDisabledDependents = getRequiredDependencies(plugin,
pw -> pw != null && Objects.equals(STARTED, pw.getPluginState())
);
if (!CollectionUtils.isEmpty(notDisabledDependents)) {
return Mono.error(
new PluginDependentsNotDisabledException(notDisabledDependents)
Expand Down Expand Up @@ -416,6 +410,27 @@ public Mono<Plugin> changeState(String pluginName, boolean requestToEnable, bool
return updatedPlugin;
}

@Override
public List<String> getRequiredDependencies(Plugin plugin,
Predicate<PluginWrapper> predicate) {
return getPluginDependency(plugin)
.stream()
.filter(dependency -> !dependency.isOptional())
.map(PluginDependency::getPluginId)
.filter(dependencyPlugin -> {
var pluginWrapper = pluginManager.getPlugin(dependencyPlugin);
return predicate.test(pluginWrapper);
})
.toList();
}

private static List<PluginDependency> getPluginDependency(Plugin plugin) {
return plugin.getSpec().getPluginDependencies().keySet()
.stream()
.map(PluginDependency::new)
.toList();
}

Mono<Plugin> findPluginManifest(Path path) {
return Mono.fromSupplier(
() -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@

import static run.halo.app.extension.index.query.QueryFactory.equal;

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.pf4j.ExtensionPoint;
import org.pf4j.PluginManager;
import org.pf4j.PluginWrapper;
import org.springframework.beans.factory.BeanFactory;
import org.springframework.core.annotation.AnnotationAwareOrderComparator;
import org.springframework.data.domain.Sort;
import org.springframework.lang.NonNull;
import org.springframework.stereotype.Component;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
Expand All @@ -22,7 +25,9 @@
import run.halo.app.extension.router.selector.FieldSelector;
import run.halo.app.infra.SystemConfigurableEnvironmentFetcher;
import run.halo.app.infra.SystemSetting.ExtensionPointEnabled;
import run.halo.app.plugin.SpringPlugin;

@Slf4j
@Component
@RequiredArgsConstructor
public class DefaultExtensionGetter implements ExtensionGetter {
Expand All @@ -37,7 +42,7 @@ public class DefaultExtensionGetter implements ExtensionGetter {

@Override
public <T extends ExtensionPoint> Flux<T> getExtensions(Class<T> extensionPoint) {
return Flux.fromIterable(pluginManager.getExtensions(extensionPoint))
return Flux.fromIterable(lookExtensions(extensionPoint))
.concatWith(
Flux.fromStream(() -> beanFactory.getBeanProvider(extensionPoint).orderedStream())
)
Expand All @@ -47,8 +52,7 @@ public <T extends ExtensionPoint> Flux<T> getExtensions(Class<T> extensionPoint)
@Override
public <T extends ExtensionPoint> List<T> getExtensionList(Class<T> extensionPoint) {
var extensions = new LinkedList<T>();
Optional.ofNullable(pluginManager.getExtensions(extensionPoint))
.ifPresent(extensions::addAll);
extensions.addAll(lookExtensions(extensionPoint));
extensions.addAll(beanFactory.getBeanProvider(extensionPoint).orderedStream().toList());
extensions.sort(new AnnotationAwareOrderComparator());
return extensions;
Expand Down Expand Up @@ -112,4 +116,24 @@ private Mono<ExtensionPointDefinition> fetchExtensionPointDefinition(
.flatMap(list -> Mono.justOrEmpty(ListResult.first(list)));
}

@NonNull
protected <T> List<T> lookExtensions(Class<T> type) {
List<T> beans = new ArrayList<>();
for (PluginWrapper startedPlugin : pluginManager.getStartedPlugins()) {
if (startedPlugin.getPlugin() instanceof SpringPlugin springPlugin) {
var pluginApplicationContext = springPlugin.getApplicationContext();
try {
pluginApplicationContext.getBeansOfType(type)
.forEach((name, bean) -> beans.add(bean));
} catch (Throwable e) {
// Ignore
log.error("Error while looking for extensions of type {}", type, e);
}
} else {
var extensions = pluginManager.getExtensions(type, startedPlugin.getPluginId());
beans.addAll(extensions);
}
}
return beans;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import run.halo.app.core.extension.Plugin;
import run.halo.app.core.extension.ReverseProxy;
import run.halo.app.core.extension.Setting;
import run.halo.app.core.reconciler.PluginReconciler;
import run.halo.app.extension.ConfigMap;
import run.halo.app.extension.ExtensionClient;
import run.halo.app.extension.Metadata;
Expand All @@ -58,6 +57,7 @@
import run.halo.app.infra.Condition;
import run.halo.app.infra.ConditionStatus;
import run.halo.app.plugin.PluginProperties;
import run.halo.app.plugin.PluginService;
import run.halo.app.plugin.SpringPluginManager;

/**
Expand All @@ -78,6 +78,9 @@ class PluginReconcilerTest {
@Mock
PluginProperties pluginProperties;

@Mock
private PluginService pluginService;

@InjectMocks
PluginReconciler reconciler;

Expand Down Expand Up @@ -111,6 +114,12 @@ class WhenNotDeleting {
@TempDir
Path tempPath;

@BeforeEach
void setUp() {
lenient().when(pluginService.getRequiredDependencies(any(), any()))
.thenReturn(List.of());
}

@Test
void shouldNotStartPluginWithDevModeInNonDevEnv() {
var fakePlugin = createPlugin(name, plugin -> {
Expand Down
Loading

0 comments on commit 89d9023

Please sign in to comment.