-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
Split ICrudAppservice
and implementations
#4346
Conversation
Thanks. I will review. |
Thanks @NecatiMeral for your contribution. I reviewed and it seems OK. However, this is a critical change and I want to get a deeper review from @maliming and @liangshiw |
...rk/src/Volo.Abp.Ddd.Application.Contracts/Volo/Abp/Application/Services/ICreateAppService.cs
Outdated
Show resolved
Hide resolved
.../Volo.Abp.Ddd.Application.Contracts/Volo/Abp/Application/Services/ICreateUpdateAppService.cs
Outdated
Show resolved
Hide resolved
...work/src/Volo.Abp.Ddd.Application.Contracts/Volo/Abp/Application/Services/ICrudAppService.cs
Outdated
Show resolved
Hide resolved
.../src/Volo.Abp.Ddd.Application.Contracts/Volo/Abp/Application/Services/IReadOnlyAppService.cs
Show resolved
Hide resolved
.../src/Volo.Abp.Ddd.Application/Volo/Abp/Application/Services/AbstractKeyReadOnlyAppService.cs
Outdated
Show resolved
Hide resolved
.../src/Volo.Abp.Ddd.Application.Contracts/Volo/Abp/Application/Services/IReadOnlyAppService.cs
Outdated
Show resolved
Hide resolved
hi @NecatiMeral I think we should use read-only repository in read-only application services. eg: abp/framework/src/Volo.Abp.Ddd.Domain/Volo/Abp/Domain/Repositories/IReadOnlyRepository.cs Line 8 in 42f37c5
|
hi @maliming, thanks for your review. This makes totally sense. I'll check the possibilities and update this PR. |
...work/src/Volo.Abp.Ddd.Application/Volo/Abp/Application/Services/AbstractKeyCrudAppService.cs
Outdated
Show resolved
Hide resolved
…lication services Still using `protected new` on `CrudAppService` because the repository member in `AbstractKeyCrudAppService` has to be renamed to something like `AbstractKeyRepository`, which would be a breaking change. Renaming the repository in each descendant could lead to misunderstandings, since in `CrudAppService` descendants will be three repositories to choose from: `Repository`, `ReadOnlyRepository`, `AbstractKeyRepository`. Currently the user must only differ between `ReadOnlyRepository` and `Repository`. @maliming, can you review this?
Resolve #4282
Updated
AbstractKeyCrudAppService
to use the newly introducedAbstractKeyReadOnlyAppService
.Implemented
ReadOnlyAppService
to provide a easy point to implement own (readonly) services.I have not provided default implementations for
ICreateAppService
,IUpdateAppService
andIDeleteAppService
because this would lead to a load of duplicated code.Maybe the
CrudAppService
implementation should act as a aggregating application service which delegates the methods to the actual services implementation, this would create a new layer of complexity and breaking changes.This has to be discussed first.
The issues requirements are adressed.