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

wants to merge 1 commit into from

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Nov 14, 2024 •
edited
Loading

What this PR changes/adds

This PR improves the public API of the DependencyGraph object and makes it a bit more usable. In addition, error reporting of dependency injection
errors is externalized and moved into the BaseRuntime class.

After dependency resolution, all problems are collected and a single EdcInjectionException is thrown by the BaseRuntime, for example:

Caused by: org.eclipse.edc.boot.system.injection.EdcInjectionException: The following problems occurred during dependency injection:
## class org.eclipse.edc.iam.identitytrust.sts.remote.client.StsRemoteClientConfigurationExtension is missing
  --> Field "fooService" of type [class org.eclipse.edc.iam.identitytrust.sts.remote.client.StsRemoteClientConfigurationExtension$FooService]
  --> Configuration object "clientConfig" of type [class org.eclipse.edc.iam.identitytrust.sts.remote.client.StsRemoteClientConfigurationExtension$StsClientConfig], through nested settings [Configuration value "tokenUrl" of type [class java.lang.String] (property 'edc.iam.sts.oauth.token.url'), Configuration value "clientId" of type [class java.lang.String] (property 'edc.iam.sts.oauth.client.id'), Configuration value "clientSecretAlias" of type [class java.lang.String] (property 'edc.iam.sts.oauth.client.secret.alias')]
	... 2 more

Why it does that

Dependency injection errors (services, config values and config objects) wasn't very elegant and needed improvement.

Linked Issue(s)

Contributes to #4610

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@paullatzelsperger paullatzelsperger added the enhancement New feature or request label Nov 14, 2024
@paullatzelsperger paullatzelsperger marked this pull request as draft November 14, 2024 13:47
@paullatzelsperger paullatzelsperger marked this pull request as ready for review November 14, 2024 13:50
@paullatzelsperger paullatzelsperger force-pushed the feat/improve_dependency_graph branch 2 times, most recently from e4a588e to e771686 Compare November 14, 2024 14:16
@paullatzelsperger paullatzelsperger mentioned this pull request Nov 14, 2024
Open
7 tasks
ndr-brt
ndr-brt reviewed Nov 15, 2024
View reviewed changes
core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
* @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.

core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
* @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

core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
* @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

core/common/boot/src/main/java/org/eclipse/edc/boot/system/DependencyGraph.java
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndr-brt ndr-brt ndr-brt left review comments

@jimmarino jimmarino Awaiting requested review from jimmarino jimmarino is a code owner

@wolf4ood wolf4ood Awaiting requested review from wolf4ood

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paullatzelsperger @ndr-brt