Using try add for shape attribute provider

[ns8482e]

Using try add for shape attribute provider to add only once

[hishamco]

Is there any issue with AddScoped()?

[jtkech]

Normally in a given module we don't call twice AddShapeAttributes() for a given shape type.

Otherwise we would need to also use TryAddScoped() here but it may not work when we pass a factory delegate.

Maybe we could have our own TryAddScoped() that would be used before line 33 and that would return a bool, so that if it returns false we would not re-register this IShapeAttributeProvider too.

[ns8482e]

Technically it's not issue as it's same class registered more than once and only last one is used. However registration is needed only once.

Useful when having shared shape library not attached to any module , that adds such shapes to service collection, and can be safely called more than once from different modules

[jtkech]

@ns8482e

What I meant is that we could also prevent to re-register

services.AddScoped<IShapeAttributeProvider>(sp => sp.GetService<T>());

But TryAddScoped() only checks the service type, here IShapeAttributeProvider;

So what you could use is TryAddEnumerable(), was not sure so I looked at the dotnet code (see below), because it also checks the implementation type, anf if it is an implementation factory as here, it also checks the return type of the factory.

services.TryAddEnumerable(
    ServiceDescriptor.Scope<IShapeAttributeProvider>(sp => sp.GetRequiredService<T>()));

https://github.com/dotnet/runtime/blob/231255dac0fe015ebc6fb55466c75cf5b4e05f56/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs#L451

https://github.com/dotnet/runtime/blob/231255dac0fe015ebc6fb55466c75cf5b4e05f56/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs#L104

[ns8482e]

Updated to use TryAddEnumerable

[jtkech]

Oops but unit tests are failing

System.ArgumentException : Implementation type cannot be 'OrchardCore.DisplayManagement.Descriptors.IShapeAttributeProvider' because it is indistinguishable from other services registered for 'OrchardCore.DisplayManagement.Descriptors.IShapeAttributeProvider'. (Parameter 'descriptor')

I will check it, maybe just use a cast

ServiceDescriptor.Scoped<IShapeAttributeProvider>(sp => (T)sp.GetRequiredService<T>()));

[jtkech]

Okay, should be

services.TryAddEnumerable(
    ServiceDescriptor.Scoped<IShapeAttributeProvider, T>(sp => sp.GetRequiredService<T>()));

I will update it.

[Skrypt]

Any perf degradation?

[jtkech]

If AddShapeAttributes<T>() is called once per shape type it is as before.

If called multiple times for a given shape type, less registrations, so better for perf.

[Skrypt]

I meant, does the reflection here makes it slower on an equivalent request?

[jtkech]

There is no additional reflection, just a generic extension helper as before.