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.

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

fix: Fix usage of default @Provider methods in IdentityDidCoreExtension #1792

Conversation

bscholtes1A
Copy link
Contributor

What this PR changes/adds

Make some @Provider methods non-default in IdentityDidCoreExtension as this is causing non-deterministic dependency injection in IdentityHub project.

Why it does that

Default @Provider methods depending on injected services might be invoked while these services are not yet initialized.

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@bscholtes1A bscholtes1A changed the title Fix usage of default @Provider methods in IdentityDidCoreExtension did: Fix usage of default @Provider methods in IdentityDidCoreExtension Aug 5, 2022
@bscholtes1A bscholtes1A changed the title did: Fix usage of default @Provider methods in IdentityDidCoreExtension fix: Fix usage of default @Provider methods in IdentityDidCoreExtension Aug 5, 2022
@bscholtes1A bscholtes1A added the bug Something isn't working label Aug 5, 2022
@bscholtes1A bscholtes1A force-pushed the fix-default-provider-identity-did-core-extension branch from e0502e6 to 503dfe8 Compare August 5, 2022 14:55
try {
return (ECKey) ECKey.parseFromPEMEncodedObjects(encoded);
return (ECKey) JWK.parseFromPEMEncodedObjects(encoded);
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related to this change?

@codecov-commenter
Copy link

Codecov Report

Merging #1792 (8cc5914) into main (765983d) will decrease coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1792      +/-   ##
==========================================
- Coverage   67.82%   67.80%   -0.02%     
==========================================
  Files         795      800       +5     
  Lines       16899    16979      +80     
  Branches     1078     1080       +2     
==========================================
+ Hits        11461    11512      +51     
- Misses       4969     4997      +28     
- Partials      469      470       +1     
Impacted Files Coverage Δ
...aceconnector/iam/did/IdentityDidCoreExtension.java 66.66% <80.00%> (+8.60%) ⬆️
...ypes/domain/contract/offer/ContractDefinition.java 77.77% <0.00%> (-3.00%) ⬇️
...taspaceconnector/spi/types/domain/asset/Asset.java 88.57% <0.00%> (-2.06%) ⬇️
...tor/spi/types/domain/transfer/TransferProcess.java 67.85% <0.00%> (-0.03%) ⬇️
.../dataspaceconnector/spi/entity/StatefulEntity.java 0.00% <0.00%> (ø)
...clipse/dataspaceconnector/policy/model/Policy.java 81.15% <0.00%> (ø)
...onnector/policy/model/PolicyRegistrationTypes.java 0.00% <0.00%> (ø)
...nnector/sql/assetindex/schema/AssetStatements.java 100.00% <0.00%> (ø)
...iation/ConsumerContractNegotiationManagerImpl.java 87.38% <0.00%> (ø)
...main/contract/negotiation/ContractNegotiation.java 0.00% <0.00%> (ø)
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bscholtes1A bscholtes1A merged commit 7c0b678 into eclipse-edc:main Aug 8, 2022
paullatzelsperger pushed a commit that referenced this pull request Aug 9, 2022
…on (#1792)

* Fix usage of default @Provider methods in IdentityDidCoreExtension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants