Fix singleton LiftService dependency on transient repositories #4091
Conversation
📝 WalkthroughWalkthroughLiftService transitions from constructor dependency injection to parameter-based injection by removing ISemanticDomainRepository and ISpeakerRepository from its constructor, making it a singleton with no transient dependencies. LiftController assumes these dependencies and passes them as method parameters. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Remove ISemanticDomainRepository and ISpeakerRepository from LiftService constructor - Pass these repositories as method parameters to LiftExport and CreateLiftRanges - Update ILiftService interface method signatures - Update LiftController to inject and pass repositories to LiftService methods - Update all test files to use parameterless LiftService constructor - This follows the existing pattern used for IWordRepository and IProjectRepository Co-authored-by: imnasnainaec <[email protected]>
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4091 +/- ##
===========================================
+ Coverage 74.57% 85.58% +11.00%
===========================================
Files 295 54 -241
Lines 10938 4772 -6166
Branches 1372 588 -784
===========================================
- Hits 8157 4084 -4073
+ Misses 2385 544 -1841
+ Partials 396 144 -252
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors LiftService to eliminate direct dependency injection of repository services, moving them to method parameters instead. This change aligns with the existing pattern for handling repositories and ensures proper dependency injection for the singleton LiftService.
Key Changes:
- Removed
ISemanticDomainRepositoryandISpeakerRepositoryfromLiftServiceconstructor - Updated
LiftExport()andCreateLiftRanges()method signatures to accept repositories as parameters - Modified
LiftControllerto inject repositories via primary constructor and pass them to service methods - Updated interface and test files to match new signatures
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Backend/Services/LiftService.cs | Removed repository fields from class, updated constructor to be parameterless, modified LiftExport and CreateLiftRanges methods to accept repositories as parameters |
| Backend/Interfaces/ILiftService.cs | Updated interface method signatures to match implementation changes |
| Backend/Controllers/LiftController.cs | Converted to primary constructor syntax, added ISemanticDomainRepository and ISpeakerRepository injection, updated service method calls to pass repositories |
| Backend.Tests/Services/LiftServiceTests.cs | Updated LiftService instantiation to use parameterless constructor |
| Backend.Tests/Controllers/LiftControllerTests.cs | Updated LiftService instantiation and LiftController constructor call to match new signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Backend.Tests/Services/LiftServiceTests.cs (1)
11-12: Consider removing unused mock fields.After the refactor,
_semDomRepoand_speakerRepoare initialized inSetup()but never used in any test. These fields are now dead code since the tests in this file only exercise export/import storage functionality (which doesn't require these repositories).♻️ Suggested cleanup
- private ISemanticDomainRepository _semDomRepo = null!; - private ISpeakerRepository _speakerRepo = null!; private ILiftService _liftService = null!;And in
Setup():[SetUp] public void Setup() { - _semDomRepo = new SemanticDomainRepositoryMock(); - _speakerRepo = new SpeakerRepositoryMock(); _liftService = new LiftService(); }Also applies to: 28-30
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Backend.Tests/Controllers/LiftControllerTests.csBackend.Tests/Services/LiftServiceTests.csBackend/Controllers/LiftController.csBackend/Interfaces/ILiftService.csBackend/Services/LiftService.cs
🧰 Additional context used
🧬 Code graph analysis (5)
Backend.Tests/Services/LiftServiceTests.cs (1)
Backend/Services/LiftService.cs (1)
LiftService(120-129)
Backend/Interfaces/ILiftService.cs (3)
Backend/Helper/Language.cs (3)
List(15-20)List(23-39)List(44-55)Backend/Models/SemanticDomain.cs (6)
SemanticDomainTreeNode(133-180)SemanticDomainTreeNode(171-179)SemanticDomainFull(82-128)SemanticDomainFull(96-101)SemanticDomainFull(103-116)SemanticDomainFull(119-127)src/api/models/semantic-domain-full.ts (1)
SemanticDomainFull(20-81)
Backend/Controllers/LiftController.cs (2)
Backend/Helper/CombineHub.cs (1)
ExportHub(12-15)src/components/ProjectExport/ExportHub.tsx (1)
ExportHub(13-26)
Backend.Tests/Controllers/LiftControllerTests.cs (4)
Backend/Services/LiftService.cs (1)
LiftService(120-129)Backend.Tests/Mocks/SemanticDomainRepositoryMock.cs (1)
SemanticDomainRepositoryMock(8-36)Backend.Tests/Mocks/HubContextMock.cs (1)
HubContextMock(12-17)src/components/ProjectExport/ExportHub.tsx (1)
ExportHub(13-26)
Backend/Services/LiftService.cs (2)
Backend/Interfaces/ILiftService.cs (6)
Task(12-12)Task(13-14)Task(15-16)Task(36-36)List(34-34)List(35-35)Backend/Models/SemanticDomain.cs (6)
SemanticDomainTreeNode(133-180)SemanticDomainTreeNode(171-179)SemanticDomainFull(82-128)SemanticDomainFull(96-101)SemanticDomainFull(103-116)SemanticDomainFull(119-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: docker_build
🔇 Additional comments (8)
Backend/Interfaces/ILiftService.cs (1)
13-16: Interface changes align with DI pattern fix.The updated method signatures correctly support per-request resolution of transient repositories by accepting them as parameters rather than relying on constructor injection. The
CreateLiftRangeschange to accept pre-fetchedallDomsdata avoids redundant repository calls and keeps the method self-contained.Backend.Tests/Controllers/LiftControllerTests.cs (1)
51-55: Test setup correctly updated for new DI pattern.The test initialization properly reflects the updated
LiftControllerconstructor signature and the parameterlessLiftServiceconstructor. The dependency wiring aligns with the production code changes.Backend/Controllers/LiftController.cs (2)
25-37: Controller refactoring follows established DI patterns.The controller now correctly owns the transient repository dependencies (
ISemanticDomainRepository,ISpeakerRepository) and passes them toLiftServicemethods as parameters. This aligns with the existing pattern used forIWordRepositoryandIProjectRepository, ensuring per-request resolution of transient services.
426-429: Export call correctly passes all required repositories.The
CreateLiftExportmethod properly forwards all repository dependencies to_liftService.LiftExport, matching the updated interface signature.Backend/Services/LiftService.cs (4)
120-129: Constructor correctly simplified to parameterless form.Removing the repository dependencies from the constructor resolves the singleton-holds-transient anti-pattern. The
Sldrinitialization and concurrent dictionary setup remain appropriately in the constructor as singleton-scoped state.
256-263: Method signature and project retrieval updated correctly.The expanded
LiftExportsignature accepts repositories as parameters, enabling per-request dependency resolution. The null-coalescing throw pattern (?? throw new MissingProjectException(...)) provides clear error handling for missing projects.
309-309: Repository usage correctly migrated to parameters.Speaker and semantic domain data are now fetched using the injected repository parameters. The null-coalescing to empty list (
?? []) on line 316 provides safe fallback when no semantic domains exist for English.Also applies to: 316-317
445-446: CreateLiftRanges refactored to use pre-fetched data.The method now accepts
allDomsas a parameter rather than fetching internally, which:
- Eliminates redundant repository calls (data already fetched in
LiftExport)- Removes the implicit async repository dependency
- Makes the method's data requirements explicit
The
Findon line 470 correctly handles the case where a parent domain may not exist by using null-conditional access (?.Name).Also applies to: 461-461, 470-470
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec).
LiftServiceis a singleton (to avoid reinitializing Sldr and prevent LanguageTag data leaks) but was injecting transientISpeakerRepositoryandISemanticDomainRepositoryvia constructor. This violates DI best practices—the singleton would hold these transient instances for its lifetime.Changes
ISemanticDomainRepositoryandISpeakerRepositoryparametersLiftExport()andCreateLiftRanges()to accept repositories as parametersLiftServicemethodsThis follows the existing pattern where
IWordRepositoryandIProjectRepositoryare passed as method parameters, allowing per-request resolution of transient services.Before:
After:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
This change is
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.