Skip to content

Commit

Permalink
refactor: remove unnecessary sort in DependencyGraph (#4593)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ndr-brt
ndr-brt authored Nov 4, 2024
1 parent 28b2133 commit 22b5f82
Show file tree
Hide file tree
Showing 17 changed files with 78 additions and 185 deletions.
2 changes: 0 additions & 2 deletions core/common/boot/src/main/java/org/eclipse/edc/boot/BootServicesExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.eclipse.edc.boot.health.HealthCheckServiceImpl;
import org.eclipse.edc.boot.system.ExtensionLoader;
import org.eclipse.edc.boot.vault.InMemoryVault;
import org.eclipse.edc.runtime.metamodel.annotation.BaseExtension;
import org.eclipse.edc.runtime.metamodel.annotation.Extension;
import org.eclipse.edc.runtime.metamodel.annotation.Provider;
import org.eclipse.edc.runtime.metamodel.annotation.Setting;
Expand All @@ -33,7 +32,6 @@
import java.time.Clock;


@BaseExtension
@Extension(value = BootServicesExtension.NAME)
public class BootServicesExtension implements ServiceExtension {

Expand Down
40 changes: 7 additions & 33 deletions core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import org.eclipse.edc.boot.system.injection.lifecycle.ServiceProvider;
import org.eclipse.edc.boot.util.CyclicDependencyException;
import org.eclipse.edc.boot.util.TopologicalSort;
import org.eclipse.edc.runtime.metamodel.annotation.BaseExtension;
import org.eclipse.edc.runtime.metamodel.annotation.CoreExtension;
import org.eclipse.edc.runtime.metamodel.annotation.Provides;
import org.eclipse.edc.runtime.metamodel.annotation.Requires;
import org.eclipse.edc.spi.EdcException;
Expand All @@ -33,7 +31,6 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -45,7 +42,6 @@

import static java.util.Optional.ofNullable;
import static java.util.function.Function.identity;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;
import static java.util.stream.Collectors.toSet;

Expand All @@ -67,14 +63,13 @@ public DependencyGraph(ServiceExtensionContext context) {
* 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.
*
* @param loadedExtensions A list of {@link ServiceExtension} instances that were picked up by the {@link ServiceLocator}
* @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> loadedExtensions) {
var extensions = sortByType(loadedExtensions);
public List<InjectionContainer<ServiceExtension>> of(List<ServiceExtension> extensions) {
Map<Class<?>, ServiceProvider> defaultServiceProviders = new HashMap<>();
Map<ServiceExtension, List<ServiceProvider>> serviceProviders = new HashMap<>();
Map<Class<?>, List<ServiceExtension>> dependencyMap = new HashMap<>();
Expand Down Expand Up @@ -136,19 +131,18 @@ public List<InjectionContainer<ServiceExtension>> of(List<ServiceExtension> load
}));

if (!unsatisfiedInjectionPoints.isEmpty()) {
var string = "The following injected fields were not provided:\n";
string += unsatisfiedInjectionPoints.stream().map(InjectionPoint::toString).collect(Collectors.joining("\n"));
throw new EdcInjectionException(string);
var message = "The following injected fields were not provided:\n";
message += unsatisfiedInjectionPoints.stream().map(InjectionPoint::toString).collect(Collectors.joining("\n"));
throw new EdcInjectionException(message);
}

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

sort.sort(extensions);

// convert the sorted list of extensions into an equally sorted list of InjectionContainers
return extensions.stream()
.map(key -> new InjectionContainer<>(key, injectionPoints.get(key), serviceProviders.get(key)))
.toList();
Expand Down Expand Up @@ -188,24 +182,4 @@ private Set<Class<?>> getProvidedFeatures(ServiceExtension ext) {
return allProvides;
}

/**
* Handles core-, transfer- and contract-extensions and inserts them at the beginning of the list so that
* explicit @Requires annotations are not necessary
*/
private List<ServiceExtension> sortByType(List<ServiceExtension> loadedExtensions) {
return loadedExtensions.stream().sorted(new SortByType()).collect(toList());
}

private static class SortByType implements Comparator<ServiceExtension> {
@Override
public int compare(ServiceExtension o1, ServiceExtension o2) {
return orderFor(o1.getClass()).compareTo(orderFor(o2.getClass()));
}

private Integer orderFor(Class<? extends ServiceExtension> class1) {
return class1.getAnnotation(BaseExtension.class) != null
? 0 : class1.getAnnotation(CoreExtension.class) != null
? 1 : 2;
}
}
}
42 changes: 17 additions & 25 deletions core/common/boot/src/test/java/org/eclipse/edc/boot/system/DependencyGraphTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,28 @@

package org.eclipse.edc.boot.system;

import org.assertj.core.data.Index;
import org.eclipse.edc.boot.system.injection.EdcInjectionException;
import org.eclipse.edc.boot.system.injection.InjectionContainer;
import org.eclipse.edc.spi.system.ServiceExtensionContext;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.eclipse.edc.boot.system.TestFunctions.mutableListOf;
import static org.mockito.Mockito.mock;

class DependencyGraphTest {

private DependencyGraph graph;

@BeforeEach
void setUp() {
graph = new DependencyGraph(mock(ServiceExtensionContext.class));
}
private final DependencyGraph graph = new DependencyGraph(mock());

@Test
void sortExtensions_withDefaultProvider() {
var providerExtension = TestFunctions.createProviderExtension(true);

var dependentExtension = TestFunctions.createDependentExtension(true);

var list = graph.of(TestFunctions.createList(dependentExtension, providerExtension));
assertThat(list).extracting(InjectionContainer::getInjectionTarget)
.contains(providerExtension, Index.atIndex(2))
.contains(dependentExtension, Index.atIndex(3));
var list = graph.of(mutableListOf(dependentExtension, providerExtension));

assertThat(list).extracting(InjectionContainer::getInjectionTarget)
.containsExactly(providerExtension, dependentExtension);
}

@Test
Expand All @@ -53,27 +44,28 @@ void sortExtensions_withNoDefaultProvider() {
var provider = TestFunctions.createProviderExtension(true);
var dependentExtension = TestFunctions.createDependentExtension(true);

var list = graph.of(TestFunctions.createList(dependentExtension, provider, defaultProvider));
var list = graph.of(mutableListOf(dependentExtension, provider, defaultProvider));

assertThat(list).extracting(InjectionContainer::getInjectionTarget)
.contains(provider, Index.atIndex(2))
.contains(defaultProvider, Index.atIndex(3))
.contains(dependentExtension, Index.atIndex(4));
.containsExactly(provider, defaultProvider, dependentExtension);
}

@Test
void sortExtensions_missingDependency() {

var dependentExtension = TestFunctions.createDependentExtension(true);
assertThatThrownBy(() -> graph.of(TestFunctions.createList(dependentExtension))).isInstanceOf(EdcInjectionException.class);

assertThatThrownBy(() -> graph.of(mutableListOf(dependentExtension)))
.isInstanceOf(EdcInjectionException.class);
}

@Test
void sortExtensions_missingOptionalDependency() {

var dependentExtension = TestFunctions.createDependentExtension(false);
assertThat(graph.of(TestFunctions.createList(dependentExtension))).hasSize(3)

var injectionContainers = graph.of(mutableListOf(dependentExtension));

assertThat(injectionContainers).hasSize(1)
.extracting(InjectionContainer::getInjectionTarget)
.usingRecursiveFieldByFieldElementComparator()
.containsOnly(dependentExtension);
.containsExactly(dependentExtension);
}
}
}
Loading

0 comments on commit 22b5f82

Please sign in to comment.