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

How to save AuditEvent entities in the same transaction? #35345

Closed
voroninp opened this issue Dec 17, 2024 · 16 comments
Closed

How to save AuditEvent entities in the same transaction? #35345

voroninp opened this issue Dec 17, 2024 · 16 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@voroninp
Copy link

voroninp commented Dec 17, 2024

I want to create audit events using ChangeTracker.
These events must be saved in the same transaction with the changed (added, modified, deleted) entities.
Current values of the properties must be read after they were generated (including the values generated in db).

I generate audit events in TransactionCommittingAsync method of DbTransactionIntepceptor. (transaction is started implicitly)
The problem I face is that state of saved entities is not updated at the moment interceptor is called, and when I attach audit events and call SaveChanges on eventData.Context entities get persisted second time.

Should I inject DbContextFactory in my interceptor, create a new instance, enlist it in the same transaction, and then attach audit events to that instance?

@voroninp
Copy link
Author

If so I'd request an api to create a fresh instance of DbContext from the existing one.

@AndriySvyryd
Copy link
Member

The problem I face is that state of saved entities is not updated at the moment interceptor is called, and when I attach audit events and call SaveChanges on eventData.Context entities get persisted second time.

You can call context.ChangeTracker.AcceptAllChanges() after you've created the audit events, but before you attach them. Just make sure that this only happens if the transaction is being committed as part of SaveChanges()

@voroninp
Copy link
Author

voroninp commented Dec 18, 2024

Just make sure that this only happens if the transaction is being committed as part of SaveChanges()

When is it not the case?
Or do you mean the situation when the following sequence of events happens:

  1. Transaction is explicitly started.
  2. Entities are changed.
  3. SaveChanges() is called
  4. Entities are changed again but without subsequent call to SaveChanges
  5. Transaction is explicitly committed, and interceptor is called.

@voroninp
Copy link
Author

Also, isn't it the risk of accepting changes prematurely? If the commit fails, the change tracker will be in a broken state so to say.

@kirides
Copy link

kirides commented Dec 18, 2024

shouldn't make a big difference, as if a transaction fails anywhere in the middle, you're forced to grab a fresh DbContext anyways, as the state it's currently in may be broken.

This is why you retry outside of EF Code, e.g. in the controller level, and not in the "repository" level, if that makes sense.

@voroninp
Copy link
Author

True, but this solves the issue of broken context.
Yet I am curious how can I prevent auditing things which were not saved in the first place.

@roji
Copy link
Member

roji commented Dec 18, 2024

@kirides is right here, you're best off placing retrying your entire unit of work - on a fresh DbContext - from the start; this means that any error triggers restarting the previous context and state. This is also exactly who EF's retrying strategies generally work.

@voroninp
Copy link
Author

@roji , Yes, I got this point. Less issues to solve ;-)
But what about this?

SavedChanges method of SaveChangesInterceptor fires after implicitly created transaction has been already committed.
That's why I use DbTRansactionInterpceptor.

@roji
Copy link
Member

roji commented Dec 18, 2024

Any reason you're using SavedChanges and not SavingChanges, which I think is supposed to run before the transaction is committed?

@voroninp
Copy link
Author

AFAIK, SavingChanges is fired before db generated values are set on properties. Am I mistaken?

@roji
Copy link
Member

roji commented Dec 18, 2024

Well, yes - SavingChanges is saved before changes are saved to the database, so obviously database-generated values aren't set at that point (the INSERT hasn't occured yet). But if your audit entities reference the things they audit, they'll have a foreign key which will get set by SavingChanges as usual (exactly like when you save a Blog and its Posts).

If you don't want to do that, you'll need a separate SaveChanges to save your audit entities - you can trigger that from the SavedChanges interceptor (and make sure to not have that run on the second, auditing SaveChanges, e.g. via an async local), or simply by overriding the SaveChanges() method itself.

@voroninp
Copy link
Author

you can trigger that from the SavedChanges interceptor

