diff --git a/CoreSync b/CoreSync index 9902bd4..9894eb1 160000 --- a/CoreSync +++ b/CoreSync @@ -1 +1 @@ -Subproject commit 9902bd4f860bcf2eb0462d384e475aedeaa64d1e +Subproject commit 9894eb1c35e531af1dcc787b4b584109bd9bafc9 diff --git a/src/Agent/CoreSyncServer.Agent.csproj b/src/Agent/CoreSyncServer.Agent.csproj index 5c5ca25..634233f 100644 --- a/src/Agent/CoreSyncServer.Agent.csproj +++ b/src/Agent/CoreSyncServer.Agent.csproj @@ -11,9 +11,9 @@ - - - + + + diff --git a/src/Data/CoreSyncServer.Data.csproj b/src/Data/CoreSyncServer.Data.csproj index 507ea7c..7388c97 100644 --- a/src/Data/CoreSyncServer.Data.csproj +++ b/src/Data/CoreSyncServer.Data.csproj @@ -8,8 +8,8 @@ - - + + diff --git a/src/Server/CoreSyncServer.csproj b/src/Server/CoreSyncServer.csproj index 9e293d1..3b96532 100644 --- a/src/Server/CoreSyncServer.csproj +++ b/src/Server/CoreSyncServer.csproj @@ -18,10 +18,10 @@ - - + + - + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/src/Services/AgentOfflineException.cs b/src/Services/AgentOfflineException.cs index 6cc0a00..c163d42 100644 --- a/src/Services/AgentOfflineException.cs +++ b/src/Services/AgentOfflineException.cs @@ -1,8 +1,11 @@ -namespace CoreSyncServer.Services; +namespace CoreSyncServer.Services; /// /// Thrown by AgentProxySyncProvider when no agent connection is registered for the target DataStore, -/// or the RPC call to the connected agent fails. SyncController maps this to HTTP 503. +/// or the RPC call to the connected agent does not complete (timeout, dropped connection). +/// SyncController maps this to HTTP 503 with the agent-unavailable code. +/// An agent that answers with an error raises the derived , +/// mapped to the same 503 with the agent-error code. /// public class AgentOfflineException : Exception { diff --git a/src/Services/AgentOperationFailedException.cs b/src/Services/AgentOperationFailedException.cs new file mode 100644 index 0000000..0b2d851 --- /dev/null +++ b/src/Services/AgentOperationFailedException.cs @@ -0,0 +1,24 @@ +namespace CoreSyncServer.Services; + +/// +/// Thrown by AgentProxySyncProvider when the agent was reached, executed the requested +/// operation and reported that it failed - typically its sync provider rejecting the changes against +/// the on-premise database. +/// +/// +/// Derives from and carries the very message that type carried +/// for this case, so SyncController keeps answering HTTP 503 with the same body and logging +/// it the same way: a server deployed ahead of its clients must look identical to them. What it adds +/// is the distinction itself, which the controller turns into the agent-error code header so +/// that a current client no longer reads an agent-side failure as "agent offline". +/// +public class AgentOperationFailedException : AgentOfflineException +{ + public string Operation { get; } + + public AgentOperationFailedException(int dataStoreId, string operation, Exception inner) + : base(dataStoreId, $"Agent RPC '{operation}' failed for DataStore {dataStoreId}.", inner) + { + Operation = operation; + } +} diff --git a/src/Services/Controllers/SyncController.cs b/src/Services/Controllers/SyncController.cs index b3ce896..fe43400 100644 --- a/src/Services/Controllers/SyncController.cs +++ b/src/Services/Controllers/SyncController.cs @@ -16,11 +16,21 @@ namespace CoreSyncServer.Controllers; +/// +/// The CoreSync HTTP wire protocol, served per published endpoint. +/// +/// +/// Failures the controller recognises are answered through : the same +/// status code as before, plus the X-CoreSync-Error / X-CoreSync-Trace headers and a +/// body a current client turns into a typed failure. Anything else +/// propagates to the host (exception handler, Sentry) as it always did, tagged with the same headers +/// by . No status code differs from before the headers existed. +/// [ApiController] [AllowAnonymous] [Route("api/sync/{endpointId:guid}")] [ServiceFilter(typeof(SyncEndpointAuthFilter))] -[ServiceFilter(typeof(SyncConfigurationExceptionFilter))] +[ServiceFilter(typeof(SyncErrorResponseFilter))] public class SyncController( ApplicationDbContext context, ISyncProviderFactory syncProviderFactory, @@ -132,7 +142,7 @@ public async Task> GetStoreId(Guid endpointId, Cancellation } catch (KeyNotFoundException) { - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { @@ -151,7 +161,7 @@ public async Task> GetSyncVersion(Guid endpointId, Can } catch (KeyNotFoundException) { - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { @@ -159,10 +169,29 @@ public async Task> GetSyncVersion(Guid endpointId, Can } } + /// + /// 404 with the endpoint-not-found code: the status an unknown endpoint always produced, + /// now distinguishable from an expired session, which is also a 404. + /// + private ObjectResult EndpointNotFound(Guid endpointId) + => SyncErrorResponses.EndpointNotFound(HttpContext, endpointId); + + /// + /// 404 with the session-expired code: the status an unknown upload/download session always + /// produced. The client is told to start a new synchronization rather than retry the request. + /// + private ObjectResult SessionExpired(Guid sessionId) + => SyncErrorResponses.SessionExpired(HttpContext, sessionId); + + /// + /// The 503 every agent failure has always produced, logged as it always was: agent-unavailable + /// when the agent owning the data store is not connected or did not answer in time, + /// agent-error when it answered that the operation failed. Both are left retryable. + /// private ObjectResult AgentOffline(AgentOfflineException ex) { logger.LogWarning(ex, "Agent offline for DataStore {DataStoreId}", ex.DataStoreId); - return StatusCode(StatusCodes.Status503ServiceUnavailable, ex.Message); + return SyncErrorResponses.AgentFailure(HttpContext, ex); } [HttpGet("changes-bulk/{storeId:guid}")] @@ -215,7 +244,7 @@ public async Task> GetBulkChangeSet(Guid endpoin } catch (KeyNotFoundException) { - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { @@ -242,7 +271,7 @@ public ActionResult> GetBulkChangeSetItem(Guid sessionId return Ok(bufferList); } - return NotFound(); + return SessionExpired(sessionId); } [HttpGet("changes-bulk-item-binary/{sessionId:guid}/{skip:int}/{take:int}")] @@ -265,7 +294,7 @@ public ActionResult GetBulkChangeSetItemBinary(Guid sessionId, int skip, int tak return File(bytes, "application/x-msgpack"); } - return NotFound(); + return SessionExpired(sessionId); } [HttpPost("changes-bulk-begin")] @@ -288,7 +317,7 @@ public async Task BeginApplyBulkChanges(Guid endpointId, [FromBody } catch (KeyNotFoundException) { - return NotFound(); + return EndpointNotFound(endpointId); } } @@ -301,7 +330,7 @@ public ActionResult ApplyBulkChangesItem([FromBody] BulkChangeSetUploadItem bulk return Ok(); } - return NotFound(); + return SessionExpired(bulkUploadItem.SessionId); } [HttpPost("changes-bulk-item-binary")] @@ -319,7 +348,7 @@ public async Task ApplyBulkChangesItemBinary() return Ok(); } - return NotFound(); + return SessionExpired(bulkUploadItem.SessionId); } [HttpPost("changes-bulk-complete/{sessionId:guid}")] @@ -360,7 +389,7 @@ public async Task> CompleteApplyBulkChanges(Guid endpoi // Cleanup on CancellationToken.None — see GetBulkChangeSet for the rationale. await sessionLogger.FlushAsync(CancellationToken.None); await syncSessionService.ErrorAsync(cached.SyncSessionId, $"Endpoint '{endpointId}' not found.", CancellationToken.None); - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { @@ -376,7 +405,7 @@ public async Task> CompleteApplyBulkChanges(Guid endpoi } } - return NotFound(); + return SessionExpired(sessionId); } [HttpPost("changes-bulk-complete-binary/{sessionId:guid}")] @@ -406,7 +435,7 @@ public async Task> CompleteApplyBulkChangesBinary(Guid // Cleanup on CancellationToken.None — see GetBulkChangeSet for the rationale. await sessionLogger.FlushAsync(CancellationToken.None); await syncSessionService.ErrorAsync(cached.SyncSessionId, $"Endpoint '{endpointId}' not found.", CancellationToken.None); - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { @@ -422,7 +451,7 @@ public async Task> CompleteApplyBulkChangesBinary(Guid } } - return NotFound(); + return SessionExpired(sessionId); } [HttpPost("save-version/{storeId:guid}/{version:long}")] @@ -439,7 +468,7 @@ public async Task SaveVersionForStore(Guid endpointId, Guid storeI } catch (KeyNotFoundException) { - return NotFound(); + return EndpointNotFound(endpointId); } catch (AgentOfflineException ex) { diff --git a/src/Services/CoreSyncServer.Services.csproj b/src/Services/CoreSyncServer.Services.csproj index 04f0e31..a2123ba 100644 --- a/src/Services/CoreSyncServer.Services.csproj +++ b/src/Services/CoreSyncServer.Services.csproj @@ -12,9 +12,9 @@ - - - + + + diff --git a/src/Services/Filters/SyncConfigurationExceptionFilter.cs b/src/Services/Filters/SyncConfigurationExceptionFilter.cs index ff608d3..e037c11 100644 --- a/src/Services/Filters/SyncConfigurationExceptionFilter.cs +++ b/src/Services/Filters/SyncConfigurationExceptionFilter.cs @@ -11,6 +11,12 @@ namespace CoreSyncServer.Filters; /// 422 Unprocessable Entity response carrying the provider's message, instead of letting them /// surface to the sync client as an opaque HTTP 500. /// +/// +/// Superseded by , which answers the same 422 with the +/// configuration-error code and a body and also +/// tags every other failure. Kept registered so hosts that reference it keep compiling. +/// +[Obsolete("Use SyncErrorResponseFilter, which covers this case and every other sync failure.")] public class SyncConfigurationExceptionFilter(ILogger logger) : IExceptionFilter { public void OnException(ExceptionContext context) diff --git a/src/Services/Filters/SyncEndpointAuthFilter.cs b/src/Services/Filters/SyncEndpointAuthFilter.cs index 1f96947..845ecda 100644 --- a/src/Services/Filters/SyncEndpointAuthFilter.cs +++ b/src/Services/Filters/SyncEndpointAuthFilter.cs @@ -29,15 +29,11 @@ public async Task OnActionExecutionAsync(ActionExecutingContext filterContext, A .Include(e => e.Authentication) .FirstOrDefaultAsync(e => e.Id == endpointId, filterContext.HttpContext.RequestAborted); - if (endpoint is null) + // Same 404 as before, now carrying the endpoint-not-found code and a body: without a body the + // host's status code pages would replace it with the HTML not-found page. + if (endpoint is null || !endpoint.IsPublished) { - filterContext.Result = new NotFoundResult(); - return; - } - - if (!endpoint.IsPublished) - { - filterContext.Result = new NotFoundResult(); + filterContext.Result = SyncErrorResponses.EndpointNotFound(filterContext.HttpContext, endpointId); return; } diff --git a/src/Services/Filters/SyncErrorResponseFilter.cs b/src/Services/Filters/SyncErrorResponseFilter.cs new file mode 100644 index 0000000..6463256 --- /dev/null +++ b/src/Services/Filters/SyncErrorResponseFilter.cs @@ -0,0 +1,48 @@ +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.Extensions.Logging; + +namespace CoreSyncServer.Filters; + +/// +/// Makes every failure escaping SyncController classifiable by the client. +/// +/// +/// +/// +/// - a store configured in a way the provider cannot synchronize +/// (composite primary key, reserved column name...) - becomes 422 Unprocessable Entity with the +/// configuration-error code and a body carrying the +/// provider's message, the status this API has always returned for it. +/// +/// +/// Everything else is left unhandled, so the host's exception handler, logging and error reporting +/// (Sentry) see it exactly as before; only the error code and trace headers are attached to the host's +/// response (see ). +/// +/// +/// +public class SyncErrorResponseFilter(ILogger logger) : IExceptionFilter +{ + public void OnException(ExceptionContext context) + { + if (context.ExceptionHandled) + return; + + if (context.Exception is NotSupportedException notSupported) + { + logger.LogWarning(notSupported, "Unsupported sync configuration: {Message}", notSupported.Message); + + context.Result = SyncErrorResponses.ConfigurationError(context.HttpContext, notSupported.Message); + context.ExceptionHandled = true; + return; + } + + if (context.Exception is OperationCanceledException && context.HttpContext.RequestAborted.IsCancellationRequested) + { + // The client went away; there is no response to tag. + return; + } + + SyncErrorResponses.TagUnhandled(context.HttpContext, context.Exception); + } +} diff --git a/src/Services/Filters/SyncErrorResponses.cs b/src/Services/Filters/SyncErrorResponses.cs new file mode 100644 index 0000000..f8941f7 --- /dev/null +++ b/src/Services/Filters/SyncErrorResponses.cs @@ -0,0 +1,119 @@ +using CoreSync; +using CoreSync.Http; +using CoreSyncServer.Services; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; + +namespace CoreSyncServer.Filters; + +/// +/// Builds the responses the sync API returns for a failure it recognises: the status code the client +/// always received for that case, plus the and +/// headers and a body that a +/// current CoreSync.Http.Client turns into a typed failure the app can explain to the user. +/// +/// +/// Status codes are deliberately unchanged from what the controller returned before the signal +/// existed (404 for an unknown endpoint or session, 503 for an offline agent, 422 for an unsupported +/// configuration): an older client only looks at the status, and must keep seeing what it saw. The +/// body also keeps the host's UseStatusCodePagesWithReExecute from replacing a 404 with the +/// HTML not-found page, which has no content type a sync client could use. +/// +public static class SyncErrorResponses +{ + public static ObjectResult EndpointNotFound(HttpContext httpContext, Guid endpointId) + => Create(httpContext, StatusCodes.Status404NotFound, SyncHttpErrorCodes.EndpointNotFound, + $"Synchronization endpoint '{endpointId}' does not exist or is not published."); + + public static ObjectResult SessionExpired(HttpContext httpContext, Guid sessionId) + => Create(httpContext, StatusCodes.Status404NotFound, SyncHttpErrorCodes.SessionExpired, + $"Synchronization session '{sessionId}' is no longer available on the server. Start a new synchronization."); + + public static ObjectResult AgentUnavailable(HttpContext httpContext, string message) + => Create(httpContext, StatusCodes.Status503ServiceUnavailable, SyncHttpErrorCodes.AgentUnavailable, message); + + /// + /// The 503 an agent failure has always produced, with agent-error when the agent answered + /// with a failure and agent-unavailable when it did not answer at all. Neither code is + /// permanent, so clients keep retrying as they did. + /// + public static ObjectResult AgentFailure(HttpContext httpContext, AgentOfflineException exception) + => Create( + httpContext, + StatusCodes.Status503ServiceUnavailable, + exception is AgentOperationFailedException ? SyncHttpErrorCodes.AgentError : SyncHttpErrorCodes.AgentUnavailable, + exception.Message); + + public static ObjectResult ConfigurationError(HttpContext httpContext, string message) + => Create(httpContext, StatusCodes.Status422UnprocessableEntity, SyncHttpErrorCodes.ConfigurationError, message); + + public static ObjectResult Create(HttpContext httpContext, int statusCode, string code, string message, string? tableName = null) + { + var details = new SyncErrorDetails + { + Code = code, + Message = message, + TableName = tableName, + TraceId = httpContext.TraceIdentifier + }; + + httpContext.Response.Headers[SyncHttpHeaders.ErrorCode] = code; + httpContext.Response.Headers[SyncHttpHeaders.TraceId] = details.TraceId; + + return new ObjectResult(details) { StatusCode = statusCode }; + } + + /// + /// Arranges for the error code and trace headers to be on the response the host's exception + /// handler produces for an exception this API does not handle, without handling it: the exception + /// keeps propagating to UseExceptionHandler, the Sentry middleware and the request logger + /// exactly as before. + /// + /// + /// The exception handler middleware clears every header already set when it takes over, so the + /// headers are added from an callback, which it does not + /// clear and which runs when its error response starts. A host without any handler lets Kestrel + /// write a bare 500, which runs no OnStarting callbacks: that response carries no header, and the + /// status alone still tells the client the server answered. Nothing is registered once the + /// response has started, because that would throw and replace the real failure. + /// + public static void TagUnhandled(HttpContext httpContext, Exception exception) + { + var response = httpContext.Response; + if (response.HasStarted) + return; + + var code = Classify(exception); + var traceId = httpContext.TraceIdentifier; + + response.OnStarting(() => + { + if (!response.Headers.ContainsKey(SyncHttpHeaders.ErrorCode)) + { + response.Headers[SyncHttpHeaders.ErrorCode] = code; + response.Headers[SyncHttpHeaders.TraceId] = traceId; + } + + return Task.CompletedTask; + }); + } + + /// + /// Picks the error code for an exception the controller did not expect: a provider's + /// means the store refused a change; anything else is an + /// unclassified server failure, which still tells the client the device is online. + /// + public static string Classify(Exception exception) + { + for (var current = exception; current != null; current = current.InnerException) + { + if (current is SynchronizationException) + return SyncHttpErrorCodes.DataError; + + if (current is NotSupportedException) + return SyncHttpErrorCodes.ConfigurationError; + } + + return SyncHttpErrorCodes.ServerError; + } +} diff --git a/src/Services/Implementation/AgentProxySyncProvider.cs b/src/Services/Implementation/AgentProxySyncProvider.cs index 722a79f..1709557 100644 --- a/src/Services/Implementation/AgentProxySyncProvider.cs +++ b/src/Services/Implementation/AgentProxySyncProvider.cs @@ -1,4 +1,4 @@ -using CoreSync; +using CoreSync; using CoreSyncServer.Agent.Contracts; using CoreSyncServer.Services.Hubs; using Microsoft.AspNetCore.SignalR; @@ -12,6 +12,14 @@ namespace CoreSyncServer.Services.Implementation; /// Analogous to CoreSync.Http.Client's SyncProviderHttpClient, but over the /// persistent agent connection rather than HTTP. Provisioning and retention APIs are out of /// scope for the initial iteration and throw . +/// +/// Two failure shapes are told apart: when no agent is connected +/// or the RPC did not complete (timeout, dropped connection), and +/// when the agent answered with an error - SignalR +/// surfaces the agent handler's exception as a - which is a failure of +/// the data store behind it, not of its availability. The latter derives from the former, so the +/// controller answers both with the 503 it always did; only the error code header differs. +/// /// internal sealed class AgentProxySyncProvider : ISyncProvider { @@ -52,6 +60,11 @@ public async Task GetStoreIdAsync(CancellationToken cancellationToken = de { return await client.SyncGetStoreId(_tables).WaitAsync(RpcTimeout, cancellationToken); } + catch (HubException ex) + { + // The agent answered: its provider failed. Same 503 as before, told apart by its code. + throw new AgentOperationFailedException(_dataStoreId, "SyncGetStoreId", ex); + } catch (Exception ex) when (ex is not AgentOfflineException and not OperationCanceledException) { throw new AgentOfflineException(_dataStoreId, $"Agent RPC 'SyncGetStoreId' failed for DataStore {_dataStoreId}.", ex); @@ -65,6 +78,11 @@ public async Task GetSyncVersionAsync(CancellationToken cancellatio { return await client.SyncGetSyncVersion(_tables).WaitAsync(RpcTimeout, cancellationToken); } + catch (HubException ex) + { + // The agent answered: its provider failed. Same 503 as before, told apart by its code. + throw new AgentOperationFailedException(_dataStoreId, "SyncGetSyncVersion", ex); + } catch (Exception ex) when (ex is not AgentOfflineException and not OperationCanceledException) { throw new AgentOfflineException(_dataStoreId, $"Agent RPC 'SyncGetSyncVersion' failed for DataStore {_dataStoreId}.", ex); @@ -89,6 +107,11 @@ public async Task GetChangesAsync( SyncPayloadConverter.NormalizeChangeSet(changeSet); return changeSet; } + catch (HubException ex) + { + // The agent answered: its provider failed. Same 503 as before, told apart by its code. + throw new AgentOperationFailedException(_dataStoreId, "SyncGetChanges", ex); + } catch (Exception ex) when (ex is not AgentOfflineException and not OperationCanceledException) { throw new AgentOfflineException(_dataStoreId, $"Agent RPC 'SyncGetChanges' failed for DataStore {_dataStoreId}.", ex); @@ -113,6 +136,11 @@ public async Task ApplyChangesAsync( return await client.SyncApplyChanges(changeSet, updateResolution, deleteResolution, _tables) .WaitAsync(RpcTimeout, cancellationToken); } + catch (HubException ex) + { + // The agent answered: its provider failed. Same 503 as before, told apart by its code. + throw new AgentOperationFailedException(_dataStoreId, "SyncApplyChanges", ex); + } catch (Exception ex) when (ex is not AgentOfflineException and not OperationCanceledException) { throw new AgentOfflineException(_dataStoreId, $"Agent RPC 'SyncApplyChanges' failed for DataStore {_dataStoreId}.", ex); @@ -126,6 +154,11 @@ public async Task SaveVersionForStoreAsync(Guid otherStoreId, long version, Canc { await client.SyncSaveVersionForStore(otherStoreId, version, _tables).WaitAsync(RpcTimeout, cancellationToken); } + catch (HubException ex) + { + // The agent answered: its provider failed. Same 503 as before, told apart by its code. + throw new AgentOperationFailedException(_dataStoreId, "SyncSaveVersionForStore", ex); + } catch (Exception ex) when (ex is not AgentOfflineException and not OperationCanceledException) { throw new AgentOfflineException(_dataStoreId, $"Agent RPC 'SyncSaveVersionForStore' failed for DataStore {_dataStoreId}.", ex); diff --git a/src/Services/ServiceCollectionExtensions.cs b/src/Services/ServiceCollectionExtensions.cs index 7995ace..4b9c1a9 100644 --- a/src/Services/ServiceCollectionExtensions.cs +++ b/src/Services/ServiceCollectionExtensions.cs @@ -68,7 +68,10 @@ public static IServiceCollection AddCoreSyncServerServices( services.AddScoped(); services.AddScoped(); +#pragma warning disable CS0618 // kept registered for hosts that still reference it services.AddScoped(); +#pragma warning restore CS0618 + services.AddScoped(); services.AddScoped(); services.AddSingleton(); diff --git a/src/WebClient/CoreSyncServer.Client.csproj b/src/WebClient/CoreSyncServer.Client.csproj index e59f5c5..ae6c893 100644 --- a/src/WebClient/CoreSyncServer.Client.csproj +++ b/src/WebClient/CoreSyncServer.Client.csproj @@ -12,9 +12,9 @@ - - - + + + diff --git a/tests/CoreSyncServer.Tests/AgentFailureResponseTests.cs b/tests/CoreSyncServer.Tests/AgentFailureResponseTests.cs new file mode 100644 index 0000000..b1fd295 --- /dev/null +++ b/tests/CoreSyncServer.Tests/AgentFailureResponseTests.cs @@ -0,0 +1,52 @@ +using CoreSync.Http; +using CoreSyncServer.Filters; +using CoreSyncServer.Services; +using FluentAssertions; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.SignalR; + +namespace CoreSyncServer.Tests; + +/// +/// An agent that answers a sync RPC with an error and an agent that does not answer at all have +/// always both been a 503 with the exception message as body. They still are - a server deployed +/// ahead of its clients must look identical to them - and the only thing that tells them apart is the +/// error code header a current client reads. +/// +public class AgentFailureResponseTests +{ + [Fact] + public void AgentOperationFailed_IsStillAnAgentOfflineExceptionWithTheLegacyMessage() + { + var ex = new AgentOperationFailedException(12, "SyncApplyChanges", new HubException("FK violation on Posts")); + + // What SyncController catches, and the body text a 1.0.1 client has been receiving + ex.Should().BeAssignableTo(); + ex.Message.Should().Be("Agent RPC 'SyncApplyChanges' failed for DataStore 12."); + ex.DataStoreId.Should().Be(12); + ex.InnerException.Should().BeOfType(); + } + + [Fact] + public void AgentFailure_Keeps503AndTellsTheTwoCasesApartByCode() + { + var reported = new DefaultHttpContext(); + var reportedResult = SyncErrorResponses.AgentFailure(reported, new AgentOperationFailedException(12, "SyncApplyChanges", new HubException("boom"))); + + reportedResult.StatusCode.Should().Be(StatusCodes.Status503ServiceUnavailable); + reported.Response.Headers[SyncHttpHeaders.ErrorCode].ToString().Should().Be(SyncHttpErrorCodes.AgentError); + reported.Response.Headers[SyncHttpHeaders.TraceId].ToString().Should().NotBeNullOrWhiteSpace(); + reportedResult.Value.Should().BeOfType() + .Which.Message.Should().Be("Agent RPC 'SyncApplyChanges' failed for DataStore 12."); + + var offline = new DefaultHttpContext(); + var offlineResult = SyncErrorResponses.AgentFailure(offline, new AgentOfflineException(12, "No agent is currently connected for DataStore 12.")); + + offlineResult.StatusCode.Should().Be(StatusCodes.Status503ServiceUnavailable); + offline.Response.Headers[SyncHttpHeaders.ErrorCode].ToString().Should().Be(SyncHttpErrorCodes.AgentUnavailable); + + // Neither stops a client from retrying, which is what every client did with a 503 + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AgentError).Should().BeFalse(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AgentUnavailable).Should().BeFalse(); + } +} diff --git a/tests/CoreSyncServer.Tests/CoreSyncServer.Tests.csproj b/tests/CoreSyncServer.Tests/CoreSyncServer.Tests.csproj index bac1904..35bff09 100644 --- a/tests/CoreSyncServer.Tests/CoreSyncServer.Tests.csproj +++ b/tests/CoreSyncServer.Tests/CoreSyncServer.Tests.csproj @@ -13,11 +13,11 @@ all - + - + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/tests/CoreSyncServer.Tests/SyncControllerTests.cs b/tests/CoreSyncServer.Tests/SyncControllerTests.cs index 04d5e15..cfbc820 100644 --- a/tests/CoreSyncServer.Tests/SyncControllerTests.cs +++ b/tests/CoreSyncServer.Tests/SyncControllerTests.cs @@ -1,4 +1,5 @@ using CoreSync; +using CoreSync.Http; using CoreSync.Http.Client; using CoreSync.Sqlite; using CoreSyncServer.Data; @@ -7,6 +8,8 @@ using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; +using System.Net; +using System.Net.Http.Json; namespace CoreSyncServer.Tests; @@ -219,14 +222,145 @@ public async Task Sync_InvalidEndpoint_ReturnsNotFound() response.StatusCode.Should().Be(System.Net.HttpStatusCode.NotFound); } - private ISyncProviderHttpClient CreateRemoteSyncClient() + // The sync API answers the failures it recognises with the status code it always used plus the + // X-CoreSync-Error / X-CoreSync-Trace headers and a SyncErrorDetails body, so a current client can + // tell the user what happened while an older one keeps seeing the status it expects. + + [Fact] + public async Task Sync_InvalidEndpoint_CarriesEndpointNotFoundSignal() + { + var httpClient = _factory.CreateClient(); + + var response = await httpClient.GetAsync($"api/sync/{Guid.NewGuid()}/store-id"); + + response.StatusCode.Should().Be(HttpStatusCode.NotFound); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle().Which.Should().Be(SyncHttpErrorCodes.EndpointNotFound); + response.Headers.GetValues(SyncHttpHeaders.TraceId).Should().ContainSingle().Which.Should().NotBeNullOrWhiteSpace(); + + // A JSON body, not the host's HTML not-found page the status code pages would otherwise serve + response.Content.Headers.ContentType!.MediaType.Should().Be("application/json"); + var details = await response.Content.ReadFromJsonAsync(); + details!.Code.Should().Be(SyncHttpErrorCodes.EndpointNotFound); + details.TraceId.Should().Be(response.Headers.GetValues(SyncHttpHeaders.TraceId).Single()); + } + + [Fact] + public async Task Sync_UnpublishedEndpoint_CarriesEndpointNotFoundSignal() + { + Guid unpublishedId; + using (var scope = _factory.Services.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + var config = await db.DataStoreConfigurations.FirstAsync(); + var endpoint = new Endpoint { Id = Guid.NewGuid(), Name = "Draft", DataStoreConfigurationId = config.Id, IsPublished = false }; + db.Endpoints.Add(endpoint); + await db.SaveChangesAsync(); + unpublishedId = endpoint.Id; + } + + var response = await _factory.CreateClient().GetAsync($"api/sync/{unpublishedId}/store-id"); + + response.StatusCode.Should().Be(HttpStatusCode.NotFound); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle(SyncHttpErrorCodes.EndpointNotFound); + } + + [Fact] + public async Task Sync_UnknownDownloadSession_CarriesSessionExpiredSignal() + { + var response = await _factory.CreateClient().GetAsync($"api/sync/{_endpointId}/changes-bulk-item/{Guid.NewGuid()}/0/50"); + + // Still a 404 - that is what an older client has always received here - but no longer + // indistinguishable from an unknown endpoint + response.StatusCode.Should().Be(HttpStatusCode.NotFound); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle(SyncHttpErrorCodes.SessionExpired); + + var details = await response.Content.ReadFromJsonAsync(); + details!.Code.Should().Be(SyncHttpErrorCodes.SessionExpired); + details.Message.Should().NotBeNullOrWhiteSpace(); + } + + [Fact] + public async Task Sync_UnknownUploadSession_CarriesSessionExpiredSignal() + { + var httpClient = _factory.CreateClient(); + + var complete = await httpClient.PostAsync($"api/sync/{_endpointId}/changes-bulk-complete/{Guid.NewGuid()}", null); + complete.StatusCode.Should().Be(HttpStatusCode.NotFound); + complete.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle(SyncHttpErrorCodes.SessionExpired); + + var item = await httpClient.PostAsJsonAsync($"api/sync/{_endpointId}/changes-bulk-item", new BulkChangeSetUploadItem { SessionId = Guid.NewGuid(), Items = [] }); + item.StatusCode.Should().Be(HttpStatusCode.NotFound); + item.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle(SyncHttpErrorCodes.SessionExpired); + } + + [Fact] + public async Task Sync_UnknownEndpoint_IsReportedAsConfigurationErrorByTheAgent() + { + var remoteSyncClient = CreateRemoteSyncClient(Guid.NewGuid()); + var localProvider = CreateSqliteSyncProvider(_localDbPath, ProviderMode.Local); + + var syncAgent = new SyncAgent(localProvider, remoteSyncClient); + var act = () => syncAgent.SynchronizeAsync(); + + var ex = (await act.Should().ThrowAsync()).Which; + + // What the app can now tell the user, and what it could before (an HttpRequestException) + ex.Kind.Should().Be(SyncFailureKind.ConfigurationError); + ex.Stage.Should().Be(SynchronizationStage.RemoteStoreId); + ex.ErrorCode.Should().Be(SyncHttpErrorCodes.EndpointNotFound); + ex.TraceId.Should().NotBeNullOrWhiteSpace(); + ex.ServerMessage.Should().Contain("not published"); + ex.InnerException.Should().BeAssignableTo(); + } + + [Fact] + public async Task Sync_ProviderFailure_TagsTheHostErrorResponse() + { + // An endpoint whose configuration names a table the database does not have: the provider + // throws, the exception propagates to the host's exception handler as before (Sentry still + // sees it), and the response it produces carries the error code and trace headers. + Guid brokenEndpointId; + using (var scope = _factory.Services.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + var dataStore = await db.DataStores.FirstAsync(); + + var config = new DataStoreConfiguration { Name = "Broken Config", DataStoreId = dataStore.Id }; + db.DataStoreConfigurations.Add(config); + await db.SaveChangesAsync(); + + config.TableConfigurations.Add(new DataStoreTableConfiguration + { + Name = "MissingTable", + SyncMode = DataStoreTableConfigurationSyncMode.UploadAndDownload, + Sort = 0, + DataStoreConfigurationId = config.Id + }); + await db.SaveChangesAsync(); + + var endpoint = new Endpoint { Id = Guid.NewGuid(), Name = "Broken Endpoint", DataStoreConfigurationId = config.Id, IsPublished = true }; + db.Endpoints.Add(endpoint); + await db.SaveChangesAsync(); + brokenEndpointId = endpoint.Id; + } + + var response = await _factory.CreateClient().GetAsync($"api/sync/{brokenEndpointId}/changes-bulk/{Guid.NewGuid()}"); + + response.StatusCode.Should().Be(HttpStatusCode.InternalServerError); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).Should().ContainSingle(SyncHttpErrorCodes.ServerError); + response.Headers.GetValues(SyncHttpHeaders.TraceId).Should().ContainSingle().Which.Should().NotBeNullOrWhiteSpace(); + } + + private ISyncProviderHttpClient CreateRemoteSyncClient() => CreateRemoteSyncClient(_endpointId); + + private ISyncProviderHttpClient CreateRemoteSyncClient(Guid endpointId) { var httpClient = _factory.CreateClient(); var services = new ServiceCollection(); services.AddSingleton(new TestHttpClientFactory(httpClient)); services.AddCoreSyncHttpClient(options => { - options.SyncControllerRoute = $"api/sync/{_endpointId}"; + options.SyncControllerRoute = $"api/sync/{endpointId}"; options.BulkItemSize = 50; }); var sp = services.BuildServiceProvider();