abpframework / abp

Open Source Web Application Framework for ASP.NET Core. Offers an opinionated architecture to build enterprise software solutions with best practices on top of the .NET and the ASP.NET Core platforms. Provides the fundamental infrastructure, production-ready startup templates, application modules, UI themes, tooling, guides and documentation.
https://abp.io
GNU Lesser General Public License v3.0
12.31k stars 3.32k forks source link

UnitOfWorkManager.Begin() doesn't work #2452

Closed gdlcf88 closed 4 years ago

gdlcf88 commented 4 years ago

Create a method in application service:

        [UnitOfWork(IsDisabled = true)]
        public async Task Test()
        {
            using (var uow = _unitOfWorkManager.Begin())
            {
                var user = await _identityUserRepository.InsertAsync(
                    new IdentityUser(GuidGenerator.Create(), "testUser", "testUser@gmail.com", CurrentTenant.Id), true);
                // no commit
                throw new ApplicationException();
            }
        }

Run it, you can see that a user has been created.

Abp v1.1.2, Npgsql

olicooper commented 4 years ago

I think this is how it is meant to work. I believe this is like disabling transactions (which means all database requests happen immediately). Please also see this: https://github.com/abpframework/abp/blob/dev/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkAttribute.cs#L33

gdlcf88 commented 4 years ago

I think this is how it is meant to work. I believe this is like disabling transactions (which means all database requests happen immediately). Please also see this: https://github.com/abpframework/abp/blob/dev/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkAttribute.cs#L33

Test() is a application service method, I forgot to explain.

maliming commented 4 years ago

IsDisabled If there is already a started unit of work, this property is ignored.

Try

[UnitOfWork(IsDisabled = true)]
public async Task Test()
{
    using (var uow = _unitOfWorkManager.Begin(requiresNew: true))
    {
        var user = await _identityUserRepository.InsertAsync(
            new IdentityUser(GuidGenerator.Create(), "testUser", "testUser@gmail.com", CurrentTenant.Id), true);
        // no commit
        throw new ApplicationException();
    }
}
gdlcf88 commented 4 years ago

Is this because controller automatically uses UOW? @maliming

gdlcf88 commented 4 years ago

I test it in Abp vNext v0.19.0, cannot reproduce this problem.

maliming commented 4 years ago

I will check it later.

maliming commented 4 years ago

Is the Testmethod called through a Getrequest?

gdlcf88 commented 4 years ago

Is the Testmethod called through a Getrequest?

It is a post request by default image

maliming commented 4 years ago

Please share a project to reproduce it.

gdlcf88 commented 4 years ago

Please share a project to reproduce it.

Steps to reproduce:

  1. Create a new abp v1.1.2 solution. (or using any existing solution)

  2. Create TestAppService.cs with these codes:

    
    using System;
    using System.Threading.Tasks;
    using Volo.Abp.Application.Services;
    using Volo.Abp.Identity;
    using Volo.Abp.Uow;

namespace MyProject.Test { public class TestAppService : ApplicationService { private readonly IUnitOfWorkManager _unitOfWorkManager; private readonly IIdentityUserRepository _identityUserRepository;

    public TestAppService(
        IUnitOfWorkManager unitOfWorkManager,
        IIdentityUserRepository identityUserRepository)
    {
        _unitOfWorkManager = unitOfWorkManager;
        _identityUserRepository = identityUserRepository;
    }

    [UnitOfWork(IsDisabled = true)]
    public async Task TestAsync()
    {
        using (var uow = _unitOfWorkManager.Begin())
        {
            var user = await _identityUserRepository.InsertAsync(
                new IdentityUser(GuidGenerator.Create(), "testUser", "testUser@gmail.com", CurrentTenant.Id), true);
            // no commit
            throw new ApplicationException();
        }
    }
}

}



3. Run /api/app/test/test
maliming commented 4 years ago

Everything works fine after transaction is enabled.

using (var uow = _unitOfWorkManager.Begin(new AbpUnitOfWorkOptions
{
    IsTransactional = true
}))
{
    var user = await _identityUserRepository.InsertAsync(
        new IdentityUser(GuidGenerator.Create(), "testUser2", "testUs2er@gmail.com", CurrentTenant.Id),
        true);

    // no commit
    throw new ApplicationException();
}
gdlcf88 commented 4 years ago