I tried it first, but eventData.Context.Database.CurrentTransaction was null, so I assumed commit happens before the SavedChanges method was called.

@voroninp
Copy link
Author

As to FKs on events.

I let events to be as generic as possible:

public sealed class EntityAuditEvent<TId, TTenantId, TUserId>
{
    public TId Id { get; private set; }
    public required DateTime Timestamp { get; init; }
    public required string? ActivityId { get; init; }
    public required Guid TransactionId { get; init; }
    public required TTenantId? TenantId { get; init; }
    public required TUserId? UserId { get; init; }
    public required EntityDescription Entity { get; init; }
    public required ChangeKind ChangeKind { get; init; }
    public required IReadOnlyList<PropertyChange> Changes { get; init; }
}

public sealed class EntityDescription
{
    public required string TypeName { get; init; }
    public required string InstanceName { get; init; }
    public required string PrimaryKeyJson { get; init; }
}

public sealed class PropertyChange
{
    public required string Name { get; init; }
    public required string OriginalValueJson { get; init; }
    public required string NewValueJson { get; init; }
}

It's the whole adventure to capture OriginalValue of complex properties :-) Also, I want to capture changes on owned entities as part of the owner.

internal static class EFExtensions
{
    public static bool IsNullEquivalent(this Dictionary<string, object?> value)
    {
        return value.All(
                kvp => kvp.Value is null
                    || (kvp.Value is Dictionary<string, object?> propertyValue && IsNullEquivalent(propertyValue)));
    }

    public static Dictionary<string, object?>? GetOriginalValue(this ComplexPropertyEntry complexProperty, AuditOptions auditOptions)
    {
        var entityType = complexProperty.CurrentValue?.GetType() ?? complexProperty.Metadata.ClrType;
        var regularProperties = complexProperty.Properties
            .Where(p => auditOptions.ShouldAuditProperty(entityType, p.Metadata.Name))
            .Select(p => (p.Metadata.Name, p.OriginalValue));

        var complexProperties = complexProperty.ComplexProperties
            .Where(p => auditOptions.ShouldAuditProperty(entityType, p.Metadata.Name))
            .Where(p => auditOptions.ShouldAuditEntity(p.CurrentValue?.GetType() ?? p.Metadata.ClrType))
            .Select(p => (p.Metadata.Name, OriginalValue: (object?)p.GetOriginalValue(auditOptions)));

        var result = regularProperties.Concat(complexProperties).ToDictionary(t => t.Name, t => t.OriginalValue);
        if (result.IsNullEquivalent())
        {
            return null;
        }

        return result;
    }
}

@roji
Copy link
Member

roji commented Dec 18, 2024

so I assumed commit happens before the SavedChanges method was called.

If there's no user-managed transaction, that's probably the case (haven't checked). If you're looking to do things with two separate SaveChanges() - which is what I'm understanding - you'll need to manage transactions on your own, at which point you're in control exactly when the transaction is committed etc.

In any case, is there any additional open question here?

@voroninp
Copy link
Author

you'll need to manage transactions on your own

Is it safe to do it inside interceptor's code? Start transaction in SavingChanges (if it's not yet started) and commit it in SavedChanges`?

@roji
Copy link
Member

roji commented Dec 18, 2024

@voroninp I'm not sure, starting a transaction in SavingChanges might be problematic (and of course would fail if you do have a user transaction already). I really recommend doing the simple thing and work with an explicit user transaction, rather than trying to automate things like this via interceptors; you can write generic code to take care of that for you - take a look at these docs, which are about resiliency, but are similar in spirit to what you're trying to achieve (i.e. close a unit of work in a single (retriable) block, which also gets automatically wrapped by a transaction).

I'll go ahead and close this as I think the alternatives are clear and there's nothing actionable on the EF side, but if you still feel you need guidance you can continue posting.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Dec 18, 2024
@roji roji added the closed-no-further-action The issue is closed and no further action is planned. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants