-
Notifications
You must be signed in to change notification settings - Fork 10
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
Tests that make sure user provided implementations of interfaces have expected ServiceLifetime #779
base: main
Are you sure you want to change the base?
Conversation
Nevermind, this doesn't actually work... Working on a new suggestion |
I think the correct approach would be to have a shared factory for all extension point interfaces, that are registered as a |
246ef16
to
2206625
Compare
src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs
Dismissed
Show dismissed
Hide dismissed
where T : class => _sp.GetService<T>(); | ||
|
||
public IEnumerable<T> GetAll<T>() | ||
where T : class => _sp.GetServices<T>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to use the same names as IServiceProivder if we expose the same functionality.
- This list of methods will work as a list of interfaces that we intend users to implement to override functionality.
- More obvious to use the special method, instead of just "use this different service provider".
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of going factory.GetServiceIWant
rather than factory.Get<ServiceIWant
> because of the doc and autocomplete it gives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 T
s 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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both ways, with a slight preference for what @martinothamar already did, but with the same method names as IServiceProvider.
/// Marker attribute for interfaces that are meant to be implemented by apps. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Interface, AllowMultiple = false)] | ||
internal sealed class ImplementableByAppsAttribute : Attribute { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What benefits will marking interfaces in this way give? Can it make them show up as suggestions in app developers editors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
{ | ||
internal sealed record DIScopeHolder(IServiceProvider? ScopedServiceProvider); | ||
|
||
private readonly DIScopeHolder _diScopeHolder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AddAppImplementationFactory
->IHttpContextAccessor.HttpContext.RequestServices
(for real/app and integration test use)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
src/Altinn.App.Internal.Analyzers/Altinn.App.Internal.Analyzers.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach, some comments
src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs
Outdated
Show resolved
Hide resolved
where T : class => _sp.GetService<T>(); | ||
|
||
public IEnumerable<T> GetAll<T>() | ||
where T : class => _sp.GetServices<T>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of going factory.GetServiceIWant
rather than factory.Get<ServiceIWant
> because of the doc and autocomplete it gives.
@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. |
src/Altinn.App.Internal.Analyzers/AnalyzerReleases.Unshipped.md
Outdated
Show resolved
Hide resolved
if (testMode) | ||
_getServiceProvider = () => sp; | ||
else | ||
// Right now we are just using the HttpContext to get the current scope, | ||
// in the future we might not be always running in a web context, | ||
// at that point we need to replace this | ||
_getServiceProvider = () => sp.GetRequiredService<IHttpContextAccessor>().HttpContext?.RequestServices; |
Check notice
Code scanning / CodeQL
Missed ternary opportunity Note
Quality Gate failedFailed conditions |
Really like the attribute plus DiagnosticAnalyzer approach. No objections. |
552e857
to
b539175
Compare
b539175
to
ef2b1b4
Compare
@@ -77,7 +78,8 @@ | |||
|
|||
// runs prefill from repo configuration if config exists | |||
await _prefillService.PrefillDataModel(instance.InstanceOwner.PartyId, dataType.Id, data, prefill); | |||
await _instantiationProcessor.DataCreation(instance, data, prefill); | |||
var instantiationProcessor = _appImplementationFactory.GetRequired<IInstantiationProcessor>(); | |||
await instantiationProcessor.DataCreation(instance, data, prefill); |
Check failure
Code scanning / CodeQL
Bad dynamic call Error
target
here
NullInstantiationProcessor
|
||
// Act | ||
ActionResult actual = await SUT.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); | ||
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>(); | ||
ActionResult actual = await controller.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); |
Check warning
Code scanning / CodeQL
Call to obsolete method Warning test
CopyInstance
.Setup(a => a.GetApplicationMetadata()) | ||
.ReturnsAsync(CreateApplicationMetadata(Org, AppName, false)); | ||
|
||
// Act | ||
ActionResult actual = await SUT.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); | ||
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>(); | ||
ActionResult actual = await controller.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); |
Check warning
Code scanning / CodeQL
Call to obsolete method Warning test
CopyInstance
|
||
// Act | ||
ActionResult actual = await SUT.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); | ||
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>(); | ||
ActionResult actual = await controller.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); |
Check warning
Code scanning / CodeQL
Call to obsolete method Warning test
CopyInstance
.ReturnsAsync(CreateXacmlResponse("Deny")); | ||
|
||
// Act | ||
ActionResult actual = await SUT.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); | ||
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>(); | ||
ActionResult actual = await controller.CopyInstance("ttd", "copy-instance", 343234, Guid.NewGuid()); |
Check warning
Code scanning / CodeQL
Call to obsolete method Warning test
CopyInstance
@@ -416,20 +411,26 @@ | |||
.ReturnsAsync(new DataElement()); | |||
|
|||
// Act | |||
ActionResult actual = await SUT.CopyInstance(Org, AppName, InstanceOwnerPartyId, instanceGuid); | |||
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>(); | |||
ActionResult actual = await controller.CopyInstance(Org, AppName, InstanceOwnerPartyId, instanceGuid); |
Check warning
Code scanning / CodeQL
Call to obsolete method Warning test
CopyInstance
ItExpr.IsAny<CancellationToken>() | ||
) | ||
.ReturnsAsync(httpResponseMessage); | ||
HttpClient httpClient = new HttpClient(handlerMock.Object); |
Check warning
Code scanning / CodeQL
Missing Dispose call on local IDisposable Warning test
"action", | ||
userActionAuthorizerMock.Object | ||
); | ||
IUserActionAuthorizerProvider userActionAuthorizerProvider = fixture.UserActionAuthorizerProvider; |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
userActionAuthorizerProvider
// provider.Should().ContainEquivalentOf(new UserActionAuthorizerProvider(taskId2, action, authorizer.First())); | ||
provider | ||
.Should() | ||
.ContainSingle(p => p.TaskId == taskId && p.Action == action && p.Authorizer == authorizer.First()); |
Check warning
Code scanning / CodeQL
Reference equality test on System.Object Warning test
this
.ContainSingle(p => p.TaskId == taskId && p.Action == action && p.Authorizer == authorizer.First()); | ||
provider | ||
.Should() | ||
.ContainSingle(p => p.TaskId == taskId2 && p.Action == action && p.Authorizer == authorizer.First()); |
Check warning
Code scanning / CodeQL
Reference equality test on System.Object Warning test
Description
Proposal for solution
Related Issue(s)
ServiceLifetime
#757Verification
Documentation