feat(service)!: migrate to AutoMapper for Model mapping #194

nanotaboada · 2025-04-07T12:47:58Z

Summary by CodeRabbit

New Features
- Player management endpoints now use clearly structured request and response models for consistent interactions.
Documentation
- API documentation has been refined with clearer summaries and parameter descriptions for create, retrieve, update, and delete operations.
Performance & Quality Improvements
- Data retrieval has been optimized to enhance performance by disabling change tracking.
- A robust mapping mechanism has been integrated to ensure reliable data transformations using AutoMapper.

coderabbitai · 2025-04-07T12:48:06Z

Walkthrough

This PR standardizes the handling of player data by replacing direct usage of the Player entity with distinct request and response models. It updates API endpoints, service interfaces, and tests across the project. Changes include adjustments to method signatures, test assertions, and fake data generation, as well as the integration of AutoMapper with a new mapping profile. Additionally, documentation comments and repository behavior have been revised to reflect these model changes.

Changes

File(s)	Change Summary
`Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs` `Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs`	Updated test method signatures and assertions to use `PlayerRequestModel` and `PlayerResponseModel`; variable renaming (e.g., `response` to `result`); added mapper configurations in tests.
`Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs` `.../PlayerFakes.cs` `.../PlayerMocks.cs`	Refactored fake and mock setups: replaced `CreateStarting11()` with `GetStarting11()`, added new methods for creating request/response models, and restructured mock initialization methods (e.g., `InitControllerMocks()`, `InitServiceMocks()`).
`Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.json` `Dotnet.Samples.AspNetCore.WebApi.csproj` `Dotnet.Samples.AspNetCore.WebApi/packages.lock.json`	Added AutoMapper (v14.0.0) dependency and updated project dependency definitions to include AutoMapper with its transitive dependencies.
`Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`	Updated method signatures and XML documentation: `PostAsync`, `GetByIdAsync`, `PutAsync`, etc., now use `PlayerRequestModel` and `PlayerResponseModel` to accurately represent input/output.
`Dotnet.Samples.AspNetCore.WebApi/Data/Repository.cs`	Modified `GetAllAsync` to use `AsNoTracking()` when querying, which disables change tracking for improved read performance.
`Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs`	Added a new mapping profile that configures AutoMapper to map between `Player`, `PlayerRequestModel`, and `PlayerResponseModel`; includes custom transformations (e.g., concatenating names, mapping positions).
`Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs`	Removed `[Required]` attributes from several properties and added XML documentation comments to improve clarity of the Player entity.
`Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs` `Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs`	Added new classes to represent structured input (request) and output (response) player models with necessary data annotations.
`Dotnet.Samples.AspNetCore.WebApi/Program.cs`	Added AutoMapper registration via `AddAutoMapper(typeof(PlayerMappingProfile))` to integrate the mapping configurations into the service container.
`Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs` `Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs`	Updated service interface and implementation to use the new request and response models; integrated AutoMapper for converting between models and the entity.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant PC as PlayerController
    participant PS as PlayerService
    participant AM as AutoMapper
    participant DB as Repository

    C->>PC: POST /players with PlayerRequestModel
    PC->>PS: CreateAsync(PlayerRequestModel)
    PS->>AM: Map PlayerRequestModel to Player
    AM-->>PS: Player entity
    PS->>DB: Add(Player)
    DB-->>PS: Confirmation
    PS->>AM: Map Player to PlayerResponseModel
    AM-->>PS: PlayerResponseModel
    PS-->>PC: IResult (201 Created with PlayerResponseModel)
    PC-->>C: HTTP Response

Possibly related PRs

feat(data)!: implement Repository pattern #189: Updates to the PlayerService and its associated tests for handling player data models; specifically, both PRs modify how player data is structured and tested using request/response models.

✨ Finishing Touches

📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

