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

Remove static #315

Closed
Steffen007 opened this issue Jul 28, 2020 · 10 comments
Closed

Remove static #315

Steffen007 opened this issue Jul 28, 2020 · 10 comments
Assignees

Comments

@Steffen007
Copy link

To my mind Audit.NET shouldn't use static. This can create some problems.

One example from my project I wrote a unit test, where I changed Entity Framework Core AuditEntityAction. I simulate an error and it throws an exception. Because of that an independent unit test in other class fails. Xunit runs unit tests from different classes in parallel and you can't change it in good way. Because of the static property "Audit.Core.DataProvider" I can't set a new DataProvider.

I also do not know how it would be possible to use a second DataProvider.

I would be very happy if that were to change.

@thepirat000
Copy link
Owner

Well, this is by design. The Data Provider is global and shared across the app domain. It is single because it represents the default behavior for "what to do with the audit events". So if you want them to be saved to multiple databases, you can implement your custom data provider with your custom logic (sequential saving, fallback mechanism, or whatever).

Also note you can set the data provider per AuditScope instance. But it depends on the extensions you are using. (i.e. for entity framework audit events, you can set a dataprovider instance on each DbContext)

I think the only problem is that you will not be able to run your tests in parallel, or what other kind of problems do you see?

@Steffen007
Copy link
Author

I think you already have the same issue. You can't checkout the project and say: Run all tests. It will fail.
You cannot run

  • Audit.EntityFramework.UnitTest.EfStressTests.Test_EF_MultiThread
  • Audit.EntityFramework.UnitTest.InheritanceTests.Test_Ef_Inheritance

together.

If my understanding is right, you must Reset() everything in every unit test because of this.

@thepirat000
Copy link
Owner

That's correct.

But apart from parallel test running, I don't see any other issue with this. Do you?

Anyway I'm open to ideas on how to handle this in a non-static way, and without breaking backwards compatibility.

@adrianiftode
Copy link
Contributor

When I integrated Audit.NET in one of the projects I worked on, I extracted two interfaces: IAuditScopeFactory and IAuditScope The main reason was to enable unit testing and to bypass direct AuditScope calls. Those interfaces were actually very minimal, for our needs and not a full API definition.

In this way, we solved the unit testing problem. We can verify for example that certain scopes are created or certain properties are added to scopes, so on.

I think if Audit.NET ships these interfaces then it will satisfy those who need to unit test the integration with Audit.NET as they don't have to build again the abstractions.

@thepirat000
Copy link
Owner

thepirat000 commented Aug 3, 2020

We could expose those interfaces, but I guess it will only be helpful for testing code where you manually create the AuditScope.
I don't see a way to do it for example for the WebAPI attributes or extensions that depends on the static configuration, without making assumptions on the client code.

@adrianiftode
Copy link
Contributor

I think The Audit.NET's internal implementation should be oblivious about these interfaces and keep the existing details as they are. Those interfaces are purely for testing purposes. Maybe I'm hijacking this issue and my intention is not really what @Steffen007 wants to achieve. I can open a separate issue and describe there how I see the role of those interfaces.

@thepirat000
Copy link
Owner

thepirat000 commented Aug 3, 2020

Sounds good to me. Anyway I'm thinking on adding at least a simple IAuditScopeFactory interface that could be set to a custom implementation/mock via fluent API configuration on most of the extensions.

@Steffen007
Copy link
Author

I suggest to make the framework well testable. It's a framework and it's used from many projects. If you implement a new storage logic like in #316, it will be very important to create a integration test. External persons don't know the details of this framework. For these people it's very important to write some (integration) tests. To my mind the tests of the pull requests aren't unusual. But the static creates an impact for your own project, which used Audit.NET.

I don't know every detail of the Audit.NET framework. I try to help with my semi-knowledge and do a kind of brainstorm session. Maybe it's helpful for a solution.
The reason to use static is, that you need a kind of central point. I would create a class of that. Maybe AuditConfigurationContext is good name. Then you can insert everything in it. And if you want you can use only one instance. Put it in (static) singleton or better you integrate it in the DI. This context is the entry point to your system. Static variables are singleton by syntax but without the possibility to do it twice. If you system depends only on "entry point class", you can assign the infrastructure to a static variable. But then this is only the singleton variable without dependencies to your system.

To my mind you can create a new major version with breaking changes. You can document it. So everybody know to upgrade it.

I hope I could help. Audit.NET is a wonderful framework and I want to make it more usable. But unfortunately the mentioned point has a negative impact for external projects at the moment. To my mind an important point about frameworks is, that an external framework should not bring a negative impact.

@thepirat000
Copy link
Owner

thepirat000 commented Aug 7, 2020

In this pull request #322 I'm proposing a new AuditScopeFactory that can replace the static calls to AuditScope.Create.

So you should be able to create an auditscope with an injected/mocked factory:

public class MyClass
{
    private IAuditScopeFactory _auditScopeFactory;
    public MyClass(IAuditScopeFactory auditScopeFactory)
    {
        _auditScopeFactory = auditScopeFactory;
    }

    public void SomeMethodToBeTested()
    {
        using (var scope = _auditScopeFactory.Create(new AuditScopeOptions(...))))
        {
            ...
        }
    }
}

@thepirat000
Copy link
Owner

This was included on version 16.0.0.

The static methods are still there, but you have the possibility to use the factory instead, so you can easily mock IAuditScopeFactory / IAuditScope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants