Revert "Align Orders queries with existing use case view models"#16
Revert "Align Orders queries with existing use case view models"#16
Conversation
Summary of ChangesHello @mzet97, 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 completely reverts the changes introduced in PR #13, which aimed to align order queries with existing use case view models. The reversion involves removing the Paramore.Darker query processing framework, consolidating Brighter event mapping logic into the shared domain, and eliminating numerous command, query, and event classes that were part of the previous implementation. This effectively rolls back the application's event dispatching and query handling mechanisms to their state before PR #13. Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![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 reverts changes from PR #13, which aimed to align Orders queries. The revert correctly removes numerous files and dependencies related to Paramore.Darker and the CQRS query pattern. However, the revert appears to be incomplete and has a critical side effect: it disables the domain event dispatching system. While event-related files are moved to the Shared project, their dependency injection registrations and the crucial DomainEventsSaveChangesInterceptor are removed. This will cause domain events, which are still being created in entities like Order, to be silently ignored. This needs to be addressed to ensure application correctness.
I am having trouble creating individual review comments. Click here to see my feedback.
src/EChamado/Server/EChamado.Server.Infrastructure/Configuration/DependencyInjectionConfig.cs (46-64)
Removing these service registrations, along with the DomainEventsSaveChangesInterceptor (which is also removed in this PR), will break the domain event dispatching mechanism. Domain entities like Order are still generating events (e.g., OrderCreated, OrderUpdated), but they will no longer be dispatched and handled. This is a critical issue as it can lead to silent failures and inconsistent state.
This revert seems to have unintentionally removed the wiring for domain events. Please either:
- Restore the domain event dispatching infrastructure, including these DI registrations and the
DomainEventsSaveChangesInterceptor. - If domain events are no longer desired, remove the event generation logic from the domain entities (e.g., calls to
AddEventinOrder.cs).
Given that event-related files were moved to the Shared project, it seems likely that their removal from DI was an oversight.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>(); | ||
| options.UseLoggerFactory(loggerFactory); | ||
| options.UseNpgsql(configuration.GetConnectionString("DefaultConnection")); |
There was a problem hiding this comment.
This DbContext configuration no longer adds the DomainEventsSaveChangesInterceptor, so aggregates that call AddEvent (e.g., OrderCreated/OrderUpdated in Order) will keep their events in-memory but nothing ever publishes them through Brighter or clears them after SaveChangesAsync. As a result, creating or updating orders/categories/statuses will silently skip all domain-event side effects and message bus handlers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR reverts changes from PR #13, which attempted to align Orders queries with existing use case view models. The revert removes duplicate CQRS query implementations that overlapped with existing functionality and reorganizes domain event infrastructure from the Infrastructure layer to the Shared layer for better architectural separation.
Key changes:
- Removes duplicate query handlers and queries for Orders, Categories, and StatusTypes that duplicated existing UseCases implementations
- Removes duplicate command handlers and commands for Order operations (Create, Update, Close, Assign, ChangeStatus) that already exist in UseCases folder
- Moves domain event infrastructure (IDomainEventDispatcher, IBrighterEventMapper, BrighterEventMapper, BrighterDomainEventDispatcher) from
EChamado.Server.Infrastructure.EventstoEChamado.Shared.Domain.Eventsfor better reusability - Removes the DomainEventsSaveChangesInterceptor and its registration from the database context configuration
- Cleans up unused event wrapper classes and Brighter event registrations from DependencyInjectionConfig
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| DomainEventsSaveChangesInterceptor.cs | Removed EF Core interceptor that was dispatching domain events on save |
| IDomainEventDispatcher.cs (Infrastructure) | Removed from Infrastructure layer (moved to Shared) |
| IBrighterEventMapper.cs (Infrastructure) | Removed from Infrastructure layer (moved to Shared) |
| BrighterDomainEventDispatcher.cs (Infrastructure) | Removed from Infrastructure layer (moved to Shared) |
| BrighterEventMapper.cs (Infrastructure) | Removed from Infrastructure layer (moved to Shared) |
| IdentityConfig.cs | Removed interceptor registration from DbContext configuration |
| DependencyInjectionConfig.cs (Infrastructure) | Removed Brighter event mapper registrations and dispatcher services; minor using statement ordering issue |
| DependencyInjectionConfig.cs (Application) | Removed Paramore.Darker query processor and handler registrations; Paramore.Darker package now unused |
| ServiceProviderQueryProcessor.cs | Removed custom query processor implementation for Paramore.Darker |
| ListStatusTypesQuery/Handler.cs | Removed duplicate query implementation |
| ListOrdersQuery/Handler.cs | Removed duplicate query implementation |
| ListCategoriesQuery/Handler.cs | Removed duplicate query implementation |
| GetOrderByIdQuery/Handler.cs | Removed duplicate query implementation |
| Update/Create/Close/Assign/ChangeOrderStatus Command/Handler.cs | Removed duplicate command implementations |
| *Events.cs (multiple files) | Removed Brighter event wrapper classes |
| IDomainEventDispatcher.cs (Shared) | Added interface to Shared layer with proper namespace |
| IBrighterEventMapper.cs (Shared) | Added interface to Shared layer with Portuguese documentation |
| BrighterEventMapper.cs (Shared) | Moved to Shared layer with formatting improvements |
| BrighterDomainEventDispatcher.cs (Shared) | Moved to Shared layer; contains redundant System.Threading using |
| EChamado.Shared.csproj | Added Paramore.Brighter package reference (needed for new Shared classes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using EChamado.Server.Application.Common; | ||
| using EChamado.Server.Application.Common.Behaviours; | ||
| using EChamado.Server.Application.Common.Messaging; | ||
| using EChamado.Server.Application.Orders.QueryHandlers; | ||
| using EChamado.Server.Application.Orders.Queries; | ||
| using EChamado.Server.Application.Services; | ||
| using EChamado.Server.Application.Services.AI; | ||
| using EChamado.Server.Application.Services.AI.Configuration; | ||
| using EChamado.Server.Application.Services.AI.Interfaces; | ||
| using EChamado.Server.Application.Services.AI.Providers; | ||
| using EChamado.Server.Application.UseCases.Categories.ViewModels; | ||
| using EChamado.Server.Application.UseCases.Orders.ViewModels; | ||
| using EChamado.Server.Application.UseCases.StatusTypes.ViewModels; | ||
| using EChamado.Server.Domain.Services.Interface; | ||
| using FluentValidation; | ||
| using MediatR; | ||
| using Microsoft.Extensions.Caching.Memory; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Paramore.Brighter; | ||
| using Paramore.Brighter.Extensions.DependencyInjection; | ||
| using Paramore.Darker; | ||
| using System.Reflection; |
There was a problem hiding this comment.
The Paramore.Darker package reference is now unused after this revert. All query handlers and the ServiceProviderQueryProcessor that used Paramore.Darker have been removed. Consider removing the Paramore.Darker package reference from EChamado.Server.Application.csproj to avoid unnecessary dependencies.
| @@ -0,0 +1,34 @@ | |||
| using Paramore.Brighter; | |||
| using System.Threading; | |||
There was a problem hiding this comment.
The using System.Threading; directive is redundant. Since the project has ImplicitUsings enabled (as seen in EChamado.Shared.csproj), the System.Threading namespace is already included implicitly, and CancellationToken is available without this explicit using statement. Consider removing this line to reduce unnecessary imports.
| using System.Threading; |
| using Microsoft.AspNetCore.Identity; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using EChamado.Server.Infrastructure.Services; |
There was a problem hiding this comment.
The using statement for EChamado.Server.Infrastructure.Services is not in alphabetical order. It should be placed between lines 5 and 6 (after EChamado.Server.Infrastructure.Email and before EChamado.Server.Infrastructure.Persistence) to maintain consistent using statement ordering.
| foreach (var de in domainEvents) | ||
| { | ||
| var req = _mapper.Map(de); | ||
| if (req is null) continue; | ||
|
|
||
| // Se for Event, Publish. Se for Command, Send. | ||
| // Mantém compatível com padrões do Brighter. | ||
| if (req is Event) | ||
| await _processor.PublishAsync(req, cancellationToken: ct); | ||
| else | ||
| await _processor.SendAsync(req, cancellationToken: ct); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
|
Reverts #13