codacy-production · 2025-04-07T12:49:34Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +4.84% (target: -1.00%)	✅ 91.10%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`95b5a8b`)	1298	714	55.01%
Head commit (`ecc10ac`)	1594 (+296)	954 (+240)	59.85% (+4.84%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#194)	517	471	91.10%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

github-advanced-security

Sonarcsharp (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

codecov · 2025-04-07T12:50:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (95b5a8b) to head (ecc10ac).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #194   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          140       158   +18     
  Branches         7         9    +2     
=========================================
+ Hits           140       158   +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai

Actionable comments posted: 5

🧹 Nitpick comments (8)

Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1)

1-34: Consider removing unused using statement.

Since all [Required] attributes have been removed from the properties as part of migrating to separate request/response models, the System.ComponentModel.DataAnnotations import at line 1 might no longer be needed. Consider removing it if it's not used elsewhere in this file.
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (1)
33-39: Effective URL helper mock setup.

The SetupUrlHelperMock method properly configures the URL helper to return a response for any action context, which is necessary for testing controller actions that generate URLs.

Consider making the return value configurable to support more specific test scenarios:
-        public static Mock<IUrlHelper> SetupUrlHelperMock()
+        public static Mock<IUrlHelper> SetupUrlHelperMock(string? returnUrl = null)
         {
             var mock = new Mock<IUrlHelper>();
-            mock.Setup(url => url.Action(It.IsAny<UrlActionContext>())).Returns(It.IsAny<string>());
+            mock.Setup(url => url.Action(It.IsAny<UrlActionContext>())).Returns(returnUrl ?? "http://test-url.com");
 
             return mock;
         }
Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (1)
1-56: Well-implemented AutoMapper profile with thorough mapping configurations

The PlayerMappingProfile class provides a clear and comprehensive mapping configuration between the request model, entity, and response model. The mappings handle properties that require transformations appropriately and consider edge cases.

A few observations:

The Birth property mapping might generate unexpected output if DateOfBirth is null

The Starting11 property is being ignored in the mapping from request to entity, which means new players will always have the default value (false)

Consider adding null-check for the DateOfBirth formatting:
.ForMember(
    destination => destination.Birth,
-   options => options.MapFrom(source => $"{source.DateOfBirth:MMMM d, yyyy}")
+   options => options.MapFrom(source => source.DateOfBirth.HasValue ? $"{source.DateOfBirth:MMMM d, yyyy}" : "Unknown")
)
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)
1-37: Well-structured request model with proper validation

The PlayerRequestModel is well-designed with appropriate XML documentation and validation attributes on required fields.

Consider making the required properties non-nullable since the project has nullable reference types enabled:
-   [Required]
-   public string? FirstName { get; set; }
+   [Required]
+   public string FirstName { get; set; } = string.Empty;

-   [Required]
-   public string? LastName { get; set; }
+   [Required]
+   public string LastName { get; set; } = string.Empty;

-   [Required]
-   public string? AbbrPosition { get; set; }
+   [Required]
+   public string AbbrPosition { get; set; } = string.Empty;
This would align the type system with the validation requirements and provide better compile-time safety.
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (1)
254-277: Significant code duplication across test methods

The test file contains many similar blocks of code for setting up mocks, creating service instances, and verification logic.

Consider refactoring the test setup to reduce duplication by:

Creating helper methods for common setup patterns

Using a base test fixture with shared setup

Using parameterized tests for similar test cases with different inputs

For example:
private (PlayerService service, Mock<IMapper> mapper) SetupServiceWithMapper(Player returnedPlayer = null, PlayerResponseModel responseModel = null)
{
    var (repository, logger, memoryCache, mapper) = PlayerMocks.InitServiceMocks();
    
    if (returnedPlayer != null)
    {
        repository
            .Setup(repo => repo.FindByIdAsync(It.IsAny<long>()))
            .ReturnsAsync(returnedPlayer);
    }
    
    if (responseModel != null)
    {
        mapper
            .Setup(m => m.Map<PlayerResponseModel>(It.IsAny<Player>()))
            .Returns(responseModel);
    }
    
    var service = new PlayerService(
        repository.Object,
        logger.Object,
        memoryCache.Object,
        mapper.Object
    );
    
    return (service, mapper);
}
🧰 Tools

🪛 GitHub Check: Codeac Code Quality

