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

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

[sonarqubecloud]

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.

[sonarqubecloud]

Quality Gate failed

Failed conditions
62.1% Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud