♻️ Entity - fix hierarchy - Entity dependency on AuditableEntity

[wicksipedia]

Found in #277

The inheritance model looks upside down - This feels backwards that all Entities are Auditable, but not all AuditableEntities are Entities?

public abstract class Entity<TId> : AuditableEntity
{
    public TId Id { get; set; } = default!;
}

Recommendations (either):

1. Flip the dependency hieracrchy Entities can inherit from Entity<TId> or AudiableEntity<TId> (which itself inherits from Entity<TId>
2. Modify IAudiableEntity to use default implementations This adds the risk of making the auditable fields protected not private It also means that entities will need to implement the interface as required instead of it being hidden away
3. Use IAuditableEntity as a marker interface Add an EntityAudit table that records table name, id, modified date, and json stringified entity Refactor interceptor to insert rows when entities are added/modified

As per conversation with @christoment and @matt-goldman - we decided to go with option 3 as it kept the demonstrable value of EF interceptors and added more business value to the "auditing" part.

[danielmackay]

CC: @christoment @wicksipedia @matt-goldman

As per our conversation on Teams, I would like to see an open team discussion and several questions answered before commiting this change.

I've got some concerns about adding a full audit tracking system into a CA/DDD template. Especially one that introduces a 2nd DB context and additional DB schema. It's a substantial change to make, and I'm not convinced it should be the default OOTB behaviour for the template.

1. Should this be the default behaviour?
2. Can it be opt-into?
3. What if I what simple auditing on entities (which I still believe is useful), can I switch to that)?
4. Did you research other mechanism for this? (such as Temporal Tables or SQL CDC?)
5. Does this change make more sense to be a reference architecture instead?
6. Regardless of the choice, we should do an ADR on this

[christoment]

I love options so I'm really keen on having a standard way to do this.

Correct me if I'm wrong, but looking from where we implement it, we can either: A. Do this in infrastructure (e.g. Temporal tables, CDC/trigger + something else) B. Do this in code (with AuditTable + DBInterceptor) C. Maybe something else...?

I think we should check on when we should go with A/B/C first (pros & cons). If our strong recommendation is to use audit table for most use cases:

1. Should this be the default behaviour? Yes, if the above outcome is option B is our default recommendation.

2. Can it be opt-into? Definitely yes for both use case. I think with a separate DBContext + different interceptor this is close to opt-in style table.

3. What if I what simple auditing on entities (which I still believe is useful), can I switch to that)? Interestingly this kind of approach is harder to plug-in on demand, since now we have to do migrations for all entities 😅.

4. Did you research other mechanism for this? (such as Temporal Tables or SQL CDC?) N/A

5. Does this change make more sense to be a reference architecture instead? Reference Architecture should definitely have this since we can't really show Option A in the ref arch. And we should only bake audit table in if we have good recommendation for this option.

6. Regardless of the choice, we should do an ADR on this Definitely 😄

[wicksipedia]

The PR isn't intended on being merged in its current state - it was more of a spike to see what was possible to get working.

Ideally, I'd prefer to see SQL dbs using triggers to snapshot the info. Cosmos would be a change feed implementation, but not sure how that would work right now.

1. Should this be the default behaviour?
2. Can it be opt-into?
3. What if I what simple auditing on entities (which I still believe is useful), can I switch to that)?

Having just built out this spike, I think it may make more sense to build this as a separate package that can be reused.

That way you can wire in via DI and choose between the simple user/timestamp column "auditing", or a more full featured one.

This may help reduce the cognitive load on the 2nd context etc (not saying that we stay with this approach).

But I strongly believe that knowing what changed is incredibly valuable especially if you can see that over time.

4. Did you research other mechanism for this? (such as Temporal Tables or SQL CDC?)

No, but again this was only a spike into what was possible with code alone not a fully worked solution.

5. Does this change make more sense to be a reference architecture instead?

I believe that it's better to have really good defaults at the starting point.

6. Regardless of the choice, we should do an ADR on this

Yep agree, but there needs to be more time than 1/2 day to look into the other solutions

[danielmackay]

@wicksipedia thanks for the reply. Did you still want to address the original issue relating to AuditableEntity

[danielmackay]

Closing this for now as we're not adding full on auditing into the CA template. Can create a separate reference architecture if needed.