[warning] 254-269: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:290
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs (1)
1-373: Consider refactoring to reduce test code duplication

Static analysis has flagged several instances of code duplication across test methods. While duplication in tests is common due to the AAA pattern, consider introducing helper methods for common setup and assertion patterns.

Some examples:

Common assertions for specific status codes

Setup for controller with invalid model state

Setup for controller with specific service return values

A simple helper method for status code assertions could look like:
private static void AssertStatusCode<T>(T result, int expectedStatusCode) where T : IResult
{
    result.Should().NotBeNull();
    result?.StatusCode.Should().Be(expectedStatusCode);
}
🧰 Tools

🪛 GitHub Check: Codeac Code Quality

[warning] 158-166: CodeDuplication
This block of 8 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:137

[warning] 137-145: CodeDuplication
This block of 8 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:158

[warning] 202-212: CodeDuplication
This block of 10 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:158

[warning] 158-168: CodeDuplication
This block of 10 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:202

[warning] 228-240: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:178

[warning] 178-190: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:228

[warning] 243-254: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:26

[warning] 26-37: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:243

[warning] 253-264: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:35

[warning] 35-46: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:253

[warning] 263-276: CodeDuplication
This block of 13 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:144

[warning] 144-157: CodeDuplication
This block of 13 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:263
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2)
179-181: Consider extracting the full name formatting logic

The full name formatting logic is quite complex and appears in multiple places in the code. Consider extracting this into a helper method to improve readability and maintainability.
+    private static string FormatFullName(string firstName, string? middleName, string lastName)
+    {
+        return $"{firstName} {(string.IsNullOrWhiteSpace(middleName) ? "" : middleName + " ")}{lastName}".Trim();
+    }

     // Then use it like this:
-    FullName =
-        $"{player.FirstName} {(string.IsNullOrWhiteSpace(player.MiddleName) ? "" : player.MiddleName + " ")}{player.LastName}".Trim(),
+    FullName = FormatFullName(player.FirstName, player.MiddleName, player.LastName),
244-251: Consider using a more specific exception type

Using ArgumentNullException for a "not found" scenario is somewhat unusual. Consider using InvalidOperationException or a custom exception that better communicates the intent.
-            ?? throw new ArgumentNullException($"Player with ID {id} not found.");
+            ?? throw new InvalidOperationException($"Player with ID {id} not found.");

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95b5a8b and 2b981cf.

📒 Files selected for processing (17)

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs (5 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (9 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.json (2 hunks)
Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (7 hunks)
Dotnet.Samples.AspNetCore.WebApi/Data/Repository.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Program.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (3 hunks)
Dotnet.Samples.AspNetCore.WebApi/packages.lock.json (1 hunks)

🧰 Additional context used

🧬 Code Definitions (9)

Dotnet.Samples.AspNetCore.WebApi/Program.cs (1)

Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (2)

PlayerMappingProfile (14-55)

PlayerMappingProfile (16-54)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2)

PlayerRequestModel (208-224)

PlayerRequestModel (253-271)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2)

PlayerResponseModel (226-242)

PlayerResponseModel (273-291)

Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (3)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (7)

Task (28-38)

Task (44-77)

Task (79-83)

Task (85-89)

Task (95-108)

Task (114-126)

Task (147-155)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (3)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1)

Player (11-34)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (4)

Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (6)

Task (15-15)

Task (22-22)

Task (32-32)

Task (42-42)

Task (49-49)

Task (56-56)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1)

Player (11-34)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (1)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (1)

PlayerService (8-156)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (3)

Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1)

Player (11-34)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (4)

Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (5)

Task (15-15)

Task (22-22)

Task (32-32)

Task (42-42)

Task (49-49)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (8)

Task (28-38)

Task (44-77)

Task (79-83)

Task (85-89)

Task (95-108)

Task (114-126)

Task (147-155)

PlayerService (8-156)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

🪛 GitHub Check: Codeac Code Quality

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs

[warning] 230-242: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:176

[warning] 176-188: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:230

[warning] 260-274: CodeDuplication
This block of 14 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:213

