-
-
Notifications
You must be signed in to change notification settings - Fork 38
Resolution of IdentityServerOptions fails with latest version #15
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
Comments
Here is a test that can be added to this project.
Maybe its worth adding logging output to
to output when ex is thrown as it would allow not to set up this project local to debugg. Trying to find more information but looks like something broke with IOptions
Looking for a solution |
@ENikS I think that some of the enumerable changes you recent commmits could be the issue |
I think that if a IEnumeral is requested and no registrations of T is made, then it should return an empty Enumerable ? |
I am at looking into:
(added a few lines to debug) but i dont understand why
has to be called. In this example.
Its resolving a generic interface So basically to return a enumerable of IPostConfigureOptions it is also building up all registrations of IdentityServerOptions ? is that the intended behavior |
btw to run the test, the following dependency is also needed.
|
So, think i found the problem. In the image the debugger shows that the GetFinalType is returning the argument IdentityServerOptions because its registered which is wrong. Trying to figure out if GetFinalType should be rewritten, but I am coming to the conclussion that _isTypeExplicitlyRegistered should have returned true since i would expect it return on this instead |
I hacky fix that solves my test and also keeps existing tests to work would be to add to EnumerableResolveStrategy such it ignores the final type . public override void PreBuildUp(IBuilderContext context)
I am in over my head finding a good solution for this, hoping that the stuff i found do help to get this resolved. |
Lets continue here @ENikS But I think I know what needs to be done now. Cool thing is that we could add it as a extension here when we know the full list of interfaces from aspnet core that needs to be build in. First item is: Microsoft.Extensions.Options.IPostConfigureOptions<> |
We could create a factory for the Options as part of extension. Looks like you know what to do now :) |
Atleast I know the idea, i will go over your examples and see if I can figure it out. Question is if we want to pull in all the additional dependencies or we want to have Unity.Microsoft.DependencyInjection.Configuration that matches future packages in aspnet core I am in favor of adding a extension point for developers to hook in the types that should be build in because when new packages comes they might also have types that needs to be build in. I will try come up with a prototype and get back with the results here. |
Sounds good |
@ENikS I think we need to take a step back and look at the original problem. Please take a moment to reflect over the following before you dismiss it :) First - i did have a go at what we discussed above - but realized that its now the users of 3th party stuff that needs to know what to tell unity that it should use as build in types. This causes alot of trial and error and not just a builder.UseUnityContainer() that one would expect. I am hoping we can take a look at the original problem and see if we can change something here that works for both worlds. Here is an example of a GetFinalType that dont use a hashset and makes all tests passes:
The reason why I keep coming back to this method is that I do think the current implementation is faulty or builds on a bad assumption. I agree that nested IEnumerable<Lazy<Fun<>>> should work - but I dont know if this should be for every generic interface like the current implementation. Would we consider changing the spec to not have tests for Func? Or should it then have tests for Func<>, Func<,>, Func<,,>, Func<,,,> and so on? Maybe custom build plans should be used to bring in the additional generics that should be included in this nested resolution. Moving on, GetFinalType proposed above works with current tests. I will try to add a small example to the web directory that illustrates the problem and hope we might be able to continue a dialog about this maybe needs fixing in Unity.Container rather then the in the consuming project. |
I created the minimal example of issue to conclude on above: https://github.com/pksorensen/examples/tree/issues/nestedEnumerable Notice the service registrations
when navigating to /api/values you will get a
Thats because the exception is absorbed here
in the serviceprovider. Which will be identical to the problem described above. It will all be fixed if Enumerable returned an empty enumerable when T is not registered. Problem is that the new behavior for IEnumerable<Microsoft.Extensions.Options.IPostConfigureOptions> will be faulty due to So i honestly believe this is faulty/bug and not something we should resolve in the web application itself. |
@ENikS Here is a minimal branch with updated unit test to show the issue aswell https://github.com/pksorensen/container/tree/issues/nestedenumerables And sorry if I am doing something stupid with the branches, but I am not that skilled with the submodule in git and how to do pull requests to those. |
You trying to convince me to change GetFinalType and for the most part I agree with you. The problem is, this behavior is carried over from version 2.+ and if we change it A LOT of code will stop working. I know it for a fact, I deal with it at work. Lets try to formulate the issue here. What is it exactly you don't like in present solution? |
I am not that familiar with asp.net |
Would it be possible for you to look at the unit test that I just did her https://github.com/pksorensen/container/tree/issues/nestedenumerables so we both understand the issue. I think it shows the problem very well atleast. The problem at its core is something to do with it trying to resolve IEnumerable<CustomGeneric> where the current implementation will blow up if T is also registered with the container. Resolve IEnumerable<CustomGeneric> should return an empty enumerable when no registrations of CustomGeneric is made. |
This test?
|
Let me take a closer look at what is going on. |
yes, and also change the service provider to throw on exceptions to get it out.
and you need this dependency also |
I usually run it with local build so I could step into Unity code. |
I do the same now that you showed me to clone the submodules properly. But you should be able to step into the ResolveGenericEnumerable and see how it get a bad type argument |
If we cant find out that there is something wrong and dont want to change, then the following extension point can be used
Then in startup of aspnet core apps the extension can be added:
or the unittest
We could add the IOptionsExtension to Unity.Microsoft.DependencyInjection. But this requires the developers to know which options to add extensions for. This can be hard because you as a developer might not know if a dependency uses something that needs to be registered. I would consider this a last resort if you end up at a point where you dont think there is anything wrong with the current implementation - its really hard for me to judge if my purposal of GetFinalType will break stuff for a lot of other people. |
Yes, you are right. It looks like a new NuGet package Unity.Microsoft.Options. |
At the same time we dont want to end up having to create packages Unity.. for everything. That puts alot of extra work on this project. Would be a cleaner approach if unity was able to work in pair with aspnet core dependency injection. Were you able to step though ResolveGenericEnumerable and see how GetFinalType returns a bad type of the unit test i provided? The part that ends up returning a bad type is this:
so I would look at if GetFinalType could be altered before going about creating extra packages. |
This one is worth supporting |
I am working on v6 engine and all this magic will be lost there. It is better to create supporting package. |
Super, I will go with my own enumerable resolution strategy for now to unblock myself and try to contribute in what way I can for getting unity and aspnet core to play nice together. |
I think I made something thats usefull here:
It creats a plan for the OptionsFactory<> in aspnet core options, which is the proplametic type. Are there any way to cache or make this more efficient in the factory metory. caching p1/p2 ect? |
This line caches it so next enumerated item will be resolved by the method you've created
|
ye, but the two dynamic child plans, p1 and p2- used to create the actual new OptionsFactory. You said hashsets was slow, so was just wondering. Normally i would have put the two plans p1 and p2 into a dictioanry based on their typeof TResult to get them next time instead of creating them again |
Your factory method seems to be too complicated. Why don't you do your p1 and p2 in the CreatePlan? |
Do you understand what it does? |
Not exactly. I got stuck here:
Could not figure out how to wrap it all up |
Got to run now, will continue tomorrow... |
:) okay thanks. |
I am having this same issue. Any progress on a resolution? |
Been busy lately. Perhaps @pksorensen could give you temporary workaround? |
Let me try backtrack to what i ended up using. Will update by tomorrow with a answer of my work around. |
I am using this currently:
and
So i can on my |
Now Unity supports Options. |
The following test fails with latest version
the debugger spits this out:
Is it possible to add extension/logging or something that would allow me to inspect those resolution failed exceptions with unity ? trying to figure out what changed atm.
The text was updated successfully, but these errors were encountered: