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: use new Configuration Injection mechanism everywhere #4633

Merged
merged 4 commits into from
Nov 18, 2024
Merged

feat: use new Configuration Injection mechanism everywhere #4633

merged 4 commits into from
Nov 18, 2024

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Nov 18, 2024 •
edited
Loading

What this PR changes/adds

This PR upgrades the entire code base to use the new Configuration Injection feature that was developed recently.

Note that this does not include:

  • runtime id and component id, as they are not read by an extension but by the ServiceExtensionContext
  • any property with a context attribute, as that is not yet implemented

Why it does that

reduce code verbosity

Further notes

  • in tests, the ObjectFactory has to be used to construct the extension, because at the time the extension is injected into the test method, it is already fully constructed (i.e. all config values are already assigned) and that means, that mocking config values in the test method is not possible, because it's too late. Directly injecting the extension into test methods only works when the config is set directly in the @BeforeEach method.
  • sorry for the big change set

Linked Issue(s)

Closes #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 18, 2024
@paullatzelsperger paullatzelsperger added the refactoring Cleaning up code and dependencies label Nov 18, 2024
@paullatzelsperger paullatzelsperger force-pushed the feat/update_codebase_config_injection branch from ea6551c to b5a9a79 Compare November 18, 2024 12:45
@paullatzelsperger paullatzelsperger force-pushed the feat/update_codebase_config_injection branch from b5a9a79 to 7a5d4e0 Compare November 18, 2024 12:51
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 18, 2024
View reviewed changes
.../iam/oauth2/oauth2-core/src/main/java/org/eclipse/edc/iam/oauth2/Oauth2ServiceExtension.java
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new NotBeforeValidationRule(clock, configuration.getNotBeforeValidationLeeway()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new ExpirationIssuedAtValidationRule(clock, configuration.getIssuedAtLeeway()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new AudienceValidationRule(config.getEndpointAudience()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new NotBeforeValidationRule(clock, config.getNotBeforeValidationLeeway()));

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
NotBeforeValidationRule.NotBeforeValidationRule
Loading
should be avoided because it has been deprecated.
.../iam/oauth2/oauth2-core/src/main/java/org/eclipse/edc/iam/oauth2/Oauth2ServiceExtension.java
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new ExpirationIssuedAtValidationRule(clock, configuration.getIssuedAtLeeway()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new AudienceValidationRule(config.getEndpointAudience()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new NotBeforeValidationRule(clock, config.getNotBeforeValidationLeeway()));
tokenValidationRulesRegistry.addRule(OAUTH2_TOKEN_CONTEXT, new ExpirationIssuedAtValidationRule(clock, config.getIssuedAtLeeway()));

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
ExpirationIssuedAtValidationRule.ExpirationIssuedAtValidationRule
Loading
should be avoided because it has been deprecated.
@paullatzelsperger paullatzelsperger changed the title feat: use new Configuration Injection mechanism everywhere" feat: use new Configuration Injection mechanism everywhere Nov 18, 2024
@paullatzelsperger paullatzelsperger mentioned this pull request Nov 18, 2024
Open
7 tasks
ndr-brt
ndr-brt approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

just some nits, is not easy to go deep in this :)

about the DependencyInjectionExtension, why not have the Configuration object automatically injected as mock objects so in every test the object can be controlled accordingly?

...ommon/junit/src/main/java/org/eclipse/edc/junit/extensions/DependencyInjectionExtension.java Outdated Show resolved Hide resolved
...e/src/main/java/org/eclipse/edc/connector/dataplane/selector/DataPlaneSelectorExtension.java Outdated Show resolved Hide resolved
@paullatzelsperger
Copy link
Member Author

paullatzelsperger commented Nov 18, 2024 •
edited
Loading

just some nits, is not easy to go deep in this :)

yeah, i know, unfortunately there isn't a good way to segment this PR :(

about the DependencyInjectionExtension, why not have the Configuration object automatically injected as mock objects so in every test the object can be controlled accordingly?

config objects (@Configuration) and config values (@Setting) are quite similar, both resolve the value from the context, or throught the default value attribute. Providing mocks for config objects would certainly be possible, but ultimately not add much value I think, unless we wrap all config values in a config object.

Maybe we could come up with a more elegant solution how config values are injected in the DependencyINjectionExtension scenario, but that is food for another PR i think

@paullatzelsperger paullatzelsperger merged commit 08f913e into eclipse-edc:main Nov 18, 2024
21 checks passed
@paullatzelsperger paullatzelsperger deleted the feat/update_codebase_config_injection branch November 18, 2024 16:18
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 approved these changes

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

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

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

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

@wolf4ood wolf4ood Awaiting requested review from wolf4ood

Assignees
No one assigned
Labels
enhancement New feature or request refactoring Cleaning up code and dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Configuration Injection
2 participants
@paullatzelsperger @ndr-brt