[warning] 213-227: CodeDuplication
This block of 14 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:260

[warning] 228-244: CodeDuplication
This block of 16 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:277

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs

[warning] 160-175: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:129

[warning] 156-171: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:217

[warning] 173-184: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:235

[warning] 217-232: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:156

[warning] 221-239: CodeDuplication
This block of 18 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:187

[warning] 254-269: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:290

[warning] 290-305: CodeDuplication
This block of 15 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs:254

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs

[warning] 35-46: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:253

[warning] 158-166: CodeDuplication
This block of 8 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:137

[warning] 137-145: CodeDuplication
This block of 8 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:158

[warning] 202-212: CodeDuplication
This block of 10 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:158

[warning] 158-168: CodeDuplication
This block of 10 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:202

[warning] 178-190: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:228

[warning] 144-157: CodeDuplication
This block of 13 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:263

[warning] 253-264: CodeDuplication
This block of 11 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:35

[warning] 263-276: CodeDuplication
This block of 13 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs:144

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Codacy Static Code Analysis
GitHub Check: Codacy Security Scan

🔇 Additional comments (53)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs (1)

51-51: Method name change aligns with new model mapping approach.

The change from PlayerFakes.CreateStarting11() to PlayerFakes.GetStarting11() is consistent with the overall PR objective of migrating to AutoMapper for model mapping. The new method name better reflects its purpose - retrieving domain entities rather than creating them directly. This separation allows the AutoMapper to handle the transformation between domain entities and response models elsewhere.
Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (3)

5-10: Good addition of XML documentation.

Adding clear XML documentation is a positive change that improves code readability and makes the purpose of this class as a database entity explicit, which aligns well with the migration to separate request and response models.

11-34: Validation moved to request models - good architectural pattern.

Removing the [Required] attributes from entity properties and moving validation to request models follows a clean separation of concerns. The entity now purely represents the database model, while validation constraints will be handled by the new PlayerRequestModel. This is a good architectural pattern that improves maintainability.

15-27:
✅ Verification successful

Verify validation is properly implemented in request models.

With the removal of [Required] attributes from these properties, ensure that proper validation is implemented in the corresponding PlayerRequestModel to maintain data integrity at the API boundary.

🏁 Script executed:
#!/bin/bash
# Search for PlayerRequestModel implementation to verify validation attributes
echo "Checking for PlayerRequestModel validation attributes..."
rg -A 15 "class PlayerRequestModel" --type csharp
Length of output: 1500
Validation Attributes Verified in PlayerRequestModel

The review comment issue regarding validation has been addressed. The [Required] attributes for critical fields (such as FirstName, LastName, and SquadNumber) are now correctly applied in the PlayerRequestModel (see Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs). No further changes are needed in Player.cs, as the split between the entity and request model properly delegates validation concerns.
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (5)

1-1: Appropriate import for AutoMapper integration.

The addition of the AutoMapper import aligns with the PR objective of migrating to AutoMapper for model mapping, which is needed to mock the IMapper interface now used in the service layer.

22-31: Well-structured controller mocks initialization.

The InitControllerMocks method returns both the service and logger mocks as a tuple, making the initialization more concise and the return values more explicit. This is a good refactoring that improves readability and makes test setup more intuitive.

41-53: Comprehensive service mocks initialization.

The InitServiceMocks method consolidates all service-related mocks (repository, logger, cache, mapper) into a single method with a clear tuple return type. This approach significantly improves test setup by providing all required mocks in one call, aligning with the AutoMapper integration objective.

50-50: Verify mapper configuration in tests.

The mapper mock is created but there's no setup for mapping behavior which will be needed in tests.

Make sure that tests using this method also include mapper configuration for the specific model mappings being tested, for example:
// Example test setup
var (_, _, _, mapper) = PlayerMocks.InitServiceMocks();
mapper.Setup(m => m.Map<PlayerResponseModel>(It.IsAny<Player>()))
      .Returns((Player p) => new PlayerResponseModel 
      { 
          Id = p.Id,
          // other properties
      });
