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

♻️ Entity - fix hierarchy - Entity dependency on AuditableEntity #282

Closed
Tracked by #290
wicksipedia opened this issue Apr 15, 2024 · 5 comments · May be fixed by #288
Closed
Tracked by #290

♻️ Entity - fix hierarchy - Entity dependency on AuditableEntity #282

wicksipedia opened this issue Apr 15, 2024 · 5 comments · May be fixed by #288
Assignees
Labels
Type: Refactor A code quality improvement e.g. Tech debt

Comments

@wicksipedia
Copy link
Member

wicksipedia commented Apr 15, 2024

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.

@wicksipedia wicksipedia added the Type: Refactor A code quality improvement e.g. Tech debt label Apr 15, 2024
@wicksipedia wicksipedia changed the title ♻️ Entity - fix Entity dependency on AuditableEntity ♻️ Entity - fix hierarchy - Entity dependency on AuditableEntity Apr 15, 2024
@wicksipedia wicksipedia linked a pull request Apr 15, 2024 that will close this issue
@danielmackay
Copy link
Member

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
Copy link
Member

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.
  1. 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.
  1. 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 😅.
  1. Did you research other mechanism for this? (such as Temporal Tables or SQL CDC?)
    N/A
  1. 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.
  1. Regardless of the choice, we should do an ADR on this
    Definitely 😄

@wicksipedia
Copy link
Member Author

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.

  1. 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.

  1. 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.

  1. 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
Copy link
Member

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

@danielmackay
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactor A code quality improvement e.g. Tech debt
Projects
Development

Successfully merging a pull request may close this issue.

3 participants