Replace AutoMapper with Mapperly and update DI#5
Conversation
Migrate mapping implementation from AutoMapper to Mapperly (BBT.Aether.Mapperly): add Mapperly package, replace map profiles with Riok.Mapperly-style partial mappers and [Mapper] annotations, and register generated mappers in DI. Update host and test startup to call AddAetherMapperlyMapper instead of the AutoMapper variant. Bump Aether package version in Directory.Build.props and remove the AutoMapper package entry. Misc: make GitRepository ORM ctor public and add solution configurations (x64/x86) to the .sln.
Reviewer's GuideReplaces AutoMapper-based profiles with Mapperly partial mapper classes integrated via BBT.Aether.Mapperly, wires the new mappers into DI and host/test startup, adjusts GitRepository’s ORM constructor visibility, updates solution configurations, and bumps Aether-related package references while removing AutoMapper dependencies. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Summary |
|---|---|
Solution & Build Configuration templates/api/BBT.MyProjectName.sln, templates/api/Directory.Build.props |
Updated solution to include x64 and x86 platform configurations alongside Any CPU; bumped AetherPackageVersion from 1.0.19 to 1.0.20; removed AutoMapperPackageVersion property. |
NuGet Dependencies templates/api/src/BBT.MyProjectName.Application/BBT.MyProjectName.Application.csproj |
Removed AutoMapper reference; replaced BBT.Aether.Application with both BBT.Aether.Application and BBT.Aether.Mapperly using the updated version. |
Mapper Implementation templates/api/src/BBT.MyProjectName.Application/GitRepositories/GitRepositoryMapProfile.cs, templates/api/src/BBT.MyProjectName.Application/Issues/IssueMapProfile.cs |
Replaced AutoMapper Profile classes with Mapperly [Mapper] partial classes; introduced compile-time mapping methods with source-to-destination and destination-update overloads. |
Dependency Injection Registration templates/api/src/BBT.MyProjectName.Application/Microsoft/Extensions/DependencyInjection/MyProjectNameApplicationModuleServiceCollectionExtensions.cs, templates/api/src/BBT.MyProjectName.HttpApi.Host/Microsoft/Extensions/DependencyInjection/MyProjectNameApiServiceCollectionExtensions.cs, templates/api/test/BBT.MyProjectName.Infrastructure.Tests/InfrastructureEntryPoint.cs |
Registered new Mapperly mapper classes; changed mapper registration calls from AddAetherAutoMapperMapper to AddAetherMapperlyMapper. |
Model Access templates/api/src/BBT.MyProjectName.Domain/Issues/GitRepository.cs |
Changed GitRepository parameterless constructor visibility from private to public to enable ORM/framework instantiation. |
Possibly Related PRs
- burgan-tech/aether-template#3: Modifies
Directory.Build.propsto update theAetherPackageVersionproperty, directly related to the version bump in this PR.
Estimated Code Review Effort
🎯 2 (Simple) | ⏱️ ~12 minutes
🐰 A migration hops along,
AutoMapper makes way for Mapperly's song,
Compile-time mapping shines so bright,
From runtime configs to partial light! ✨
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
f/aether-upgrade
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 @coderabbitai help to get the list of available commands and usage tips.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the mapping infrastructure of the project by replacing AutoMapper with Mapperly. This change aims to improve performance and simplify the mapping definitions using Mapperly's code generation approach. Additionally, the pull request includes updates to the dependency injection setup and project configurations to accommodate the new mapping strategy. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The mapper registrations in
AddApplicationModuleonly addIssueMapperandGitRepositoryMapper, but not the newGitRepositoryCreateInputMapper,GitRepositoryUpdateInputMapper, orIssueListMapper; consider either registering all concrete mappers or relying solely onAddAetherMapperlyMapperfor consistency. - Changing
GitRepository's parameterless constructor from private to public opens it up for general use; if this is only intended for ORM, considerprotectedorprotected internalinstead to reduce accidental misuse. - The explicit
IssueListMapper : MapperBase<List<Issue>, List<IssueDto>>may be redundant if Mapperly already handles collection mappings via theIssueMapper; consider simplifying by relying on element mapping unless there is a specific need for a dedicated list mapper.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The mapper registrations in `AddApplicationModule` only add `IssueMapper` and `GitRepositoryMapper`, but not the new `GitRepositoryCreateInputMapper`, `GitRepositoryUpdateInputMapper`, or `IssueListMapper`; consider either registering all concrete mappers or relying solely on `AddAetherMapperlyMapper` for consistency.
- Changing `GitRepository`'s parameterless constructor from private to public opens it up for general use; if this is only intended for ORM, consider `protected` or `protected internal` instead to reduce accidental misuse.
- The explicit `IssueListMapper : MapperBase<List<Issue>, List<IssueDto>>` may be redundant if Mapperly already handles collection mappings via the `IssueMapper`; consider simplifying by relying on element mapping unless there is a specific need for a dedicated list mapper.
## Individual Comments
### Comment 1
<location path="templates/api/src/BBT.MyProjectName.Domain/Issues/GitRepository.cs" line_range="17" />
<code_context>
Name = Check.NotNullOrWhiteSpace(name, nameof(name));
}
- private GitRepository()
+ public GitRepository()
{
//For orm
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing the parameterless constructor from private to public weakens the aggregate’s invariants and may allow invalid state.
The private ctor ensured all `GitRepository` instances went through the `name` validation. Exposing a public parameterless ctor lets callers/ORMs bypass those checks and create invalid or partially initialized aggregates. If this is required for Mapperly/ORM support, prefer options like `internal` visibility, a factory, or mapper/ORM configuration that keeps the invariant-protecting constructor as the main entry point.
</issue_to_address>
### Comment 2
<location path="templates/api/src/BBT.MyProjectName.Application/Microsoft/Extensions/DependencyInjection/MyProjectNameApplicationModuleServiceCollectionExtensions.cs" line_range="22-24" />
<code_context>
services.AddAetherApplication();
+ // Mappers
+ services.AddScoped<IssueMapper>();
+ services.AddScoped<GitRepositoryMapper>();
+
// You can register your application service here.
</code_context>
<issue_to_address>
**suggestion (performance):** Mapperly mapper registrations could likely use Singleton lifetime instead of Scoped for better resource usage.
Since these Mapperly-generated mappers are stateless and thread-safe, they don’t need a scoped lifetime. Using `AddSingleton` would reduce per-request allocations without changing behavior, unless they depend on scoped services.
```suggestion
// Mappers (Mapperly-generated, stateless, thread-safe)
services.AddSingleton<IssueMapper>();
services.AddSingleton<GitRepositoryMapper>();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Name = Check.NotNullOrWhiteSpace(name, nameof(name)); | ||
| } | ||
|
|
||
| private GitRepository() |
There was a problem hiding this comment.
issue (bug_risk): Changing the parameterless constructor from private to public weakens the aggregate’s invariants and may allow invalid state.
The private ctor ensured all GitRepository instances went through the name validation. Exposing a public parameterless ctor lets callers/ORMs bypass those checks and create invalid or partially initialized aggregates. If this is required for Mapperly/ORM support, prefer options like internal visibility, a factory, or mapper/ORM configuration that keeps the invariant-protecting constructor as the main entry point.
| // Mappers | ||
| services.AddScoped<IssueMapper>(); | ||
| services.AddScoped<GitRepositoryMapper>(); |
There was a problem hiding this comment.
suggestion (performance): Mapperly mapper registrations could likely use Singleton lifetime instead of Scoped for better resource usage.
Since these Mapperly-generated mappers are stateless and thread-safe, they don’t need a scoped lifetime. Using AddSingleton would reduce per-request allocations without changing behavior, unless they depend on scoped services.
| // Mappers | |
| services.AddScoped<IssueMapper>(); | |
| services.AddScoped<GitRepositoryMapper>(); | |
| // Mappers (Mapperly-generated, stateless, thread-safe) | |
| services.AddSingleton<IssueMapper>(); | |
| services.AddSingleton<GitRepositoryMapper>(); |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the project from AutoMapper to Mapperly, updating package dependencies, mapper definitions, and dependency injection configurations. The changes are generally well-executed. However, I've identified a critical issue where several new mappers are not registered in the DI container, which would lead to runtime errors. I've provided a suggestion to fix this. Additionally, I've noted an opportunity for simplification by potentially removing a redundant list mapper.
|
|
||
| // Mappers | ||
| services.AddScoped<IssueMapper>(); | ||
| services.AddScoped<GitRepositoryMapper>(); |
There was a problem hiding this comment.
The new mappers for GitRepository (GitRepositoryCreateInputMapper and GitRepositoryUpdateInputMapper) are not registered for dependency injection. This will cause runtime errors when these mappers are required.
services.AddScoped<GitRepositoryMapper>();
services.AddScoped<GitRepositoryCreateInputMapper>();
services.AddScoped<GitRepositoryUpdateInputMapper>();| services.AddAetherApplication(); | ||
|
|
||
| // Mappers | ||
| services.AddScoped<IssueMapper>(); |
There was a problem hiding this comment.
IssueListMapper is defined in IssueMapProfile.cs but is not registered here for dependency injection. If it's intended to be used, it should be registered.
However, you should also consider if this mapper is necessary. Mapperly automatically handles mapping of collections if a mapping for the element type is defined (which IssueMapper provides). You might be able to remove IssueListMapper entirely for a simpler design.
1 participant
Migrate mapping implementation from AutoMapper to Mapperly (BBT.Aether.Mapperly): add Mapperly package, replace map profiles with Riok.Mapperly-style partial mappers and [Mapper] annotations, and register generated mappers in DI. Update host and test startup to call AddAetherMapperlyMapper instead of the AutoMapper variant. Bump Aether package version in Directory.Build.props and remove the AutoMapper package entry. Misc: make GitRepository ORM ctor public and add solution configurations (x64/x86) to the .sln.
Summary by Sourcery
Migrate the application template from AutoMapper to Mapperly-based mapping and align dependency injection, domain model, and configuration accordingly.
New Features:
Enhancements:
Build:
Chores:
Summary by CodeRabbit
Release Notes
Chores
Refactor
Bug Fixes