55-55: Improved naming for memory cache mock method.

Renaming from MemoryCacheMock() to SetupMemoryCacheMock() follows a consistent naming convention that clearly indicates the method not only creates a mock but also configures it with specific behaviors.
Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj (1)

11-11: AutoMapper package added correctly

The AutoMapper package has been added with the latest stable version, which is appropriate for this migration.

Dotnet.Samples.AspNetCore.WebApi/Program.cs (2)

3-3: Appropriate namespace import added

The Mappings namespace import has been correctly added to support the registration of AutoMapper.

49-49: AutoMapper service registration properly implemented

The AutoMapper service has been registered correctly in the dependency injection container using the PlayerMappingProfile class as a marker for assembly scanning.

Dotnet.Samples.AspNetCore.WebApi/packages.lock.json (1)

5-13: Well-structured dependency integration

The addition of AutoMapper 14.0.0 as a direct dependency is appropriate for implementing the model mapping pattern. This aligns perfectly with the PR objective to migrate to AutoMapper.

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

1-28: Well-structured response model with good documentation

The PlayerResponseModel class is well-designed with appropriate property types and XML documentation. It correctly follows the DTO pattern for sending player data to clients.

Dotnet.Samples.AspNetCore.WebApi/Data/Repository.cs (1)

23-23: Good performance optimization with AsNoTracking

Adding AsNoTracking() is an excellent performance improvement for read-only operations. This reduces memory usage since the entities aren't being tracked by the change tracker.

Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.json (1)

47-54: Consistent dependency configuration in test project

The AutoMapper dependency is correctly added to both the main project and test project with consistent versioning, ensuring the test environment matches the production code.

Also applies to: 1817-1817

Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (5)

13-15: Method parameters and documentation have been standardized to use request models

The parameter type change from Player to PlayerRequestModel is consistent with the API design pattern of using request/response models instead of directly exposing domain entities. This change improves the separation of concerns between the API layer and domain layer.

22-22: Return type updated to use response model

Changing the return type from Task<List<Player>> to Task<List<PlayerResponseModel>> properly encapsulates the domain entity and allows for better control over what data is exposed to clients.

29-32: Return type changed from ValueTask to Task for consistency

The change from ValueTask<Player?> to Task<PlayerResponseModel?> not only replaces the entity with a response model but also standardizes on using Task rather than ValueTask. This ensures consistency across the interface.

39-42: Return type updated for squad number lookup

Similar to the ID lookup, this method now returns a response model rather than exposing the domain entity directly, maintaining the clean separation between API and domain layers.

47-49: Update method parameter changed to use request model

This change aligns with the pattern used in the CreateAsync method, ensuring consistency across all methods that modify data.

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (10)

1-2: Added AutoMapper and updated namespace imports

Adding the AutoMapper dependency is necessary for implementing the mapping between domain entities and request/response models.

11-12: Constructor parameters reordered and IMapper added

The IMapper dependency has been correctly added to the constructor parameters. When using the primary constructor syntax, order matters for readability.

15-18: Constants refactored for clarity and consistency

Renaming the constants and extracting environment variable names into constants improves code readability and maintainability.

22-22: IMapper field initialized

The mapper field is properly initialized from the constructor parameter.

28-38: CreateAsync method updated to use AutoMapper

The method now takes a PlayerRequestModel and maps it to a Player entity before persisting to the repository. This implementation correctly handles the conversion between the API model and domain entity.

44-76: RetrieveAsync method updated to handle response models in cache

The method has been refactored to:

Store and retrieve PlayerResponseModel objects in the cache instead of Player entities

Map the repository results to response models using AutoMapper

This approach ensures that the domain entities are not exposed outside the service layer.

79-83: RetrieveByIdAsync simplified with AutoMapper

The method implementation uses a clean, concise approach with the conditional operator to handle the mapping only when a player is found.

85-89: RetrieveBySquadNumberAsync uses the same pattern as RetrieveByIdAsync

Following the same pattern as the ID-based lookup provides consistency in the codebase.

95-108: UpdateAsync now uses AutoMapper to update the entity

