Skip to content
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

fix: 412 status on multiple requests without revision header #427

Merged
merged 18 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Digdir.Domain.Dialogporten.Application.Externals;
public interface IUnitOfWork
{
IUnitOfWork WithoutAuditableSideEffects();
Task<SaveChangesResult> SaveChangesAsync(CancellationToken cancellationToken = default);
Task<SaveChangesResult> SaveChangesAsync(bool optimisticConcurrency, CancellationToken cancellationToken = default);
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
}

[GenerateOneOf]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Digdir.Domain.Dialogporten.Application.Features.V1.EndUser.Dialogs.Que
public sealed class GetDialogDto
{
public Guid Id { get; set; }
public Guid Revision { get; set; }
public Guid IfMatchDialogRevision { get; set; }
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
public string Org { get; set; } = null!;
public string ServiceResource { get; set; } = null!;
public string Party { get; set; } = null!;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public async Task<GetDialogResult> Handle(GetDialogQuery request, CancellationTo

var saveResult = await _unitOfWork
.WithoutAuditableSideEffects()
.SaveChangesAsync(cancellationToken);
.SaveChangesAsync(optimisticConcurrency: false, cancellationToken);

saveResult.Switch(
success => { },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public async Task<DeleteOutboxMessagesResult> Handle(DeleteOutboxMessagesCommand
}

_db.OutboxMessages.Remove(outboxMessage);
var saveResult = await _unitOfWork.SaveChangesAsync(cancellationToken);
var saveResult = await _unitOfWork.SaveChangesAsync(optimisticConcurrency: false, cancellationToken);
saveResult.Switch(
success => { },
domainError => throw new UnreachableException("Should never get a domain error when deleting an outbox message"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public async Task<CreateDialogResult> Handle(CreateDialogCommand request, Cancel

await _db.Dialogs.AddAsync(dialog, cancellationToken);

var saveResult = await _unitOfWork.SaveChangesAsync(cancellationToken);
var saveResult = await _unitOfWork.SaveChangesAsync(optimisticConcurrency: false, cancellationToken);
return saveResult.Match<CreateDialogResult>(
success => new Success<Guid>(dialog.Id),
domainError => domainError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialog
public sealed class DeleteDialogCommand : IRequest<DeleteDialogResult>
{
public Guid Id { get; set; }
public Guid? Revision { get; set; }
public Guid? IfMatchDialogRevision { get; set; }
}

[GenerateOneOf]
Expand Down Expand Up @@ -51,9 +51,9 @@ public async Task<DeleteDialogResult> Handle(DeleteDialogCommand request, Cancel
return new EntityNotFound<DialogEntity>(request.Id);
}

_db.TrySetOriginalRevision(dialog, request.Revision);
_db.TrySetOriginalRevision(dialog, request.IfMatchDialogRevision);
_db.Dialogs.SoftRemove(dialog);
var saveResult = await _unitOfWork.SaveChangesAsync(cancellationToken);
var saveResult = await _unitOfWork.SaveChangesAsync(optimisticConcurrency: !request.IfMatchDialogRevision.HasValue, cancellationToken);
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
return saveResult.Match<DeleteDialogResult>(
success => success,
domainError => throw new UnreachableException("Should never get a domain error when creating a new dialog"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialog
public sealed class UpdateDialogCommand : IRequest<UpdateDialogResult>
{
public Guid Id { get; set; }
public Guid? Revision { get; set; }
public Guid? IfMatchDialogRevision { get; set; }
public UpdateDialogDto Dto { get; set; } = null!;
}

Expand Down Expand Up @@ -75,7 +75,7 @@ public async Task<UpdateDialogResult> Handle(UpdateDialogCommand request, Cancel
return new EntityNotFound<DialogEntity>(request.Id);
}

_db.TrySetOriginalRevision(dialog, request.Revision);
_db.TrySetOriginalRevision(dialog, request.IfMatchDialogRevision);

// Update primitive properties
_mapper.Map(request.Dto, dialog);
Expand Down Expand Up @@ -123,8 +123,7 @@ await dialog.Elements
update: UpdateApiActions,
delete: DeleteDelegate.NoOp);


var saveResult = await _unitOfWork.SaveChangesAsync(cancellationToken);
var saveResult = await _unitOfWork.SaveChangesAsync(optimisticConcurrency: !request.IfMatchDialogRevision.HasValue, cancellationToken);
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
return saveResult.Match<UpdateDialogResult>(
success => success,
domainError => domainError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialog
public sealed class GetDialogDto
{
public Guid Id { get; set; }
public Guid Revision { get; set; }
public Guid IfMatchDialogRevision { get; set; }
public string Org { get; set; } = null!;
public string ServiceResource { get; set; } = null!;
public string Party { get; set; } = null!;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ internal sealed class MappingProfile : Profile
public MappingProfile()
{
CreateMap<DialogEntity, GetDialogDto>()
.ForMember(dest => dest.IfMatchDialogRevision, opt => opt.MapFrom(src => src.Revision))
.ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.StatusId));

CreateMap<DialogActivity, GetDialogDialogActivityDto>()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,15 @@ x.Entity is IEventPublisher publisher
.Select(OutboxMessage.Create)
.ToList();

dbContext.Set<OutboxMessage>().AddRange(outboxMessages);
foreach (var outboxMessage in outboxMessages)
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
{
var existingOutboxMessage = dbContext.Set<OutboxMessage>().Find(outboxMessage.EventId);
if (existingOutboxMessage == null)
{
dbContext.Set<OutboxMessage>().Add(outboxMessage);
}
}

return base.SavingChangesAsync(eventData, result, cancellationToken);
}
}
66 changes: 65 additions & 1 deletion src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
using Digdir.Domain.Dialogporten.Infrastructure.Persistence;
using Digdir.Library.Entity.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore;
using OneOf.Types;
using Polly;
using Polly.Contrib.WaitAndRetry;
using Polly.Retry;
using Polly.Timeout;
using Polly.Wrap;

namespace Digdir.Domain.Dialogporten.Infrastructure;

Expand All @@ -29,7 +35,7 @@ public IUnitOfWork WithoutAuditableSideEffects()
return this;
}

public async Task<SaveChangesResult> SaveChangesAsync(CancellationToken cancellationToken = default)
public async Task<SaveChangesResult> SaveChangesAsync(bool optimisticConcurrency, CancellationToken cancellationToken = default)
{
if (!_domainContext.IsValid)
{
Expand All @@ -46,6 +52,16 @@ public async Task<SaveChangesResult> SaveChangesAsync(CancellationToken cancella
await _dialogDbContext.ChangeTracker.HandleAuditableEntities(_transactionTime.Value, cancellationToken);
}

if (optimisticConcurrency)
{
await ConcurrencyRetryPolicy.ExecuteAsync(async () =>
{
await _dialogDbContext.SaveChangesAsync(cancellationToken);
});

return new Success();
}

try
{
await _dialogDbContext.SaveChangesAsync(cancellationToken);
Expand All @@ -57,4 +73,52 @@ public async Task<SaveChangesResult> SaveChangesAsync(CancellationToken cancella

return new Success();
}

// Optimistic concurrency
// Total timeout for optimistic concurrency handling
private const int TimeoutInSeconds = 10;
private static readonly AsyncTimeoutPolicy TimeoutPolicy =
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
Policy.TimeoutAsync(TimeoutInSeconds,
TimeoutStrategy.Pessimistic,
(_, _, _) => throw new OptimisticConcurrencyTimeoutException());
MagnusSandgren marked this conversation as resolved.
Show resolved Hide resolved

// Backoff strategy with jitter for retry policy, starting at ~5ms
private const int MedianFirstDelayInMs = 5;
private static readonly IEnumerable<TimeSpan> JitterDelay = Backoff.DecorrelatedJitterBackoffV2(
medianFirstRetryDelay: TimeSpan.FromMilliseconds(MedianFirstDelayInMs),
retryCount: int.MaxValue);

// Fetch the db revision and retry
// https://learn.microsoft.com/en-us/ef/core/saving/concurrency?tabs=data-annotations#resolving-concurrency-conflicts
private static readonly AsyncRetryPolicy RetryPolicy = Policy
.Handle<DbUpdateConcurrencyException>()
.WaitAndRetryAsync(JitterDelay, (exception, _) =>
{
if (exception is not DbUpdateConcurrencyException concurrencyException)
{
return;
}

foreach (var entry in concurrencyException.Entries)
{
var dbValues = entry.GetDatabaseValues();
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
if (dbValues == null)
{
continue;
}

var revision = dbValues![nameof(DialogEntity.Revision)];
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
if (revision == null)
{
continue;
}

entry.CurrentValues[nameof(DialogEntity.Revision)] = revision;
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
entry.OriginalValues.SetValues(dbValues);
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
}
});

private static readonly AsyncPolicyWrap ConcurrencyRetryPolicy = TimeoutPolicy.WrapAsync(RetryPolicy);
}

internal class OptimisticConcurrencyTimeoutException : Exception;
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public override async Task HandleAsync(GetDialogQuery req, CancellationToken ct)
await result.Match(
dto =>
{
HttpContext.Response.Headers.ETag = dto.Revision.ToString();
HttpContext.Response.Headers.ETag = dto.IfMatchDialogRevision.ToString();
return SendOkAsync(dto, ct);
},
notFound => this.NotFoundAsync(notFound, ct),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public override async Task HandleAsync(CreateDialogActivityRequest req, Cancella

updateDialogDto.Activities.Add(req);

var updateDialogCommand = new UpdateDialogCommand { Id = req.DialogId, Revision = req.Revision, Dto = updateDialogDto };
var updateDialogCommand = new UpdateDialogCommand { Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision, Dto = updateDialogDto };

var result = await _sender.Send(updateDialogCommand, ct);
await result.Match(
Expand All @@ -76,8 +76,8 @@ public sealed class CreateDialogActivityRequest : UpdateDialogDialogActivityDto
{
public Guid DialogId { get; set; }

[FromHeader(headerName: Constants.IfMatch, isRequired: false)]
public Guid? Revision { get; set; }
[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? IfMatchDialogRevision { get; set; }
}

public sealed class CreateDialogActivityEndpointSummary : Summary<CreateDialogActivityEndpoint>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public override async Task HandleAsync(CreateDialogElementRequest req, Cancellat

updateDialogDto.Elements.Add(req);

var updateDialogCommand = new UpdateDialogCommand { Id = req.DialogId, Revision = req.Revision, Dto = updateDialogDto };
var updateDialogCommand = new UpdateDialogCommand { Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision, Dto = updateDialogDto };

var result = await _sender.Send(updateDialogCommand, ct);
await result.Match(
Expand All @@ -72,8 +72,8 @@ public sealed class CreateDialogElementRequest : UpdateDialogDialogElementDto
{
public Guid DialogId { get; set; }

[FromHeader(headerName: Constants.IfMatch, isRequired: false)]
public Guid? Revision { get; set; }
[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? IfMatchDialogRevision { get; set; }
}

public sealed class CreateDialogElementEndpointSummary : Summary<CreateDialogElementEndpoint>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public override async Task HandleAsync(DeleteDialogElementRequest req, Cancellat
updateDialogDto.Elements.Remove(dialogElement);

var updateDialogCommand = new UpdateDialogCommand
{ Id = req.DialogId, Revision = req.Revision, Dto = updateDialogDto };
{ Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision, Dto = updateDialogDto };

var result = await _sender.Send(updateDialogCommand, ct);
await result.Match(
Expand All @@ -82,7 +82,7 @@ public sealed class DeleteDialogElementRequest
public Guid ElementId { get; set; }

[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? Revision { get; set; }
public Guid? IfMatchDialogRevision { get; set; }
}

public sealed class DeleteDialogElementEndpointSummary : Summary<DeleteDialogElementEndpoint>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ await this.NotFoundAsync(
updateDialogDto.Elements.Add(updateDialogElementDto);

var updateDialogCommand = new UpdateDialogCommand
{ Id = req.DialogId, Revision = req.Revision, Dto = updateDialogDto };
{ Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision, Dto = updateDialogDto };

var result = await _sender.Send(updateDialogCommand, ct);
await result.Match(
Expand Down Expand Up @@ -94,8 +94,8 @@ public sealed class UpdateDialogElementRequest

public Guid ElementId { get; set; }

[FromHeader(headerName: Constants.IfMatch, isRequired: false)]
public Guid? Revision { get; set; }
[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? IfMatchDialogRevision { get; set; }

public Uri? Type { get; set; }
public string? AuthorizationAttribute { get; set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public override void Configure()

public override async Task HandleAsync(DeleteDialogRequest req, CancellationToken ct)
{
var command = new DeleteDialogCommand { Id = req.DialogId, Revision = req.Revision };
var command = new DeleteDialogCommand { Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision };
var result = await _sender.Send(command, ct);
await result.Match(
success => SendNoContentAsync(ct),
Expand All @@ -47,7 +47,7 @@ public sealed class DeleteDialogRequest
public Guid DialogId { get; set; }

[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? Revision { get; set; }
public Guid? IfMatchDialogRevision { get; set; }
}

public sealed class DeleteDialogEndpointSummary : Summary<DeleteDialogEndpoint>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public override async Task HandleAsync(GetDialogQuery req, CancellationToken ct)
await result.Match(
dto =>
{
HttpContext.Response.Headers.ETag = dto.Revision.ToString();
HttpContext.Response.Headers.ETag = dto.IfMatchDialogRevision.ToString();
return SendOkAsync(dto, ct);
},
notFound => this.NotFoundAsync(notFound, ct));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public async Task<IActionResult> Patch(
return BadRequest(ModelState);
}

var command = new UpdateDialogCommand { Id = dialogId, Revision = etag, Dto = updateDialogDto };
var command = new UpdateDialogCommand { Id = dialogId, IfMatchDialogRevision = etag, Dto = updateDialogDto };
var result = await _sender.Send(command, ct);
return result.Match(
success => (IActionResult)NoContent(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public override void Configure()

public override async Task HandleAsync(UpdateDialogRequest req, CancellationToken ct)
{
var command = new UpdateDialogCommand { Id = req.DialogId, Revision = req.Revision, Dto = req.Dto };
var command = new UpdateDialogCommand { Id = req.DialogId, IfMatchDialogRevision = req.IfMatchDialogRevision, Dto = req.Dto };
var updateDialogResult = await _sender.Send(command, ct);
await updateDialogResult.Match(
success => SendNoContentAsync(ct),
Expand All @@ -53,8 +53,8 @@ public sealed class UpdateDialogRequest
[FromBody]
public UpdateDialogDto Dto { get; set; } = null!;

[FromHeader(headerName: Constants.IfMatch, isRequired: false)]
public Guid? Revision { get; set; }
[FromHeader(headerName: Constants.IfMatch, isRequired: false, removeFromSchema: true)]
public Guid? IfMatchDialogRevision { get; set; }
}

public sealed class UpdateDialogEndpointSummary : Summary<UpdateDialogEndpoint>
Expand Down
4 changes: 2 additions & 2 deletions tests/k6/run.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ if (-not (Test-Path $FilePath)) {
}

# Generate tests
& "$PSScriptRoot\scripts\generate_alltests.ps1" "$PSScriptRoot\tests\serviceowner\" > $null
& "$PSScriptRoot\scripts\generate_alltests.ps1" "$PSScriptRoot\tests\enduser\" > $null
& "$PSScriptRoot\scripts\generate_all_tests.ps1" "$PSScriptRoot\tests\serviceowner\" > $null
& "$PSScriptRoot\scripts\generate_all_tests.ps1" "$PSScriptRoot\tests\enduser\" > $null

# Handle environment settings
$insecureSkipTLS = $null
Expand Down
4 changes: 2 additions & 2 deletions tests/k6/run.sh
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ fi

DIR="$(dirname "$0")"

"$DIR/scripts/generate_alltests.sh" "$DIR/tests/serviceowner/" >/dev/null
"$DIR/scripts/generate_alltests.sh" "$DIR/tests/enduser/" >/dev/null
"$DIR/scripts/generate_all_tests.sh" "$DIR/tests/serviceowner/" >/dev/null
"$DIR/scripts/generate_all_tests.sh" "$DIR/tests/enduser/" >/dev/null

if [[ "$API_ENVIRONMENT" == "localdev" ]]; then
# Handle self-signed certs when using docker compose
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion tests/k6/tests/serviceowner/concurrency.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ export default function () {
});

});
}
}
Loading