feat: improve DependencyGraph and error reporting (#4628)

* feat: improve DependencyGraph and error reporting

* remove unneeded methods

core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java

@@ -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;
@@ -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;
@@ -37,7 +36,6 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static java.util.stream.Collectors.toList;
@@ -48,25 +46,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()
@@ -94,30 +109,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) {
@@ -127,24 +143,50 @@ 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);
-        }
-
-        if (!unsatisfiedRequirements.isEmpty()) {
-            var message = String.format("The following @Require'd features were not provided: [%s]", String.join(", ", unsatisfiedRequirements));
-            throw new EdcException(message);
-        }
-
         sort.sort(injectionContainers);
 
+        return new DependencyGraph(injectionContainers, unsatisfiedInjectionPoints, unsatisfiedRequirements);
+    }
+
+    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();
@@ -156,7 +198,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
@@ -168,10 +210,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);
         }
     }
 }

core/common/boot/src/main/java/org/eclipse/edc/boot/system/ExtensionLoader.java

@@ -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;
@@ -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);
     }
 
     /**

core/common/boot/src/main/java/org/eclipse/edc/boot/system/injection/ConfigurationInjectionPoint.java

@@ -158,18 +158,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) {

core/common/boot/src/main/java/org/eclipse/edc/boot/system/injection/ServiceInjectionPoint.java

@@ -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());
         }
 
     }
@@ -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());
     }
 }

core/common/boot/src/main/java/org/eclipse/edc/boot/system/injection/ValueInjectionPoint.java

@@ -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
@@ -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) {