The method:

Takes a request model as input

Finds the existing entity

Maps the request properties to the entity

Updates the repository

Clears the cache

This approach preserves the entity identity while updating its properties from the request model.

119-124: DeleteAsync cache clearing logic updated

Cache key references have been updated to use the constant.

Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (6)

22-24: Documentation improved for clarity

Minor improvement in documentation wording helps clarify the endpoint's purpose.

55-61: GetAsync method documentation and return type updated

The documentation and response type have been correctly updated to reflect the use of PlayerResponseModel.

77-84: GetByIdAsync method documentation and return type updated

The documentation now correctly uses the term "ID" (more common in API design) instead of "Id", and the return type has been updated to PlayerResponseModel.

102-107: GetBySquadNumberAsync method documentation and return type updated

Documentation and return type have been correctly updated to match the pattern of the other methods.

127-139: PutAsync method documentation and parameter type updated

The method signature has been correctly updated to use PlayerRequestModel instead of Player.

162-165: DeleteAsync method documentation improved

Minor improvement in documentation wording for consistency with other methods.

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (7)

25-25: Test method name improved for clarity

Renamed from GivenCreateAsync_WhenRepositoryAddAsync_ThenAddsPlayerToRepositoryAndRemovesMemoryCache to GivenCreateAsync_WhenRepositoryAddAsync_ThenAddsPlayerToRepositoryAndRemovesCache, which is more concise.

28-39: CreateAsync test updated to include IMapper

The test has been properly updated to include the new IMapper dependency and verify that AddAsync is called once.

54-68: RetrieveAsync test updated to verify AutoMapper usage

The test now properly sets up the mapper mock to return fake response models and verifies that the mapping is performed correctly.

77-81: Added verification for mapping

The test now explicitly verifies that the mapper is called once to map from entities to response models.

89-103: Second RetrieveAsync test updated to include IMapper

The test has been updated to include the new dependency and verify that the mapper is called only once.

114-118: Verification of mapping call in cached scenario

Correctly verifies that mapping only happens once in the first execution, not in the second (cached) execution.

312-312: ExecutionTimeAsync method made static

Making the helper method static is appropriate since it doesn't depend on instance state.
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs (7)

29-40: LGTM: Controller tests correctly updated for model validation

The test now properly uses the new PlayerRequestModel and correctly verifies the model state errors for the "SquadNumber" field instead of "FirstName", following the migration to request/response models.

47-63: LGTM: Properly updated to use PlayerResponseModel

The test has been correctly refactored to use the new model types, with proper setup of the mocked service to return PlayerResponseModel instead of direct Player entities.

70-95: Consider validating response model instead of request model

Line 93 asserts that the result value equals the request model with a comment indicating "Request not mapped to Response". In a typical API flow, the controller should return a response model (which might have different properties from the request).
-result?.Value.Should().BeEquivalentTo(request); // Request not mapped to Response
+// Consider validating against a response model that would be created from the request
+// or at minimum, validate only the essential fields that should match
Is this intentional behavior? Should the controller be mapping the request to a response model before returning it?

106-121: LGTM: GetAsync correctly uses PlayerResponseModel

The test has been properly updated to use response models instead of direct player entities, with appropriate type assertions.

128-140: LGTM: GET endpoints correctly updated to use PlayerResponseModel

All GET endpoint tests have been appropriately modified to work with the new response model structure. The test assertions correctly verify the new model types being returned from the controller.

Also applies to: 147-161, 168-183, 190-207, 214-235

246-259: LGTM: PUT endpoints correctly updated for request/response models

The PUT endpoint tests have been correctly refactored to use PlayerRequestModel for inputs and PlayerResponseModel for retrieval operations, with appropriate assertions.

Also applies to: 266-281, 289-309

320-334: LGTM: DELETE endpoints correctly updated for response models

The DELETE endpoint tests have been properly updated to work with response models when verifying entity existence, with appropriate type assertions.

Also applies to: 341-357
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2)

15-171: Method renamed from CreateStarting11 to GetStarting11 for better semantics

