Tests that make sure user provided implementations of interfaces have expected ServiceLifetime

[martinothamar]

Description

Proposal for solution

Related Issue(s)

• Tests that make sure user provided implementations of interfaces have expected ServiceLifetime #757

Verification

• Your code builds clean without any errors or warnings
• Manual testing done (required)
• Relevant automated test added (if you find this hard, leave it and we'll help out)
• All tests run green

Documentation

• User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

[martinothamar]

Nevermind, this doesn't actually work... Working on a new suggestion

[ivarne]

I think the correct approach would be to have a shared factory for all extension point interfaces, that are registered as a Scoped service and retrieves services from a IServiceProvider (that inherits the scope). That way we can also add telemetry span around the GetRequiredService call to catch potentially slow constructors written by users.

[ivarne]

What is the point of this wrapper around the ServiceProvider giving the methods new names?

It would make more sense if it contained a function per interface or something like that.

    public IEnumerable<IDataProcessor> GetDataProcessors() => 
        _sp.GetServices<IDataProcessor>()

[martinothamar]

No specific intention behind the names, I went with short and simple, open to alternatives.

Specific overalods per "type" feels like a lot of code for little reason to me, what is the added benefit specifically?

[ivarne]

I think it makes more sense to use the same names as IServiceProivder if we expose the same functionality.

1. This list of methods will work as a list of interfaces that we intend users to implement to override functionality.
2. More obvious to use the special method, instead of just "use this different service provider".
3. It documents and enforces that an interface should be retrieved as IEnumerable<T> instead of just fetching the first `T, so that we do that consistently.

[HauklandJ]

I like the idea of going factory.GetServiceIWant rather than factory.Get<ServiceIWant> because of the doc and autocomplete it gives.

[martinothamar]

This list of methods will work as a list of interfaces that we intend users to implement to override functionality.

We can get the same effect from searching for attribute use though

More obvious to use the special method, instead of just "use this different service provider".

In this case there are build errors if one tries to inject IEFormidlingMetadata using constructors or IServiceProvider directly though (and so you have to use the the different service provider/factory regardless of how we implement it). In any case, if we are developing in the context of some feature, these methods living somewhere else probably won't matter in terms of "obviousness"? In that scenario you are thinking about how to implement the feature, right?

It documents and enforces that an interface should be retrieved as IEnumerable instead of just fetching the first `T, so that we do that consistently.

Isn't that obvious depending on the T? Presumably the person developing the code requiring the T also knows how many Ts there are/could be?

I like the idea of going factory.GetServiceIWant rather than factory.Get because of the doc and autocomplete it gives.

Again, at the time one is reaching for ServiceIWant, you already know what you need right? I'm struggling a bit with these scenarios that are raised 😄

[bjorntore]

I'm fine with both ways, with a slight preference for what @martinothamar already did, but with the same method names as IServiceProvider.

[ivarne]

What benefits will marking interfaces in this way give? Can it make them show up as suggestions in app developers editors?

[martinothamar]

In this PR just as a mechanism to categorize. I could make a list of interfaces, but then it is hard to know that the list should be updated when an interface is added. There is future potential as well. Main point is having some mechanism to categorize the interfaces (since it is public). Could also be the inverse - public interfaces that are not meant to be public are attributed with Pubternal or something

[ivarne]

I find this concept and the additional indirection a little hard to understand.

The straight forward solution would be to just inject a scoped ServiceProvider and use reflection to access the IsRootScope to verify when running in debug mode (as we do in tests, but not when apps are running)

#if DEBUG
        var serviceProviderType = serviceProvider.GetType();
        if (
            serviceProviderType.FullName
            != "Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope"
        )
        {
            throw new InvalidOperationException(
                "InterfaceFactory expects ServiceProviderEngineScope to run debug scope validation"
            );
        }
        var rootProperty = serviceProviderType.GetProperty(
            "IsRootScope",
            System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance
        );
        if (rootProperty == null)
        {
            throw new InvalidOperationException(
                "InterfaceFactory expects ServiceProviderEngineScope to have IsRootScope property"
            );
        }
        var rootPropertyValue = rootProperty.GetValue(serviceProvider);
        switch (rootPropertyValue)
        {
            case true:
                throw new InvalidOperationException("InterfaceFactory should not be used in root scope");
            case false:
                break;
            default:
                throw new InvalidOperationException("InterfaceFactory expects IsRootScope to be a boolean");
        }
#endif

[martinothamar]

Hmmm so my only intention with this thing was to enable HTTP-less test fixtures, and that's it. Might have given it a poor name 😄 So ideally what we go with only changes behavior for those specific cases (simple tests using only a DI container). Since for all other cases (app runtime, integration tests) we want the IHttpContextAccessor request scope, I'm not sure if the code you posted is justified. To me it seems complicating rather than simplifying

[ivarne]

I think having a constructor (IServiceProivder sp) is simpler (especially for tests) than having to wrap it in an internal DIScopeHolder. The code I posted is just part of an internal verification, and not the how the rest of the code base uses the interface factory.

[martinothamar]

The tests don't need to care about that though. There are the 2 DI methods for registering the factory as a service, so in that sense I would say this code is internal to the factory. So the two "modes" of execution for the current code is

1. AddAppImplementationFactory -> IHttpContextAccessor.HttpContext.RequestServices (for real/app and integration test use)
2. AddTestAppImplementationFactory -> Root container (for simple tests where we don't want to spin up ASP.NET Core test host and whatnot)

I added a commit where I get rid of the so called "scope holder" and just use a bool in the constructor instead. So in the first mode we will get an exception if there is no request scope, and in the test mode I don't think we really care

[HauklandJ]

I like the approach, some comments

[HauklandJ]

I like the idea of going factory.GetServiceIWant rather than factory.Get<ServiceIWant> because of the doc and autocomplete it gives.

[danielskovli]

@martinothamar I don't a useful opinion or contribution to this discussion, unfortunately. It would be great to get a summary of the solution at some point though, after the details have been worked out.

[sonarcloud]

Quality Gate failed

Failed conditions
24.3% Coverage on New Code (required ≥ 65%)
33.33% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

[bjorntore]

Really like the attribute plus DiagnosticAnalyzer approach. No objections.