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: Dagger refactor #134

Merged
merged 17 commits into from
Aug 27, 2024
Merged

feat: Dagger refactor #134

merged 17 commits into from
Aug 27, 2024

Conversation

AlfredoG87
Copy link
Contributor

@AlfredoG87 AlfredoG87 commented Aug 24, 2024 •
edited
Loading

Description:
This PR is part 2 of #118,

  • Refactors everything within the startup sequence (already moved to BlockNodeApp) so dependencies are automatically injected by Dagger DI Framework.
  • Adds Unit Test for BlockNodeApp
  • Adds needed modules and Dagger Beans so all can be initialized by DI.
  • Adds UT for some of the Provides Modules.
  • Refactored Dagger Component to receive Configuration instead of creating it itself.

Related issue(s):

Fixes #110

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@AlfredoG87
Injecting BlockWriter as part of PersistenInjectionModule
cedb64d 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Refactoring everything at BlockNodeApp to use Dagger singletons inste…
31eddb9 
…ad injected at the constructor by DaggerComponent, instead of initializing them on the startup sequence. Also, adding a BlockNodeApp Test now that everything is being injected.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Improving test and including missing java docs
e945fb1 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
bringing back mock needed for test
58b61a7 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
started refactor of BlockNodeContextFactory and BlockNodeContext, is …
41db6ca 
…working, some tests are failing, but saving this checkpoint before further large scale refactor.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Fixed UT for existing tests, ContextFactory is just a Shell, we shoul…
3f563f3 
…d be able to get rid of it soon.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
fixing warnings
a00e0b3 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Got rid of the BlockNodeContextFactory, next step, probably for a fut…
f9b69e2 
…ure PR, is to get Rid of the BlockNodeContext completely. Now that we have Dagger dependencies are cheap, and is better to inject the minimum amount of them, only when in need.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
fixed javadocs and exported metrics to remove warning
883969c 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Refactor BlockNodeAppInjectionComponent to need an external configura…
30cda24 
…tion instead of providing one itself.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Moved PersistenceInjectionModule to persistance package instead of pe…
99aa01f 
…rsistence.storage

Added export metricsService to the module-info

Refactored Dagger Component to receive a Configuration from outside.

Added UT for MediatorInjectionModule

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Added UT for PersistenceInjectionModule
1b2690c 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Added UT for missing InjectionModules, removed unneeded configuration…
689b6fc 
… definition from ConfigExtensions

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87 AlfredoG87 self-assigned this Aug 25, 2024
@AlfredoG87 AlfredoG87 added Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule. labels Aug 25, 2024
@AlfredoG87 AlfredoG87 added this to the 0.1.0 milestone Aug 25, 2024
@AlfredoG87 AlfredoG87 marked this pull request as ready for review August 26, 2024 20:53
@AlfredoG87 AlfredoG87 requested a review from a team as a code owner August 26, 2024 20:53
@AlfredoG87
Refactored GrpcProvider dagger bean to use the interface instead of t…
15076c5 
…he implementation, to be able to use Bind instead of Provide, deleted unneeded UT.

Removed @nonnull annotation from injection module, since is redundant.

Signed-off-by: Alfredo Gutierrez <[email protected]>
mattp-swirldslabs
mattp-swirldslabs previously approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Looks good

server/src/main/java/com/hedera/block/server/Server.java Outdated Show resolved Hide resolved
mattp-swirldslabs
mattp-swirldslabs approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Looks good

@AlfredoG87 AlfredoG87 merged commit d00633e into main Aug 27, 2024
13 checks passed
@AlfredoG87 AlfredoG87 deleted the dagger-refactor branch August 27, 2024 14:59
@codecov Codecov
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (587fac2) to head (020dd92).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@              Coverage Diff              @@
##               main      #134      +/-   ##
=============================================
+ Coverage     93.90%   100.00%   +6.09%     
- Complexity      122       132      +10     
=============================================
  Files            23        26       +3     
  Lines           525       510      -15     
  Branches         27        27              
=============================================
+ Hits            493       510      +17     
+ Misses           32         0      -32
Files Coverage Δ
...ain/java/com/hedera/block/server/BlockNodeApp.java 100.00% <100.00%> (+100.00%) ⬆️
...dera/block/server/BlockNodeAppInjectionModule.java 100.00% <100.00%> (+66.66%) ⬆️
...va/com/hedera/block/server/BlockStreamService.java 100.00% <ø> (ø)
.../block/server/config/BlockNodeConfigExtension.java 100.00% <ø> (ø)
...m/hedera/block/server/config/BlockNodeContext.java 100.00% <ø> (ø)
...era/block/server/config/ConfigInjectionModule.java 100.00% <100.00%> (ø)
...block/server/mediator/MediatorInjectionModule.java 100.00% <100.00%> (ø)
...a/block/server/metrics/MetricsInjectionModule.java 100.00% <100.00%> (ø)
...server/persistence/PersistenceInjectionModule.java 100.00% <100.00%> (ø)

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

@mattp-swirldslabs mattp-swirldslabs mattp-swirldslabs approved these changes

Assignees

@AlfredoG87 AlfredoG87

Labels
Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule.
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.

Use Dagger as DI in codebase
2 participants
@AlfredoG87 @mattp-swirldslabs