This update to the method name provides better clarity about its purpose - it retrieves an existing list of players rather than creating new ones. This change aligns well with the overall refactoring to clearly separate entity access from model creation.

173-188: New method to create response models from existing players

This addition helps support the AutoMapper migration by providing a way to generate PlayerResponseModel objects from Player entities. The method demonstrates the transformation logic applied to each Player, which is useful for testing purposes.

🧰 Tools

🪛 GitHub Check: Codeac Code Quality

[warning] 176-188: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:230

Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (1)
173-188: Refactor repeated logic using AutoMapper or shared helpers.

There is substantial overlap in manually creating PlayerResponseModel objects. In the past, a similar suggestion was made about extracting repeated mapping logic (see prior “Significant code duplication” comments). Since you have introduced AutoMapper, consider using it here too to avoid duplicated code:
- .Select(player => new PlayerResponseModel { ... })
+ .Select(player => Mapper.Map<PlayerResponseModel>(player))
This ensures consistency with production mapping and reduces maintenance overhead.

Also applies to: 226-242, 253-271, 273-291

🧰 Tools

🪛 GitHub Check: Codeac Code Quality

[warning] 176-188: CodeDuplication
This block of 12 lines is too similar to Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs:230

🧹 Nitpick comments (2)

Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (1)

27-27: Ignoring Starting11 might mask issues in integration tests.

If tests rely on the Starting11 flag, consider a valid mapping approach or confirm that ignoring it is intentional.
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (1)
244-251: Consider throwing a more relevant exception.

For an unfound ID, a KeyNotFoundException or a domain-specific exception might be more semantically accurate than ArgumentNullException.
- ?? throw new ArgumentNullException($"Player with ID {id} not found.");
+ ?? throw new KeyNotFoundException($"Player with ID {id} not found.");

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b981cf and 6a6d5a6.

📒 Files selected for processing (19)

.codacy.yml (1 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs (5 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (9 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.json (2 hunks)
Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (7 hunks)
Dotnet.Samples.AspNetCore.WebApi/Data/Repository.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Mappings/PlayerMappingProfile.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Program.cs (2 hunks)
Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (1 hunks)
Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (3 hunks)
Dotnet.Samples.AspNetCore.WebApi/packages.lock.json (1 hunks)
codecov.yml (1 hunks)

✅ Files skipped from review due to trivial changes (1)

codecov.yml

🚧 Files skipped from review as they are similar to previous changes (12)

Dotnet.Samples.AspNetCore.WebApi/Data/Repository.cs
Dotnet.Samples.AspNetCore.WebApi/Program.cs
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs
Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs
Dotnet.Samples.AspNetCore.WebApi/packages.lock.json
Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs
Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.json
Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj
Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs
Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs
Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs

🧰 Additional context used

🧬 Code Definitions (2)

Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (3)

Dotnet.Samples.AspNetCore.WebApi/Models/Player.cs (1)

Player (11-34)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerServiceTests.cs (4)

Dotnet.Samples.AspNetCore.WebApi/Services/IPlayerService.cs (5)

Task (15-15)

Task (22-22)

Task (32-32)

Task (42-42)

Task (49-49)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs (8)

Task (28-38)

Task (44-77)

Task (79-83)

Task (85-89)

Task (95-108)

Task (114-126)

Task (147-155)

PlayerService (8-156)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerRequestModel.cs (1)

PlayerRequestModel (13-36)

Dotnet.Samples.AspNetCore.WebApi/Models/PlayerResponseModel.cs (1)

PlayerResponseModel (11-28)

🪛 GitHub Check: Sonarcsharp (reported by Codacy)

Dotnet.Samples.AspNetCore.WebApi/Services/PlayerService.cs

[warning] 1-1:
Analyzer 'SonarAnalyzer.Rules.CSharp.SymbolicExecutionRunner' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.
Exception occurred with following context:
Compilation: srcassembly.dll
SyntaxTree: PlayerService.cs
SyntaxNode: public async Task<PlayerResponseModel ... [MethodDeclarationSyntax]@[3468..3734) (84,4)-(88,5)