diff --git a/framework/Volo.Abp.slnx b/framework/Volo.Abp.slnx index 5f4adbd622..6b9ad8af61 100644 --- a/framework/Volo.Abp.slnx +++ b/framework/Volo.Abp.slnx @@ -194,6 +194,7 @@ + diff --git a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUnitOfWorkOptions.cs b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUnitOfWorkOptions.cs index 3949bb6639..67bfbfaec5 100644 --- a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUnitOfWorkOptions.cs +++ b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUnitOfWorkOptions.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; namespace Volo.Abp.AspNetCore.Uow; @@ -11,4 +11,29 @@ public class AbpAspNetCoreUnitOfWorkOptions /// starting with an ignored URL. /// public List IgnoredUrls { get; } = new List(); + + /// + /// Completes the request unit of work just before the response starts (on + /// HttpResponse.OnStarting) instead of at the end of the pipeline, so data written during + /// the request is committed before the response is flushed. Disabled by default; enable it here + /// globally or opt-in per endpoint via . + /// + /// Trade-offs when it applies: an exception after the response starts can no longer roll back the + /// committed data (commit and network response are not atomic); database access after the response + /// starts is outside the request unit of work (unsuitable for streaming responses); unit of work + /// events and completed handlers run before the first response byte (adding to its latency); a + /// nested (requiresNew) unit of work that is current when the response starts is left to its owner + /// and the request unit of work then completes at the end of the pipeline as usual. + /// + /// + public bool CompleteUnitOfWorkOnResponseStarting { get; set; } = false; + + /// + /// Absolute request path prefixes (matched by segment) that opt-in to + /// even when it is globally disabled (for example + /// "/connect" matches "/connect/token" but not "/connections"). A trailing slash is normalized; blank, + /// non-absolute, and root ("/") entries are ignored - use + /// to enable it for every request. + /// + public List CompleteUnitOfWorkOnResponseStartingUrls { get; } = new List(); } diff --git a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs index 534d8f9828..36c00208f9 100644 --- a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs +++ b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs @@ -37,21 +37,25 @@ public class AbpUnitOfWorkMiddleware : AbpMiddlewareBase, ITransientDependency using (var uow = _unitOfWorkManager.Reserve(UnitOfWork.UnitOfWorkReservationName)) { - // Commit the ambient unit of work before the response starts, so data written - // during the request is committed before the response is flushed to the client. - // Only when this reserved unit of work is the current one: if an explicit nested - // unit of work is in progress, it is the current one and must be left to its owner. - context.Response.OnStarting(async () => + var completedOnResponseStarting = false; + + if (!context.Response.HasStarted && ShouldCompleteOnResponseStarting(context)) { - if (_unitOfWorkManager.Current == uow) + context.Response.OnStarting(async () => { - await uow.CompleteAsync(_cancellationTokenProvider.Token); - } - }); + // A nested (requiresNew) unit of work that is current is left to its owner. + if (_unitOfWorkManager.Current == uow) + { + // Set before completing so a post-commit failure isn't masked by the completion below. + completedOnResponseStarting = true; + await uow.CompleteAsync(_cancellationTokenProvider.Token); + } + }); + } await next(context); - if (!uow.IsCompleted) + if (!completedOnResponseStarting) { await uow.CompleteAsync(_cancellationTokenProvider.Token); } @@ -64,6 +68,32 @@ public class AbpUnitOfWorkMiddleware : AbpMiddlewareBase, ITransientDependency _options.IgnoredUrls.Any(x => context.Request.Path.Value.StartsWith(x, StringComparison.OrdinalIgnoreCase)); } + private bool ShouldCompleteOnResponseStarting(HttpContext context) + { + if (_options.CompleteUnitOfWorkOnResponseStarting) + { + return true; + } + + foreach (var url in _options.CompleteUnitOfWorkOnResponseStartingUrls) + { + if (string.IsNullOrWhiteSpace(url)) + { + continue; + } + + // Normalize a trailing slash ("/connect/" behaves like "/connect") and ignore non-absolute entries. + var prefix = url.TrimEnd('/'); + if (prefix.StartsWith("/", StringComparison.Ordinal) && + context.Request.Path.StartsWithSegments(prefix, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; + } + protected async override Task ShouldSkipAsync(HttpContext context, RequestDelegate next) { // Blazor components will render concurrently, so we need to skip the middleware for them. diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Tests.cs index 7953162f40..4e241cccf0 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Tests.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Tests.cs @@ -1,14 +1,19 @@ -using System.Net; +using System.Net; using System.Net.Http; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using Shouldly; +using Volo.Abp.AspNetCore.Uow; using Xunit; namespace Volo.Abp.AspNetCore.Mvc.Uow; public class UnitOfWorkMiddleware_Tests : AspNetCoreMvcTestBase { + private AbpAspNetCoreUnitOfWorkOptions Options => + ServiceProvider.GetRequiredService>().Value; + [Fact] public async Task Get_Actions_Should_Not_Be_Transactional() { @@ -31,17 +36,28 @@ public class UnitOfWorkMiddleware_Tests : AspNetCoreMvcTestBase } [Fact] - public async Task Ambient_Uow_Should_Be_Completed_Before_Response_Is_Flushed() + public async Task Ambient_Uow_Should_Be_Completed_Before_Response_Is_Flushed_When_Enabled() { + Options.CompleteUnitOfWorkOnResponseStarting = true; + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); result.ShouldBe("first:completed"); } [Fact] - public async Task Exception_After_Response_Flush_Should_Not_Undo_Committed_Work() + public async Task Ambient_Uow_Is_Not_Completed_On_Response_Start_By_Default() { - // Once the response has started, an exception can't turn it into an error response - // (the connection is reset). What matters: the uow was committed before the throw. + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); + result.ShouldBe("first:not-completed"); + } + + [Fact] + public async Task Ambient_Uow_Is_Already_Completed_When_An_Exception_Is_Raised_After_The_Response_Started() + { + Options.CompleteUnitOfWorkOnResponseStarting = true; + + // Once the response has started, an exception can't turn it into an error response (the + // connection is reset). Database-level rollback/commit is covered by the relational tests. await Should.ThrowAsync(async () => { var response = await Client.GetAsync("/api/unitofwork-test/CommitThenThrowAfterResponseFlush"); @@ -55,8 +71,8 @@ public class UnitOfWorkMiddleware_Tests : AspNetCoreMvcTestBase [Fact] public async Task Repository_Access_After_Response_Flush_Runs_Outside_The_Request_Uow() { - // After the response starts the request uow is gone; a repository still works via its - // own implicit uow (ambient=null), so it no longer joins the request transaction. + Options.CompleteUnitOfWorkOnResponseStarting = true; + var body = await GetResponseAsStringAsync("/api/unitofwork-test/ReadRepositoryAfterResponseFlush"); body.ShouldBe("before=ok(1);after=ok(1,ambient=null)"); } @@ -64,7 +80,8 @@ public class UnitOfWorkMiddleware_Tests : AspNetCoreMvcTestBase [Fact] public async Task Raw_Database_Provider_After_Response_Flush_Throws() { - // Unlike repositories, raw provider access after the response started has no uow and throws. + Options.CompleteUnitOfWorkOnResponseStarting = true; + var body = await GetResponseAsStringAsync("/api/unitofwork-test/RawDatabaseProviderAfterResponseFlush"); body.ShouldBe("first:threw-AbpException"); } @@ -72,7 +89,54 @@ public class UnitOfWorkMiddleware_Tests : AspNetCoreMvcTestBase [Fact] public async Task Response_Flush_Inside_Nested_Uow_Should_Not_Complete_The_Nested_Uow() { + Options.CompleteUnitOfWorkOnResponseStarting = true; + var body = await GetResponseAsStringAsync("/api/unitofwork-test/NestedUowDuringResponseFlush"); - body.ShouldBe("first:nested-completed-by-owner"); + body.ShouldBe("first:outer-not-completed:nested-completed-by-owner"); + } + + [Fact] + public async Task Completing_The_Uow_In_The_Action_Still_Fails_At_End_Of_Pipeline_By_Default() + { + var response = await Client.GetAsync("/api/unitofwork-test/CompleteCurrentUow"); + response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + } + + [Fact] + public async Task Opt_In_Url_Enables_The_Feature_For_A_Matching_Path() + { + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add("/api/unitofwork-test/CommitBeforeResponseFlush"); + + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); + result.ShouldBe("first:completed"); + } + + [Fact] + public async Task Opt_In_Url_With_A_Trailing_Slash_Still_Matches() + { + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add("/api/unitofwork-test/"); + + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); + result.ShouldBe("first:completed"); + } + + [Fact] + public async Task Opt_In_Url_With_A_Non_Segment_Prefix_Should_Not_Match() + { + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add("/api/unitofwork-test/Commit"); + + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); + result.ShouldBe("first:not-completed"); + } + + [Fact] + public async Task Blank_Or_Malformed_Opt_In_Urls_Are_Ignored() + { + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add(""); + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add(" "); + Options.CompleteUnitOfWorkOnResponseStartingUrls.Add("api/no-leading-slash"); + + var result = await GetResponseAsStringAsync("/api/unitofwork-test/CommitBeforeResponseFlush"); + result.ShouldBe("first:not-completed"); } } diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs index cdb7c80917..435a44d9e8 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs @@ -97,7 +97,6 @@ public class UnitOfWorkTestController : AbpController await Response.WriteAsync("first"); await Response.Body.FlushAsync(); - // Record the commit state so the test can assert the throw below doesn't undo it. _testUnitOfWorkConfig.UowCompletedAfterResponseFlush = uow.IsCompleted; throw new UserFriendlyException("boom after the response was already flushed"); @@ -161,6 +160,10 @@ public class UnitOfWorkTestController : AbpController await Response.WriteAsync("first"); await Response.Body.FlushAsync(); + // The outer request unit of work (nested.Outer) must not have been completed on response + // start while a nested unit of work is current. + await Response.WriteAsync(nested.Outer!.IsCompleted ? ":outer-completed" : ":outer-not-completed"); + string outcome; try { @@ -175,4 +178,13 @@ public class UnitOfWorkTestController : AbpController await Response.WriteAsync(outcome); } } + + [HttpGet] + [Route("CompleteCurrentUow")] + public async Task CompleteCurrentUow() + { + // Complete the request unit of work inside the action, without writing the response yet. + // The middleware must still try to complete it at the end of the pipeline (original behavior). + await CurrentUnitOfWork.CompleteAsync(); + } } diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo.Abp.AspNetCore.Uow.Tests.csproj b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo.Abp.AspNetCore.Uow.Tests.csproj new file mode 100644 index 0000000000..08b17d2002 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo.Abp.AspNetCore.Uow.Tests.csproj @@ -0,0 +1,26 @@ + + + + + + net10.0 + Volo.Abp.AspNetCore.Uow.Tests + Volo.Abp.AspNetCore.Uow.Tests + true + false + false + false + true + + + + + + + + + + + + + diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUowTestModule.cs b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUowTestModule.cs new file mode 100644 index 0000000000..df2256258f --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/AbpAspNetCoreUowTestModule.cs @@ -0,0 +1,61 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Volo.Abp.AspNetCore.Mvc; +using Volo.Abp.AspNetCore.TestBase; +using Volo.Abp.Autofac; +using Volo.Abp.Data; +using Volo.Abp.EntityFrameworkCore; +using Volo.Abp.EntityFrameworkCore.Sqlite; +using Volo.Abp.Modularity; + +namespace Volo.Abp.AspNetCore.Uow; + +[DependsOn( + typeof(AbpAspNetCoreTestBaseModule), + typeof(AbpAspNetCoreMvcModule), + typeof(AbpEntityFrameworkCoreSqliteModule), + typeof(AbpAutofacModule) + )] +public class AbpAspNetCoreUowTestModule : AbpModule +{ + private readonly AbpUnitTestSqliteDatabase _database = new AbpUnitTestSqliteDatabase(); + + public override void ConfigureServices(ServiceConfigurationContext context) + { + context.Services.AddAbpDbContext(options => + { + options.AddDefaultRepositories(includeAllEntities: true); + }); + + Configure(options => + { + options.ConnectionStrings.Default = _database.ConnectionString; + }); + + Configure(options => + { + options.Configure(dbContext => dbContext.UseSqlite().AddAbpDbContextOptionsExtension()); + }); + + _database.CreateTables(new UowVisibilityTestDbContext( + new DbContextOptionsBuilder() + .UseSqlite(_database.ConnectionString) + .AddAbpDbContextOptionsExtension() + .Options)); + } + + public override void OnApplicationInitialization(ApplicationInitializationContext context) + { + var app = context.GetApplicationBuilder(); + + app.UseRouting(); + app.UseUnitOfWork(); + app.UseConfiguredEndpoints(); + } + + public override void OnApplicationShutdown(ApplicationShutdownContext context) + { + _database.Dispose(); + } +} diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/Program.cs b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/Program.cs new file mode 100644 index 0000000000..98ab220169 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/Program.cs @@ -0,0 +1,15 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Hosting; +using Volo.Abp.AspNetCore.TestBase; +using Volo.Abp.AspNetCore.Uow; + +var builder = WebApplication.CreateBuilder(new WebApplicationOptions +{ + EnvironmentName = Environments.Staging +}); + +await builder.RunAbpModuleAsync(); + +public partial class Program +{ +} diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UnitOfWorkMiddleware_Relational_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UnitOfWorkMiddleware_Relational_Tests.cs new file mode 100644 index 0000000000..00d4cdd2ec --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UnitOfWorkMiddleware_Relational_Tests.cs @@ -0,0 +1,120 @@ +using System; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Shouldly; +using Volo.Abp.AspNetCore.TestBase; +using Xunit; + +namespace Volo.Abp.AspNetCore.Uow; + +public class UnitOfWorkMiddleware_Relational_Tests : AbpWebApplicationFactoryIntegratedTest +{ + private void EnableCompleteOnResponseStarting() + { + ServiceProvider.GetRequiredService>() + .Value.CompleteUnitOfWorkOnResponseStarting = true; + } + + private async Task CountAsync(string name) + { + var response = await Client.GetAsync("/api/uow-visibility/count?name=" + name); + response.EnsureSuccessStatusCode(); + return int.Parse(await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task Row_Written_During_Request_Is_Visible_From_An_Independent_Connection_On_Response_Start() + { + EnableCompleteOnResponseStarting(); + + var response = await Client.GetAsync("/api/uow-visibility/insert-then-read"); + response.EnsureSuccessStatusCode(); + (await response.Content.ReadAsStringAsync()).ShouldBe("inserted:visible"); + } + + [Fact] + public async Task Row_Written_During_Request_Is_Still_Committed_When_The_Feature_Is_Disabled() + { + var name = Guid.NewGuid().ToString("N"); + + var insert = await Client.GetAsync("/api/uow-visibility/insert?name=" + name); + insert.EnsureSuccessStatusCode(); + + (await CountAsync(name)).ShouldBe(1); + } + + [Fact] + public async Task Exception_Before_Response_Rolls_Back_The_Written_Row() + { + EnableCompleteOnResponseStarting(); + var name = Guid.NewGuid().ToString("N"); + + var insert = await Client.GetAsync("/api/uow-visibility/insert-then-throw?name=" + name); + insert.IsSuccessStatusCode.ShouldBeFalse(); + + (await CountAsync(name)).ShouldBe(0); + } + + [Fact] + public async Task Committed_Row_Survives_An_Exception_Raised_After_The_Response_Started() + { + EnableCompleteOnResponseStarting(); + var name = Guid.NewGuid().ToString("N"); + + await Should.ThrowAsync(async () => + { + var response = await Client.GetAsync("/api/uow-visibility/insert-flush-then-throw?name=" + name); + await response.Content.ReadAsStringAsync(); + }); + + (await CountAsync(name)).ShouldBe(1); + } + [Fact] + public async Task Committed_Row_Survives_A_Completed_Handler_Failing_On_Response_Start() + { + EnableCompleteOnResponseStarting(); + var name = Guid.NewGuid().ToString("N"); + + // The handler's error must surface as-is (not masked by a second "already requested" completion); + // the row is committed regardless, since the handler runs after commit. + Exception surfaced = null; + try + { + var response = await Client.GetAsync("/api/uow-visibility/insert-flush-throwing-completed-handler?name=" + name); + await response.Content.ReadAsStringAsync(); + } + catch (Exception ex) + { + surfaced = ex; + } + + surfaced.ShouldNotBeNull(); + surfaced.ToString().ShouldContain("boom in a completed handler"); + surfaced.ToString().ShouldNotContain("already"); + + (await CountAsync(name)).ShouldBe(1); + } + [Fact] + public async Task A_Failing_Commit_On_Response_Start_Does_Not_Persist_Data() + { + EnableCompleteOnResponseStarting(); + var name = Guid.NewGuid().ToString("N"); + + Exception surfaced = null; + try + { + var response = await Client.GetAsync("/api/uow-visibility/insert-then-fail-commit?name=" + name); + await response.Content.ReadAsStringAsync(); + } + catch (Exception ex) + { + surfaced = ex; + } + + // The commit fails on response start, so the request must surface an error and persist nothing. + surfaced.ShouldNotBeNull(); + (await CountAsync(name)).ShouldBe(0); + } +} diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityController.cs b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityController.cs new file mode 100644 index 0000000000..83d30d8410 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityController.cs @@ -0,0 +1,107 @@ +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Volo.Abp; +using Volo.Abp.AspNetCore.Mvc; +using Volo.Abp.Domain.Repositories; +using Volo.Abp.Uow; + +namespace Volo.Abp.AspNetCore.Uow; + +[Route("api/uow-visibility")] +public class UowVisibilityController : AbpController +{ + private readonly IRepository _repository; + + public UowVisibilityController(IRepository repository) + { + _repository = repository; + } + + [HttpGet] + [Route("insert-then-read")] + [UnitOfWork(isTransactional: true)] + public async Task InsertThenRead() + { + var name = Guid.NewGuid().ToString("N"); + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name)); + + await Response.WriteAsync("inserted"); + await Response.Body.FlushAsync(); + + int count; + using (var uow = UnitOfWorkManager.Begin(requiresNew: true, isTransactional: false)) + { + count = await _repository.CountAsync(x => x.Name == name); + await uow.CompleteAsync(); + } + + await Response.WriteAsync(count == 1 ? ":visible" : ":not-visible"); + } + + [HttpGet] + [Route("insert")] + [UnitOfWork(isTransactional: true)] + public async Task Insert(string name) + { + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name)); + await Response.WriteAsync("inserted"); + } + + [HttpGet] + [Route("count")] + public async Task Count(string name) + { + var count = await _repository.CountAsync(x => x.Name == name); + await Response.WriteAsync(count.ToString()); + } + + // Insert (autoSave sends the INSERT to the transaction) then throw before the response: must roll back. + [HttpGet] + [Route("insert-then-throw")] + [UnitOfWork(isTransactional: true)] + public async Task InsertThenThrow(string name) + { + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name), autoSave: true); + throw new AbpException("boom before the response started"); + } + + // Insert, flush the response (committed here when enabled), then throw: the committed row survives. + [HttpGet] + [Route("insert-flush-then-throw")] + [UnitOfWork(isTransactional: true)] + public async Task InsertFlushThenThrow(string name) + { + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name)); + + await Response.WriteAsync("inserted"); + await Response.Body.FlushAsync(); + + throw new AbpException("boom after the response started"); + } + // Insert, register a completed handler that throws (runs after commit), then flush. + [HttpGet] + [Route("insert-flush-throwing-completed-handler")] + [UnitOfWork(isTransactional: true)] + public async Task InsertFlushWithThrowingCompletedHandler(string name) + { + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name)); + CurrentUnitOfWork.OnCompleted(() => throw new AbpException("boom in a completed handler")); + + await Response.WriteAsync("inserted"); + await Response.Body.FlushAsync(); + } + // Insert a valid row plus an invalid one (Name is required): the commit at response start fails. + [HttpGet] + [Route("insert-then-fail-commit")] + [UnitOfWork(isTransactional: true)] + public async Task InsertThenFailCommit(string name) + { + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), name)); + await _repository.InsertAsync(new UowVisibilityTestEntity(Guid.NewGuid(), null)); + + await Response.WriteAsync("inserted"); + await Response.Body.FlushAsync(); + } +} diff --git a/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityTestEntity.cs b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityTestEntity.cs new file mode 100644 index 0000000000..f3595ddd33 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Uow.Tests/Volo/Abp/AspNetCore/Uow/UowVisibilityTestEntity.cs @@ -0,0 +1,46 @@ +using System; +using Microsoft.EntityFrameworkCore; +using Volo.Abp.Data; +using Volo.Abp.Domain.Entities; +using Volo.Abp.EntityFrameworkCore; +using Volo.Abp.EntityFrameworkCore.Modeling; + +namespace Volo.Abp.AspNetCore.Uow; + +public class UowVisibilityTestEntity : AggregateRoot +{ + public string Name { get; set; } + + protected UowVisibilityTestEntity() + { + } + + public UowVisibilityTestEntity(Guid id, string name) + : base(id) + { + Name = name; + } +} + +[ConnectionStringName("Default")] +public class UowVisibilityTestDbContext : AbpDbContext +{ + public DbSet UowVisibilityTestEntities { get; set; } + + public UowVisibilityTestDbContext(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + base.OnModelCreating(modelBuilder); + + modelBuilder.Entity(b => + { + b.ToTable("UowVisibilityTestEntities"); + b.ConfigureByConvention(); + b.Property(x => x.Name).IsRequired(); + }); + } +} diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs index 26729669e7..0826f7a3ae 100644 --- a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs +++ b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs @@ -1,10 +1,14 @@ -using Microsoft.AspNetCore.Identity; +using System.Collections.Generic; +using System.Linq; +using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using OpenIddict.Abstractions; using OpenIddict.Server; using Volo.Abp.AspNetCore.MultiTenancy; using Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared; +using Volo.Abp.AspNetCore.Uow; using Volo.Abp.Modularity; using Volo.Abp.OpenIddict.Globalization; using Volo.Abp.OpenIddict.Scopes; @@ -45,6 +49,40 @@ public class AbpOpenIddictAspNetCoreModule : AbpModule { options.RemoveClientIdClaim(); }); + + // Commit tokens/authorizations/sessions written during sign-in before the response is flushed. + // Derived from the configured OpenIddict server endpoint paths (including the device endpoint). + context.Services.AddOptions() + .Configure>((uowOptions, serverOptions) => + { + foreach (var path in GetServerEndpointPaths(serverOptions.Value)) + { + if (!uowOptions.CompleteUnitOfWorkOnResponseStartingUrls.Contains(path)) + { + uowOptions.CompleteUnitOfWorkOnResponseStartingUrls.Add(path); + } + } + }); + } + + private static IEnumerable GetServerEndpointPaths(OpenIddictServerOptions serverOptions) + { + var endpoints = serverOptions.TokenEndpointUris + .Concat(serverOptions.AuthorizationEndpointUris) + .Concat(serverOptions.DeviceAuthorizationEndpointUris) + .Concat(serverOptions.PushedAuthorizationEndpointUris) + .Concat(serverOptions.EndSessionEndpointUris) + .Concat(serverOptions.RevocationEndpointUris) + .Concat(serverOptions.EndUserVerificationEndpointUris); + + foreach (var uri in endpoints) + { + var path = uri.IsAbsoluteUri ? uri.AbsolutePath : uri.OriginalString; + if (!string.IsNullOrWhiteSpace(path)) + { + yield return "/" + path.TrimStart('/'); + } + } } private void AddOpenIddictServer(IServiceCollection services) diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj index f4c6815fd6..003b78894c 100644 --- a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj @@ -5,6 +5,8 @@ net10.0 + true + true @@ -16,6 +18,10 @@ + + + + diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenEndpoint_Integration_Tests.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenEndpoint_Integration_Tests.cs new file mode 100644 index 0000000000..cf11cffdf3 --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenEndpoint_Integration_Tests.cs @@ -0,0 +1,58 @@ +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Shouldly; +using Volo.Abp.AspNetCore.TestBase; +using Volo.Abp.AspNetCore.Uow; +using Xunit; + +namespace Volo.Abp.OpenIddict.Integration; + +// A real "/connect/token" (client_credentials) request through the OpenIddict server. A probe registered +// outside UseUnitOfWork reads the token count from an independent connection at response start. +public class OpenIddictTokenEndpoint_Integration_Tests : AbpWebApplicationFactoryIntegratedTest +{ + private AbpAspNetCoreUnitOfWorkOptions Options => + ServiceProvider.GetRequiredService>().Value; + + private long? TokenCountAtResponseStart => + ServiceProvider.GetRequiredService().TokenCountAtResponseStart; + + private Task RequestTokenAsync() + { + return Client.PostAsync("/connect/token", new FormUrlEncodedContent(new Dictionary + { + ["grant_type"] = "client_credentials", + ["client_id"] = "test-client", + ["client_secret"] = "test-secret" + })); + } + + [Fact] + public async Task Token_Row_Is_Committed_Before_The_Connect_Token_Response_Is_Sent() + { + // The OpenIddict module opts "/connect" in by default. + var response = await RequestTokenAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).ShouldContain("access_token"); + TokenCountAtResponseStart.ShouldBe(1); + } + + [Fact] + public async Task Without_The_Opt_In_The_Token_Is_Not_Committed_When_The_Response_Starts() + { + // Negative control: without the opt-in the token is committed only at the end of the pipeline, + // so the probe reads 0. This proves the positive case genuinely observes response-start timing. + Options.CompleteUnitOfWorkOnResponseStartingUrls.Clear(); + Options.CompleteUnitOfWorkOnResponseStarting = false; + + var response = await RequestTokenAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + TokenCountAtResponseStart.ShouldBe(0); + } +} diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenIntegrationTestModule.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenIntegrationTestModule.cs new file mode 100644 index 0000000000..1319045ca6 --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/OpenIddictTokenIntegrationTestModule.cs @@ -0,0 +1,150 @@ +using System; +using System.IO; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Mvc.ApplicationParts; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using OpenIddict.Abstractions; +using OpenIddict.Server; +using Volo.Abp.AspNetCore.TestBase; +using Volo.Abp.AspNetCore.Uow; +using Volo.Abp.Data; +using Volo.Abp.Domain.Repositories; +using Volo.Abp.EntityFrameworkCore; +using Volo.Abp.EntityFrameworkCore.Sqlite; +using Volo.Abp.Modularity; +using Volo.Abp.OpenIddict.Applications; +using Volo.Abp.OpenIddict.EntityFrameworkCore; +using Volo.Abp.OpenIddict.Tokens; +using Volo.Abp.Uow; +using Volo.Abp.Autofac; + +namespace Volo.Abp.OpenIddict.Integration; + +public class TokenVisibilityRecorder +{ + public long? TokenCountAtResponseStart { get; set; } +} + +[DependsOn( + typeof(AbpAspNetCoreTestBaseModule), + typeof(AbpOpenIddictAspNetCoreModule), + typeof(AbpOpenIddictEntityFrameworkCoreModule), + typeof(AbpEntityFrameworkCoreSqliteModule), + typeof(AbpAutofacModule) + )] +public class OpenIddictTokenIntegrationTestModule : AbpModule +{ + // File-based SQLite (not shared-cache in-memory) so an independent connection can read committed + // state while another holds an open write transaction, without the shared-cache single-writer deadlock. + private readonly string _databasePath = Path.Combine(Path.GetTempPath(), $"abp-oidc-uow-{Guid.NewGuid():N}.db"); + private string ConnectionString => $"Data Source={_databasePath};Pooling=False"; + + public override void PreConfigureServices(ServiceConfigurationContext context) + { + PreConfigure(options => + { + options.AddDevelopmentEncryptionAndSigningCertificate = false; + }); + + PreConfigure(builder => + { + builder.AddEphemeralEncryptionKey(); + builder.AddEphemeralSigningKey(); + builder.UseAspNetCore().DisableTransportSecurityRequirement(); + }); + } + + public override void ConfigureServices(ServiceConfigurationContext context) + { + context.Services.AddSingleton(); + + // The OpenIddict controllers (including the token endpoint) live in a referenced assembly. + context.Services.GetSingletonInstance() + .ApplicationParts.AddIfNotContains(typeof(AbpOpenIddictAspNetCoreModule).Assembly); + + using (var dbContext = new OpenIddictDbContext( + new DbContextOptionsBuilder().UseSqlite(ConnectionString).Options)) + { + dbContext.Database.EnsureCreated(); + } + + Configure(options => + { + options.ConnectionStrings.Default = ConnectionString; + }); + + Configure(options => + { + options.Configure(c => c.UseSqlite()); + }); + } + + public override void OnApplicationInitialization(ApplicationInitializationContext context) + { + SeedClientAsync(context.ServiceProvider).GetAwaiter().GetResult(); + + var app = context.GetApplicationBuilder(); + app.UseRouting(); + + // Registered before UseUnitOfWork so its OnStarting runs after the unit of work commits + // (callbacks run in reverse order): reads the token count from an independent connection. + app.Use(async (ctx, next) => + { + if (ctx.Request.Path.StartsWithSegments("/connect/token")) + { + ctx.Response.OnStarting(async () => + { + var recorder = ctx.RequestServices.GetRequiredService(); + var uowManager = ctx.RequestServices.GetRequiredService(); + using var uow = uowManager.Begin(requiresNew: true, isTransactional: false); + var repository = ctx.RequestServices.GetRequiredService>(); + recorder.TokenCountAtResponseStart = await repository.GetCountAsync(); + await uow.CompleteAsync(); + }); + } + + await next(); + }); + + app.UseAuthentication(); + app.UseUnitOfWork(); + app.UseAuthorization(); + app.UseConfiguredEndpoints(); + } + + public override void OnApplicationShutdown(ApplicationShutdownContext context) + { + if (File.Exists(_databasePath)) + { + File.Delete(_databasePath); + } + } + + private static async Task SeedClientAsync(IServiceProvider serviceProvider) + { + using var scope = serviceProvider.CreateScope(); + var uowManager = scope.ServiceProvider.GetRequiredService(); + using var uow = uowManager.Begin(); + + var applicationManager = scope.ServiceProvider.GetRequiredService(); + if (await applicationManager.FindByClientIdAsync("test-client") == null) + { + await applicationManager.CreateAsync(new AbpApplicationDescriptor + { + ClientId = "test-client", + ClientSecret = "test-secret", + DisplayName = "Test Client", + ClientType = OpenIddictConstants.ClientTypes.Confidential, + Permissions = + { + OpenIddictConstants.Permissions.Endpoints.Token, + OpenIddictConstants.Permissions.GrantTypes.ClientCredentials + } + }); + } + + await uow.CompleteAsync(); + } +} diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/Program.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/Program.cs new file mode 100644 index 0000000000..a2ac64130a --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/Integration/Program.cs @@ -0,0 +1,15 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Hosting; +using Volo.Abp.AspNetCore.TestBase; +using Volo.Abp.OpenIddict.Integration; + +var builder = WebApplication.CreateBuilder(new WebApplicationOptions +{ + EnvironmentName = Environments.Staging +}); + +await builder.RunAbpModuleAsync(); + +public partial class Program +{ +}