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.

Sign up for GitHub

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

Jump to bottom

feat: improve DependencyGraph and error reporting #4628

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: improve DependencyGraph and error reporting #4628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 126 additions & 37 deletions core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package org.eclipse.edc.boot.system;

import org.eclipse.edc.boot.system.injection.EdcInjectionException;
import org.eclipse.edc.boot.system.injection.InjectionContainer;
import org.eclipse.edc.boot.system.injection.InjectionPoint;
import org.eclipse.edc.boot.system.injection.InjectionPointScanner;
Expand All @@ -25,9 +24,9 @@
import org.eclipse.edc.boot.util.TopologicalSort;
import org.eclipse.edc.runtime.metamodel.annotation.Provides;
import org.eclipse.edc.runtime.metamodel.annotation.Requires;
import org.eclipse.edc.spi.EdcException;
import org.eclipse.edc.spi.system.ServiceExtension;
import org.eclipse.edc.spi.system.ServiceExtensionContext;
import org.jetbrains.annotations.Nullable;

import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -37,7 +36,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.function.Function;
import java.util.stream.Stream;

import static java.util.stream.Collectors.toList;
Expand All @@ -48,25 +47,42 @@
* which extension depends on which other extension.
*/
public class DependencyGraph {
private final InjectionPointScanner injectionPointScanner = new InjectionPointScanner();
private final ServiceExtensionContext context;

public DependencyGraph(ServiceExtensionContext context) {
this.context = context;
private final List<InjectionContainer<ServiceExtension>> injectionContainers;
/**
* contains all missing dependencies that were expressed as injection points
*/
private final HashMap<Class<? extends ServiceExtension>, List<InjectionFailure>> unsatisfiedInjectionPoints;
/**
* contains all missing dependencies that were expressed as @Require(...) annotations on the extension class
*/
private final ArrayList<Class<?>> unsatisfiedRequirements;

private DependencyGraph(List<InjectionContainer<ServiceExtension>> injectionContainers, HashMap<Class<? extends ServiceExtension>, List<InjectionFailure>> unsatisfiedInjectionPoints, ArrayList<Class<?>> unsatisfiedRequirements) {

this.injectionContainers = injectionContainers;
this.unsatisfiedInjectionPoints = unsatisfiedInjectionPoints;
this.unsatisfiedRequirements = unsatisfiedRequirements;
}

/**
* Sorts all {@link ServiceExtension} implementors, that were found on the classpath, according to their dependencies.
* Depending Extensions (i.e. those who <em>express</em> a dependency) are sorted first, providing extensions (i.e. those
* who provide a dependency) are sorted last.
* Builds the DependencyGraph by evaluating all {@link ServiceExtension} implementors, that were found on the classpath,
* and sorting them topologically according to their dependencies.
* <em>Dependent</em> extensions (i.e. those who <em>express</em> a dependency) are sorted first, providing extensions (i.e. those
* who <em>provide</em> a dependency) are sorted last.
* <p>
* This factory method does not throw any exception except a {@link CyclicDependencyException}, please check {@link DependencyGraph#isValid()} if the graph is valid.
*
* @param context An instance of the (fully-initialized) {@link ServiceExtensionContext} which is used to resolve services and configuration.
* @param extensions A list of {@link ServiceExtension} instances that were picked up by the {@link ServiceLocator}
* @return A list of {@link InjectionContainer}s that are sorted topologically according to their dependencies.
* @throws CyclicDependencyException when there is a dependency cycle
* @see TopologicalSort
* @see InjectionContainer
*/
public List<InjectionContainer<ServiceExtension>> of(List<ServiceExtension> extensions) {
public static DependencyGraph of(ServiceExtensionContext context, List<ServiceExtension> extensions) {
var injectionPointScanner = new InjectionPointScanner();

Map<Class<?>, ServiceProvider> defaultServiceProviders = new HashMap<>();
Map<Class<?>, List<InjectionContainer<ServiceExtension>>> dependencyMap = new HashMap<>();
var injectionContainers = extensions.stream()
Expand Down Expand Up @@ -94,30 +110,31 @@ public List<InjectionContainer<ServiceExtension>> of(List<ServiceExtension> exte

// check if all injected fields are satisfied, collect missing ones and throw exception otherwise
var unsatisfiedInjectionPoints = new HashMap<Class<? extends ServiceExtension>, List<InjectionFailure>>();
var unsatisfiedRequirements = new ArrayList<String>();
var unsatisfiedRequirements = new ArrayList<Class<?>>();

injectionContainers.forEach(container -> {
//check that all the @Required features are there
getRequiredFeatures(container.getInjectionTarget().getClass()).forEach(serviceClass -> {
var dependencies = dependencyMap.get(serviceClass);
if (dependencies == null) {
unsatisfiedRequirements.add(serviceClass.getName());
unsatisfiedRequirements.add(serviceClass);
} else {
dependencies.forEach(dependency -> sort.addDependency(container, dependency));
}
});

injectionPointScanner.getInjectionPoints(container.getInjectionTarget())
.peek(injectionPoint -> {
var providersResult = injectionPoint.getProviders(dependencyMap, context);
if (providersResult.succeeded()) {
List<InjectionContainer<ServiceExtension>> providers = providersResult.getContent();
providers.stream().filter(d -> !Objects.equals(d, container)).forEach(provider -> sort.addDependency(container, provider));
} else {
if (injectionPoint.isRequired()) {
unsatisfiedInjectionPoints.computeIfAbsent(injectionPoint.getTargetInstance().getClass(), s -> new ArrayList<>()).add(new InjectionFailure(injectionPoint, providersResult.getFailureDetail()));
}
}
injectionPoint.getProviders(dependencyMap, context)
.onSuccess(providers -> providers.stream()
.filter(d -> !Objects.equals(d, container))
.forEach(provider -> sort.addDependency(container, provider)))
.onFailure(f -> {
if (injectionPoint.isRequired()) {
unsatisfiedInjectionPoints.computeIfAbsent(injectionPoint.getTargetInstance().getClass(), s -> new ArrayList<>())
.add(new InjectionFailure(injectionPoint.getTargetInstance(), injectionPoint, f.getFailureDetail()));
}
});

var defaultServiceProvider = defaultServiceProviders.get(injectionPoint.getType());
if (defaultServiceProvider != null) {
Expand All @@ -127,24 +144,95 @@ public List<InjectionContainer<ServiceExtension>> of(List<ServiceExtension> exte
.forEach(injectionPoint -> container.getInjectionPoints().add(injectionPoint));
});

if (!unsatisfiedInjectionPoints.isEmpty()) {
var message = "The following injected fields or values were not provided or could not be resolved:\n";
message += unsatisfiedInjectionPoints.entrySet().stream()
.map(entry -> String.format("%s is missing \n --> %s", entry.getKey(), String.join("\n --> ", entry.getValue().stream().map(Object::toString).toList()))).collect(Collectors.joining("\n"));
throw new EdcInjectionException(message);
}
sort.sort(injectionContainers);

if (!unsatisfiedRequirements.isEmpty()) {
var message = String.format("The following @Require'd features were not provided: [%s]", String.join(", ", unsatisfiedRequirements));
throw new EdcException(message);
}
return new DependencyGraph(injectionContainers, unsatisfiedInjectionPoints, unsatisfiedRequirements);
Copy link
Member

Choose a reason for hiding this comment

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

at this point the DependencyGraph looks like some sort of Result, with List<InjectionContainer> as content type and a Failure that contains the unsatisfied injection points and requirement.

Copy link
Member Author

@paullatzelsperger paullatzelsperger Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

given the choice, I would much rather pass around a DependencyGraph object than a List<InjectionPoint<ServiceExtension>>. Personally, I find it easier to read and understand and easier to handle, and it doesn't leak the InjectionPoint all over the code.
Also, it will hide the implementation better, so changes down the road would have less of an impact.

}

sort.sort(injectionContainers);
/**
* Returns all {@link InjectionPoint}s for a particular extension class. These include all types of injection points.
*
* @param serviceClass The extension class
* @return A (potentially empty) list of injection points.
*/
public List<InjectionPoint<ServiceExtension>> getDependenciesOf(Class<? extends ServiceExtension> serviceClass) {
Copy link
Member

Choose a reason for hiding this comment

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

this method is only used by tests

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the idea was to make the DG into more like a graph, with methods to match. It would make the DG more usable, esp. in runtimes other than the BaseRuntime

Copy link
Member

Choose a reason for hiding this comment

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

I'm not in favor of that, this dependency graph is really something related to the BaseRuntime implementation, if someone wants for any reason to customize it, they would need to provide their own logic, otherwise we'll be responsible to maintain logic that we don't really need.

Copy link
Member Author

@paullatzelsperger paullatzelsperger Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had a version where the DependencyGraph is an interface, and one implementation is using EDC-internal dependency injection. Other implementations could use other DI frameworks like Guice, which would make the DI independent from the runtime. I ultimately abandoned that, because it would have meant pulling the InjectionContainer and InjectionPoint classes into an SPI module, which seemed too much work and we don't really have a need for that now.

That said, these methods aren't needed by us at the moment, but since the BaseRuntime is extensible, and the DependencyGraph is exposed in protected methods, overriders may benefit from them.

return injectionContainers.stream().filter(ic -> ic.getInjectionTarget().getClass().equals(serviceClass))
.flatMap(ic -> ic.getInjectionPoints().stream())
.toList();
}

/**
* Obtains all extension classes that declare a dependency on an object of the given point. For example, declaring a
* field {@code @Inject FooService fooService} in an extension would constitute such a dependency, and in that case
* {@code FooService.class} has to be passed into this method.
* <p>
* This can also be invoked if the dependency graph is invalid, i.e. if dependency injection is not possible.
*
* @param dependencyType The type of the injection point, for example the type of service that is injected.
* @return a list of extension classes that declare a dependency onto the given injection point
*/
public List<Class<? extends ServiceExtension>> getDependentExtensions(Class<?> dependencyType) {
Copy link
Member

Choose a reason for hiding this comment

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

same


return injectionContainers.stream()
.filter(ic -> ic.getInjectionPoints().stream().anyMatch(ip -> ip.getType().equals(dependencyType)))
.map(InjectionContainer::getInjectionTarget)
.map((Function<ServiceExtension, Class<? extends ServiceExtension>>) ServiceExtension::getClass) // yes, it's nasty, but keeps the compiler happy
.toList();
}

/**
* Obtains a list of injection points, where the injection target is of the given type. For example, passing in {@code FooService.class}
* would return a list that contains injection points across all {@link ServiceExtension}s that declare a {@code @Inject FooService fooService} field.
* This is similar to {@link DependencyGraph#getDependenciesOf(Class)}, but the result values are {@link InjectionPoint}s rather than extension classes.
*
* @param dependencyType The type of the injection point, for example the type of service that is injected.
* @return a list of {@link InjectionPoint} instances that represent the concrete dependency
*/
public List<InjectionPoint<ServiceExtension>> getDependenciesFor(Class<?> dependencyType) {
Copy link
Member

Choose a reason for hiding this comment

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

same

return injectionContainers.stream()
.flatMap(ic -> ic.getInjectionPoints().stream().filter(ip -> ip.getType().equals(dependencyType)))
.toList();
}

public List<InjectionContainer<ServiceExtension>> getInjectionContainers() {
return injectionContainers;
}

private Stream<Class<?>> getRequiredFeatures(Class<?> clazz) {
/**
* Returns a list of extension instances that were found on the classpath
*/
public List<ServiceExtension> getExtensions() {
return injectionContainers.stream().map(InjectionContainer::getInjectionTarget).toList();
}

/**
* Checks if the current dependency graph is valid, i.e. there are no cycles in it and all required injection
* dependencies are satisfied.
*
* @return true if the dependency graph is valid, and the DI container can be built, false otherwise.
*/
public boolean isValid() {
return unsatisfiedInjectionPoints.isEmpty() && unsatisfiedRequirements.isEmpty();
}

/**
* Returns a list of strings, each containing information about a missing dependency
*
* @return A list of errors describing one missing dependency each
*/
public List<String> getProblems() {
var messages = unsatisfiedInjectionPoints.entrySet().stream()
.map(entry -> {
var dependent = entry.getKey();
var dependencies = entry.getValue();
var missingDependencyList = dependencies.stream().map(injectionFailure -> " --> " + injectionFailure.failureDetail()).toList();
return "## %s is missing\n%s".formatted(dependent, String.join("\n", missingDependencyList));
});

return messages.toList();
}

private static Stream<Class<?>> getRequiredFeatures(Class<?> clazz) {
var requiresAnnotation = clazz.getAnnotation(Requires.class);
if (requiresAnnotation != null) {
var features = requiresAnnotation.value();
Expand All @@ -156,7 +244,7 @@ private Stream<Class<?>> getRequiredFeatures(Class<?> clazz) {
/**
* Obtains all features a specific extension requires as strings
*/
private Set<Class<?>> getProvidedFeatures(ServiceExtension ext) {
private static Set<Class<?>> getProvidedFeatures(ServiceExtension ext) {
var allProvides = new HashSet<Class<?>>();

// check all @Provides
Expand All @@ -168,10 +256,11 @@ private Set<Class<?>> getProvidedFeatures(ServiceExtension ext) {
return allProvides;
}

private record InjectionFailure(InjectionPoint<ServiceExtension> injectionPoint, String failureDetail) {
private record InjectionFailure(ServiceExtension dependent, InjectionPoint<ServiceExtension> dependency,
@Nullable String failureDetail) {
@Override
public String toString() {
return "%s %s".formatted(injectionPoint.getTypeString(), failureDetail);
return "%s %s".formatted(dependency.getTypeString(), failureDetail);
}
}
}
5 changes: 2 additions & 3 deletions core/common/boot/src/main/java/org/eclipse/edc/boot/system/ExtensionLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import org.eclipse.edc.boot.monitor.MultiplexingMonitor;
import org.eclipse.edc.boot.system.injection.InjectionContainer;
import org.eclipse.edc.spi.EdcException;
import org.eclipse.edc.spi.monitor.ConsoleMonitor;
import org.eclipse.edc.spi.monitor.Monitor;
Expand Down Expand Up @@ -83,9 +82,9 @@ public Monitor loadMonitor(String... programArgs) {
/**
* Loads and orders the service extensions.
*/
public List<InjectionContainer<ServiceExtension>> loadServiceExtensions(ServiceExtensionContext context) {
public DependencyGraph buildDependencyGraph(ServiceExtensionContext context) {
var serviceExtensions = loadExtensions(ServiceExtension.class, true);
return new DependencyGraph(context).of(serviceExtensions);
return DependencyGraph.of(context, serviceExtensions);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions ...boot/src/main/java/org/eclipse/edc/boot/system/injection/ConfigurationInjectionPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,18 @@ public Result<List<InjectionContainer<T>>> getProviders(Map<Class<?>, List<Injec
.filter(Result::failed)
.map(AbstractResult::getFailureDetail)
.toList();
return violators.isEmpty() ? Result.success(List.of()) : Result.failure("%s (%s) --> %s".formatted(configurationObject.getName(), configurationObject.getType().getSimpleName(), violators));
return violators.isEmpty() ? Result.success(List.of()) : Result.failure("%s, through nested settings %s".formatted(toString(), violators));
}

@Override
public String getTypeString() {
return "Config object";
return "Configuration object";
}

@Override
public String toString() {
return "Configuration object '%s' of type '%s' in %s"
.formatted(configurationObject.getName(), configurationObject.getType(), targetInstance.getClass());
return "Configuration object \"%s\" of type [%s]"
.formatted(configurationObject.getName(), configurationObject.getType());
}

private Predicate<Constructor<?>> constructorFilter(List<FieldValue> args) {
Expand Down
4 changes: 2 additions & 2 deletions ...ommon/boot/src/main/java/org/eclipse/edc/boot/system/injection/ServiceInjectionPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public Result<List<InjectionContainer<T>>> getProviders(Map<Class<?>, List<Injec
return Result.success(List.of());
} else {
// attempt to interpret the feature name as class name and see if the context has that service
return Result.failure(injectedField.getName() + " of type " + serviceClass);
return Result.failure(toString());
}

}
Expand All @@ -118,6 +118,6 @@ public String getTypeString() {

@Override
public String toString() {
return format("Field \"%s\" of type [%s] required by %s", injectedField.getName(), getType(), instance.getClass().getName());
return format("Field \"%s\" of type [%s]", injectedField.getName(), getType());
}
}
12 changes: 6 additions & 6 deletions .../common/boot/src/main/java/org/eclipse/edc/boot/system/injection/ValueInjectionPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,12 @@ public Object resolve(ServiceExtensionContext context, DefaultServiceSupplier de
/**
* Determines whether a configuration value is "satisfied by" the given {@link ServiceExtensionContext} (the dependency map is ignored).
*
* @param dependencyMap Ignored
* @param context the {@link ServiceExtensionContext} in which the config is expected to be found.
* @param ignoredMap Ignored
* @param context the {@link ServiceExtensionContext} in which the config is expected to be found.
* @return success if found in the context, a failure otherwise.
*/
@Override
public Result<List<InjectionContainer<T>>> getProviders(Map<Class<?>, List<InjectionContainer<T>>> dependencyMap, ServiceExtensionContext context) {
public Result<List<InjectionContainer<T>>> getProviders(Map<Class<?>, List<InjectionContainer<T>>> ignoredMap, ServiceExtensionContext context) {

if (!annotationValue.required()) {
return Result.success(emptyProviderlist); // optional configs are always satisfied
Expand All @@ -177,17 +177,17 @@ public Result<List<InjectionContainer<T>>> getProviders(Map<Class<?>, List<Injec
// no default value, the required value may be found in the config
return context.getConfig().hasKey(annotationValue.key())
? Result.success(emptyProviderlist)
: Result.failure("%s (property \"%s\")".formatted(targetField.getName(), annotationValue.key()));
: Result.failure(toString());
}

@Override
public String getTypeString() {
return "Config value";
return "Configuration value";
}

@Override
public String toString() {
return "Configuration value \"%s\" of type %s (config key '%s') in %s".formatted(targetField.getName(), getType(), annotationValue.key(), declaringClass);
return "Configuration value \"%s\" of type [%s] (property '%s')".formatted(targetField.getName(), getType(), annotationValue.key());
}

private Object parseEntry(String string, Class<?> valueType) {
Expand Down
Loading
Loading