Why IsTransactional is false by default in new abp version?

gdlcf88 commented 4 years ago

I found the remark in abp v0.19.0 codes:

        /// <summary>
        /// Default: false.
        /// </summary>
        public bool IsTransactional { get; set; }

It seems IsTransactional in v0.19.0 is false by default too, but it doesn't have this problem, so what makes the difference?

And how about making it true by default?

gdlcf88 commented 4 years ago

I found a description in old abp docs: https://aspnetboilerplate.com/Pages/Documents/Unit-Of-Work Use a non-transactional unit of work carefully since most of the time things should be transactional to ensure data integrity. If your method just reads data and does not change it, it can be safely non-transactional.

And UnitOfWorkDefaultOptions sets IsTransactional = true by default. https://github.com/aspnetboilerplate/aspnetboilerplate/blob/e0ded5d8702f389aa1f5947d3446f16aec845287/src/Abp/Domain/Uow/UnitOfWorkDefaultOptions.cs#L35

real-zony commented 4 years ago

Why non transactional uow doesn't work now without requiresNew: true? @hikalkan

hikalkan commented 4 years ago

If you begin a UOW and set transaction to false, it works. BUT.. if there is already an ongoing transaction in the outer scope, your transaction begin/complete calls are ignored. This is by design (to allow larger transactions to cover transactions of inner services).

The only way to explicitly declare (by requiresNew) that you want an independent transaction.

real-zony commented 4 years ago

@hikalkan OK, thank you for your answer.

gdlcf88 commented 4 years ago

If you begin a UOW and set transaction to false, it works. BUT.. if there is already an ongoing transaction in the outer scope, your transaction begin/complete calls are ignored. This is by design (to allow larger transactions to cover transactions of inner services).

The only way to explicitly declare (by requiresNew) that you want an independent transaction.

But in v0.19.0, I can disable appservice's uow and create a new uow manually without requiresNew: true

I test it in Abp vNext v0.19.0, cannot reproduce this problem.

gdlcf88 commented 4 years ago

Automatic uow is disabled by [UnitOfWork(IsDisabled = true)], where is the ongoing transaction from in latest abp version? @hikalkan

hikalkan commented 4 years ago

where is the ongoing transaction from in latest abp version

If your appservice method is called by a controller, then the controller will start the transaction.

gdlcf88 commented 4 years ago

Back to the topic, if controller provide a uow which I cannot disable in appservice by [UnitOfWork(IsDisabled = true)]:

        [UnitOfWork(IsDisabled = true)]
        public async Task Test()
        {
            using (var uow = _unitOfWorkManager.Begin())
            {
                var user = await _identityUserRepository.InsertAsync(
                    new IdentityUser(GuidGenerator.Create(), "testUser", "testUser@gmail.com", CurrentTenant.Id), true);
                // no commit
                throw new ApplicationException();
            }
        }

Why this entity can be inserted under the ongoing controller's uow?

gdlcf88 commented 4 years ago

It is the difference between v0.19.0 and v1.1.2, and the latter is puzzling. You can refuse to reply if you still think I'm worried too much. @hikalkan 😃

hikalkan commented 4 years ago

It is the difference between v0.19.0 and v1.1.2

There was some changes, but I am not sure if the change effected the behaviour. But the expected behaviour is just like I explained.

hikalkan commented 4 years ago

@gdlcf88 if your public async Task Test() is exposed as API controller and directly called by the browser, then the [UnitOfWork(IsDisabled = true)] will work. If it is injected by a Page/Controller it won't work.

So, if you are exposing it as API and calling from the client then your UOW will not be transactional because transaction is not enabled by default, so no problem.

Why transaction is not enabled by default?

Because it makes your application slow, use more memory, locks your database tables and so on.. So, transactions should be used only if necessary. For example, for GET requests it is not needed normally.

If you let ABP to handle it, ABP disables transaction for GET, enables for POST, PUT and others. If you always want to enabled it, set AbpUnitOfWorkDefaultOptions.TransactionBehavior (like Configure<AbpUnitOfWorkDefaultOptions>(opt => ...))

If you manually start the transaction, you need to decide it.

gdlcf88 commented 4 years ago

I got it wrong. Non transaction uow cannot roll back modified data.