Skip to content
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

ASP.NET Core facility #376

Merged
merged 1 commit into from Feb 10, 2018
Merged

ASP.NET Core facility #376

merged 1 commit into from Feb 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 27, 2018

This addresses #120

It provides an integration with aspnet core 2.0.

Considerations for this PR:

  • Windsor is a non-conforming container. This facility implements the "2 container" solution.

The vanilla aspnet container consuming "Framework Configured Container" via the IServiceCollection and IApplicationBuilder interfaces.

The windsor integration as the "Application Container". These are well known framework types that serve as integration points for application developers provided by ASPNET Core 2.0.

The "application container" integrates with the "framework configured container" by using a sub dependency resolver(FrameworkConfigurationDependencyResolver). This allows seamless type resolution of framework configured components without having to "cross wire" things.

"Cross wiring" at the time of writing was attempted by SimpleInjector but it led to side effects ie. "Captive Dependencies" and "Torn Lifestyles".

  • This is also a preview of the NUnit Adapter Upgrade.

We have an issue here: #243, which this hopefully addresses in how we run tests, we can switch over to 'dotnet test'. Please see this and this

Would be good if we could roll this out across the board. It will also start exposing the various target frameworks(net45, netcoreapp1.0, netcoreapp2.0) in VS2017/Resharper(latest) which is handy.

These guys both tried to implement the Microsoft Extensions Dependency Injection Abstraction(myself included but gave up). Credit for creating a foundation from which to learn from! :)

Push-AppveyorArtifact $env:APPVEYOR_BUILD_FOLDER\src\Castle.Windsor.Tests\$testBinPath\TestResult_Windsor.xml

Push-AppveyorArtifact $env:APPVEYOR_BUILD_FOLDER\src\Castle.Facilities.WcfIntegration.Tests\$testBinPath\TestResult_WcfIntegration.xml

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really was not adding much value because the log output is enough.

Copy link
Author

@ghost ghost Jan 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was also causing pathing issues. Bad reason for a build to fail in appveyor but pass locally :'(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that NUnit's console output is much better for failures now so we don't need the last 2, however the "/api/testresults/nunit/..." ones are for the "Tests" tab in AppVeyor. Castle Core only has the first ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone look at those test tabs? I dont. Also noticed test output from netcoreapp1.0 would wipe out test results from net45 because of the namespace collisions. Let me know if you use this and I will put it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the test tab is pretty pointless because I don't think you can ever navigate to anything including failures unlikely TeamCity.

It doesn't look like one test run wipes out the other, there are duplicate test names under different file names, Castle.Windsor.Tests.dll (.NET 4.5) and Castle.Windsor.Tests.exe (.NET Core).

But yer I agree let's just get rid of them. Should we also run NUnit's runner with --noresult since the command line output is plenty good?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var handlers = container.Kernel.GetHandlers();
var hasAspNetCoreFrameworkRegistrations = handlers.Any(x => x.ComponentModel.Implementation.FullName.StartsWith("Microsoft.AspNetCore"));
if (hasAspNetCoreFrameworkRegistrations)
throw new Exception("Looks like you have implementations registered from 'Microsoft.AspNetCore'. Please do not do this as it could lead to torn lifestyles. Please remove the registrations.");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably rename the exception message to:

Looks like you have implementations registered from 'Microsoft.AspNetCore'. Please do not do this as it could lead to torn lifestyles. Please remove the registrations from Castle Windsor.

@ghost ghost requested a review from jonorossi January 27, 2018 22:18
@ghost
Copy link
Author

ghost commented Jan 27, 2018

@jonorossi - If you could take a look that would be great.

Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fir3pho3nixx looking good, just a bunch of review comments, some you might have already thought about.


private static void ThrowIfFrameworkComponentsAreRegistered(IWindsorContainer container)
{
var offendingFrameworkTypes = InvalidFrameworkConfigurationNamespaces;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name refers to type names which you prime with namespace names than compare against Type.FullName, obviously FullName returns the namespace but if this should only match the namespace then it should probably look at namespace otherwise should be renamed.

I wouldn't have a problem with this being hardcoded compared against a single string rather than "building for the future".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name refers to type names which you prime with namespace names than compare against Type.FullName, obviously FullName returns the namespace but if this should only match the namespace then it should probably look at namespace otherwise should be renamed.

Will do the namespace option.

I wouldn't have a problem with this being hardcoded compared against a single string rather than "building for the future".

Will move it back to how I had it before. :D

var offendingFrameworkTypes = InvalidFrameworkConfigurationNamespaces;
foreach (var invalidFrameworkConfiguredType in offendingFrameworkTypes)
{
var handlers = container.Kernel.GetHandlers();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should only fetch the handlers once outside the loop.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that is garbage code. Will fix.

var handlers = container.Kernel.GetHandlers();
var hasAspNetCoreFrameworkRegistrations = handlers.Any(x => x.ComponentModel.Implementation.FullName.StartsWith(invalidFrameworkConfiguredType));
if (hasAspNetCoreFrameworkRegistrations)
throw new Exception($"Looks like you have implementations registered from '{string.Join(",",InvalidFrameworkConfigurationNamespaces)}'. Please do not do this as it could lead to torn lifestyles and captive dependencies. Please remove the registrations from Castle.Windsor.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should have surrounding braces, and probably should be broken up on multiple lines rather than 300 or so characters.

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.5" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it now Microsoft's recommendation to reference Microsoft.AspNetCore.All? I would have thought since we are a library we'd want to take a smaller dependency?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can cut this right back.

I just thought it would be handy if you created a console app, then installed Castle.Factilities.AspNetCore you would have everything ready go as a reference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should cut it back.

The ASP.NET team just announced today that they are actually introducing a new Microsoft.AspNetCore.App package and are changing their new project templates not to use ".All" in favour of this one because the "All" metapackage actually brings in heaps of stuff that isn't expected like Azure KeyVault, a Redis client and Sqlite. We really shouldn't force others to bring in packages we don't need for convenience.

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp2.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember something different about netcoreapp2.0 and ASP.NET Core 2 projects, but just wanting to confirm this doesn't block our facility library from being used by another library.

public static void UseCastleWindsorMiddleware<T>(this IApplicationBuilder app, IWindsorContainer container) where T : class, ICastleWindsorMiddleware
{
container.Register(Component.For<T>());
app.Use(async (context, next) => { await container.Resolve<T>().Invoke(context, next); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does calling Invoke directly here rather than returning the middleware cause any problems with the framework? I'm an ASP.NET Core newbie but I just wondered if there was maybe a diagnostic trace that it keeps state of as each middleware is executed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does calling Invoke directly here rather than returning the middleware cause any problems with the framework?

Not that I have come across. Do you have any further links?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it was just a thought as I've never dug into how this stuff works.


using Microsoft.AspNetCore.Http;

public interface ICastleWindsorMiddleware
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this couldn't use IMiddleware? Does ASP.NET Core automatically wire up IMiddlewares not allowing them to be resolved from the container?

It looks like ASP.NET Core already handles this, see the "Per-request dependencies" section, is this functionality useful?

Copy link
Author

@ghost ghost Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this couldn't use IMiddleware? Does ASP.NET Core automatically wire up IMiddlewares not allowing them to be resolved from the container?

Great question. You can definitely do this but you are moving away from the Windsor container and falling back onto "Framework Configured Container". This creates problems if your middleware consumes dependencies that are known to Windsor but not registered within the IServiceCollection. If anyone decided to go down this route they will end up with a mish mash of registrations across both containers. This should be avoided at all costs.

It looks like ASP.NET Core already handles this, see the "Per-request dependencies" section, is this functionality useful?

If their middleware does not consume any dependencies, then this functionality is useful. Otherwise it is recommended you use our extension because then you are free to consume your own types(registered in Windsor) and framework types which is dealt with in the sub resolvers.

I did update the XML documentation for this extension.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still feels icky as it means your application code has to implement a container specific interface otherwise the container won't let you use it. It feels a bit like the lifestyle attributes Windsor has that aren't really recommended as registration/configuration should happen outside the application code, or the anti-pattern that is depending on IKernel from non-factory components.

If a user did implement IMiddleware or used someone else's middleware would ASP.NET Core automatically wire it up, or can we just tell them not to register it with ASP.NET Core and instead register it with the Windsor facility?

I think that the type name (Castle.Facilities.AspNetCore.ICastleWindsorMiddleware) is something that keeps sticking its nose at me. Having the product name in the interface name is something I don't believe we've ever had to do before which sort of tells me we are doing something wrong, especially that Windsor doesn't have middleware but that this is a workaround.

I did update the XML documentation for this extension.

FYI, the interface doesn't have any documentation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still feels icky as it means your application code has to implement a container specific interface otherwise the container won't let you use it.

You are right it is superfluous and can be removed altogether.

If a user did implement IMiddleware or used someone else's middleware would ASP.NET Core automatically wire it up, or can we just tell them not to register it with ASP.NET Core and instead register it with the Windsor facility?

Not aware of anything that wires this up for you.

For framework middleware:

I think that the type name (Castle.Facilities.AspNetCore.ICastleWindsorMiddleware) is something that keeps sticking its nose at me.

Let's delete it. We still need the extension method though right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, ICastleWindsorMiddleware is gone 🎉 .

Yes, we'll still need the extension method, you were saying trying to do it via the framework container will just cause too much pain so happy for it to stay, however I noticed you renamed the IApplicationBuilder extension method:

app.UseCastleWindsorMiddleware<CustomMiddleware>(container);
... to ...
app.UseResolvableMiddleware<CustomMiddleware>(container);

I wonder if this new name is very clear, if you didn't want "Windsor" in the extension method name (which I thought was convention for these IApplicationBuilder thingys) then naming it just UseMiddleware like the framework one would explain just as much but you'd have an overload with the container.

app.UseMiddleware<CustomMiddleware>(); // register with framework
app.UseMiddleware<CustomMiddleware>(container); // register with Windsor

Or maybe name it app.UseMiddlewareViaWindsor<CustomMiddleware>(container)?

{
container.Register(Classes.FromAssemblyInThisApplication(typeof(TStartup).Assembly).BasedOn<Controller>().LifestyleScoped());
container.Register(Classes.FromAssemblyInThisApplication(typeof(TStartup).Assembly).BasedOn<ViewComponent>().LifestyleTransient());
container.Register(Classes.FromAssemblyInThisApplication(typeof(TStartup).Assembly).BasedOn<TagHelper>().LifestyleTransient());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your controllers are in a different assembly do you want people register their components themselves and skip UseCastleWindsor? You mentioned in the docs if they wanted different lifestyles but not if they are in different assemblies.

XML comments on these with what they specifically do would help people saving having to jump to the docs or into the code.

Copy link
Author

@ghost ghost Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your controllers are in a different assembly do you want people register their components themselves and skip UseCastleWindsor? You mentioned in the docs if they wanted different lifestyles but not if they are in different assemblies.

I updated the docs to be a bit clearer.

XML comments on these with what they specifically do would help people saving having to jump to the docs or into the code.

Also added some XML comments.

9bc8b07


public static void AddCastleWindsor(this IServiceCollection services, IWindsorContainer container)
{
services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see IHttpContextAccessor used in the facility, just wanting to understand what it is needed for.

Push-AppveyorArtifact $env:APPVEYOR_BUILD_FOLDER\src\Castle.Windsor.Tests\$testBinPath\TestResult_Windsor.xml

Push-AppveyorArtifact $env:APPVEYOR_BUILD_FOLDER\src\Castle.Facilities.WcfIntegration.Tests\$testBinPath\TestResult_WcfIntegration.xml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that NUnit's console output is much better for failures now so we don't need the last 2, however the "/api/testresults/nunit/..." ones are for the "Tests" tab in AppVeyor. Castle Core only has the first ones.

@generik0
Copy link
Contributor

generik0 commented Jan 28, 2018

HI.
Great faciltiy. Just what i was needing.

Could it be interesting to add the Microsoft.Extensions.Logging as a resolver (or something) into the faciltiy? I know this would mean a dependancy on the Microsoft.Extensions.Logging but it is inbuilt and automatically registerd with aspnetcore.

public class LoggerResolver : ISubDependencyResolver
    {
        private readonly ILoggerFactory _loggingFactory;

        public LoggerResolver(ILoggerFactory loggingFactory)
        {
            _loggingFactory = loggingFactory;
        }

        public bool CanResolve(CreationContext context, ISubDependencyResolver contextHandlerResolver, ComponentModel model,
            DependencyModel dependency)
        {
            return dependency.TargetType == typeof(ILogger);
        }

        public object Resolve(CreationContext context, ISubDependencyResolver contextHandlerResolver, ComponentModel model,
            DependencyModel dependency)
        {
            return _loggingFactory.CreateLogger(model.Name);
        }
    }

@ghost
Copy link
Author

ghost commented Jan 29, 2018

@generik0

We are using the following meta NuGet package:

<PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.5" />

Had a quick look and this did include:

<PackageReference Include="Microsoft.Extensions.Logging" Version="2.0.0" />

In my original repo I also managed to inject LoggingFactory into a controller as a test.

public HomeController(IUserService userService, ILoggerFactory loggerFactory, IViewBufferScope viewBufferScope)
{
	this.userService = userService;
	this.loggerFactory = loggerFactory;
	this.viewBufferScope = viewBufferScope;
}

Code is over here: https://github.com/fir3pho3nixx/Windsor.AspNetCore/blob/master/WebApp/Controllers/HomeController.cs#L15

You should be able to easily inject your framework types without having to register them in Windsor or using the sub dependency resolver. If this is not happening or not working I would love to see an example so I can fix it.

@generik0
Copy link
Contributor

generik0 commented Jan 29, 2018

@fir3pho3nixx
Yes i know i can inject the factory. Stranely i did have to add the nuget package (Microsoft.Extensions.Logging) into the webapp to get it to work.

It is good to inject the factory, but even easiet to inject the ILogger directly. Hence, a good reason to add the resolver in your apnetcore Faciltiy. So you can allows inject a Microsoft.Extensions.Logging.ILogger out of the box.
Allowing for:

public HomeController(IUserService userService, ILogger logger, IViewBufferScope viewBufferScope)
{
	this.userService = userService;
	this.logger= logger;
	this.viewBufferScope = viewBufferScope;

	this.logger.LogDebug("Log something baby");
}

Anyway, just an idea. Up to you.... Why inject the factory, with the resolver can inject the logger :-)

@ghost
Copy link
Author

ghost commented Jan 29, 2018

@generik0 - Let me check this and get back to you. It should support ILogger.

@generik0
Copy link
Contributor

generik0 commented Jan 29, 2018

@fir3pho3nixx

If i write:

public HomeController(IUserService userService, ILoggerFactory loggerFactory, IViewBufferScope viewBufferScope)
{
	this.userService = userService;
	this.loggerFactory = loggerFactory;
	this.viewBufferScope = viewBufferScope;
	this.logger = this.loggingfactory.CreateLogger(this.GetType()); 
	this.logger.LogDebug("Log something baby");
}

Then i get a logger, you need to pass the caling type to the Factory to get the correct output in your log..
Hence i made the resolver to add to your facilitet :-)

If you want to include this feature i can easily make pull request with the code. :-)

Again, i would think it is usefull, and i would personly think it is good to have in the facility, and it fits with good SOLID and IoC practices to have the resolver in your factory :-)

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?tabs=aspnetcore2x

@ghost
Copy link
Author

ghost commented Jan 29, 2018

@generik0

Can you try again with the code here https://github.com/fir3pho3nixx/Windsor.AspNetCore. I have enabled the delegation of open/closed generics. This now allows constructors like this to be created with framework configured dependencies like:

public HomeController(IUserService userService, ILoggerFactory loggerFactory, ILogger<HomeController> logger, IViewBufferScope viewBufferScope, IOpenGenericService<ClosedGenericTypeParameter> closedGenericService)
{
	this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
	this.userService = userService ?? throw new ArgumentNullException(nameof(userService));
	this.loggerFactory = loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory));
	this.viewBufferScope = viewBufferScope ?? throw new ArgumentNullException(nameof(viewBufferScope));
	this.closedGenericService = closedGenericService;
}

Following behaviours are supported:

  • ILogger'T is an open generic service. Resolves. Think this fixes IOptions too.
  • IOpenGenericService<ClosedGenericTypeParameter> is closed generic service. Resolves.

Most of the change was here

If there are any more glaring obvious things you can spot, by all means PR away to my forked remote branch. Thanks for helping us find this. Will commit this change with tests tomorrow night :)

** Edited **

Good catch!

@generik0
Copy link
Contributor

@fir3phonixx
That’s great. Always nice with a generic resolver.
I have to admin though. I am to lazy to add a generic to a constructor injected object with the same type that the main class is. I.e. you wrote HomeController twice, the name of the class and the in the ILogger. This will really irritate me in the long run.

the resolver should do all the work for me.
Hence, I personally believe the loggerresolver I wrote above would be better to add.
No stress though, I can always add it myself in my own code :-)

@ghost
Copy link
Author

ghost commented Jan 30, 2018

Yup, I could see how that could be irritating.

I have added your resolver here: 0d46f46#diff-6e2e807723a1993c52dc81571e69c073

Thanks!

@jonorossi
Copy link
Member

I have to admin though. I am to lazy to add a generic to a constructor injected object with the same type that the main class is. I.e. you wrote HomeController twice, the name of the class and the in the ILogger. This will really irritate me in the long run.

I agree, ILogger<HomeController> logger just looks strange and prone to error, glad you guys are looking at changing that.

@ghost ghost deleted a comment from jonorossi Jan 31, 2018
@ghost
Copy link
Author

ghost commented Jan 31, 2018

@jonorossi - This is now ready for one final check.

@ghost
Copy link
Author

ghost commented Feb 1, 2018

@jonorossi - I also double checked there were no undue memory leaks. I even introduced console and debug logging. I did find the fairness from the CPU/IO was a bit weird with netcore apps.

I also introduced IDisposable on the AspNetUserService just to be sure burdens and scopes are behaving themselves.

This was with the async ICastleWindsorMiddleware

@ghost
Copy link
Author

ghost commented Feb 1, 2018

Should check this on ubuntu linux before merge.

@jonorossi
Copy link
Member

Should check this on ubuntu linux before merge.

I guess that should be another pull request since Windsor doesn't have a Travis CI build like Castle Core.

@@ -43,6 +43,7 @@ In addition to the above, as part of Castle Project, some other facilities are p
* [System Web Facility](systemweb-facility.md) - Provides system web integration for web projects using `PerWebRequest` lifestyles.
* [AspNet Mvc Facility](aspnetmvc-facility.md) - Provides aspnet mvc integration for web projects using Windsor.
* [AspNet WebApi Facility](aspnetwebapi-facility.md) - Provides aspnet webapi integration for web projects using Windsor.
* [AspNet Core Facility](aspnetcore-facility.md) - Provides aspnet core integration for web projects using Windsor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ASP.NET"

<PropertyGroup>
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
<PackageId>Castle.Facilities.AspNetCore</PackageId>
<Title>Castle Windsor AspNet Core facility</Title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ASP.NET Core"

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore" Version="2.0.1" />
<PackageReference Include="Microsoft.AspNetCore.Mvc" Version="2.0.2" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.Razor" Version="2.0.2" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this facility needs ASP.NET Core MVC and Razor, and not just the main ASP.NET Core package. I haven't had a chance to check out the work the Nancy guys have done to get Nancy running on ASP.NET Core but I thought this facility might be a drop in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Razor is there because of the activator for TagHelpers.

Mvc is there because of the activator for Controllers.

Open to any idea's that could make this easier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to any idea's that could make this easier.

I've got no ideas, I think just leave this as is for now. Nancy is different with its container usage anyway wired through the bootstrapper so let's get this going and the first person to want Windsor on Nancy can sort that one out.

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ Breaking changes:
- Created Castle.Facilities.AspNet.SystemWeb facility so we can remove this from the Windsor core library. (@fir3pho3nixx, #283)

New Features:
- Created Castle.Facilities.AspNetCore facility to support Mvc web applications on dotnet core. (@fir3pho3nixx, #120)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get a chance to try this out using ASP.NET Core on .NET Framework?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, feel free to just log an issue so we don't forget for v5 and defer it for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did check it for all of about 2-3 minutes before moving on to something else. Let me get back to you on this one. Let's hold out before raising the issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonorossi - I looked at this again, it is definitely possible to run ASP.NET Core targeting net461.

The Visual Studio WebApp template appears to have a NuGet reference to Microsoft.AspNetCore.All which misleadingly targets netcoreapp2.0 instead of netstandard2.0. Once I added in the individual NuGets I needed it ran without any issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Visual Studio WebApp template appears to have a NuGet reference to Microsoft.AspNetCore.All which misleadingly targets netcoreapp2.0 instead of netstandard2.0. Once I added in the individual NuGets I needed it ran without any issues.

The Microsoft.AspNetCore.All package by design is .NET Core only, I think also one of the reasons they have introduced Microsoft.AspNetCore.App.

@jonorossi
Copy link
Member

@fir3pho3nixx sorry for sitting on your comments the last few days, I'll try to be more responsive. A few minor issues but that middleware interface is still a sticking point for me. We are nearly there 🤞.

@ghost
Copy link
Author

ghost commented Feb 5, 2018

@fir3pho3nixx sorry for sitting on your comments the last few days, I'll try to be more responsive.

Don't worry.

A few minor issues but that middleware interface is still a sticking point for me.

Glad it is. I can delete the interface. Let me get those changes in tomorrow night and then we can evaluate what else we need then.

We are nearly there 🤞.

Time to squish me thinks.

@jonorossi
Copy link
Member

Time to squish me thinks.

Yep. Let me know how .NET Framework goes.

@jonorossi jonorossi changed the title Facilities aspnetcore ASP.NET Core facility Feb 6, 2018
@jonorossi jonorossi added this to the v5.0 milestone Feb 6, 2018
@generik0
Copy link
Contributor

generik0 commented Feb 6, 2018

Hi

I am using it on .Net Framework. I.e. MVC6 aspnet5 aka aspnetcore. I have copied the files over.
Project just starting so there is not much in it, but the framework seams to be integrating nicely...

@ghost
Copy link
Author

ghost commented Feb 28, 2018

@generik0

I had a look at injecting IHttpContextAccessor as a service and could not find any problems.

This is where I registered it and this is where I resolved it. All runs fine.

Do you have a repro?

@generik0
Copy link
Contributor

generik0 commented Mar 7, 2018

Hi

Are we should there should be dependancies on AspNetCore.Mvc.Razor ?
A simple WebApi project does not use razor.

From WebApi template (warnings are because i have not built yet):
image

@ghost
Copy link
Author

ghost commented Mar 8, 2018

@generik0 - The razor dependency is there for view components and tag helpers. Don’t think we can remove it if we want to support MVC.

@generik0
Copy link
Contributor

generik0 commented Mar 8, 2018

@fir3pho3nixx I know why it is there. I am just thinking if we could do it differently.
even though it wont help the dependancies, Autofac e.g. has sepater helpers to register controlleres, accessor etc:
Should we always register the controllers, helpers and everything, or is it better to split?
Maybe both possibilites could be provided. I could extend to ahve individual registrations also?
image

@ghost
Copy link
Author

ghost commented Mar 8, 2018

@generik0 - the castle windsor community has been waiting for so long for an aspnet core facility, I have personally been researching this problem for over a year now. Before we prematurely optimise this by applying separation of concerns perhaps we should make the entry easy breezy before we start breaking this out into different NuGet packages.

I am not saying you are wrong, you are right. What I am saying is, let's keep it slim to start(which might mean breaking some SoC rules) and let the community decide as we go. What you are suggesting creates more documentation, more NuGet's and honestly this is not something I want for people out there as a first go. The templates for ASP.NET Core that come with VS2017 are rubbish, but that is where people start and unfortunately it includes references to meta NuGet packages like Microsoft.AspNet.Core.All.

The barrier to entry should be kept low. We can evolve this later on. Not even Microsoft has solved this problem fully yet. Look forward to your response.

@generik0
Copy link
Contributor

generik0 commented Mar 8, 2018

I agree, we have been waiting a long time. So long I was forced to use autofac in a project :-:

@generik0
Copy link
Contributor

generik0 commented Mar 8, 2018

Hi. Should @Inject be working on the views?
I have one special place needed :-/

@ghost
Copy link
Author

ghost commented Mar 9, 2018

Yes it should! I will check this out before we release. Good catch!

@generik0
Copy link
Contributor

generik0 commented Mar 9, 2018

It is one of web college that has used it.
It was failing for him. I will look at it tonight, it very might not be castle, but rather him...
I will get back to you ASAP!

@generik0
Copy link
Contributor

generik0 commented Mar 9, 2018

Hi @fir3pho3nixx
There does appear to be an issue with a custom regisered service that can be injected into a controller. It is not working in the view with @Inject. However a MS standard is e.g. IHtmlLocalizer :

image

image

image

@generik0
Copy link
Contributor

generik0 commented Mar 9, 2018

Hi @fir3pho3nixx
I have spent a couple of hours on it. Even is i register the custom service and class for the service, i.e. service.AddSingleton<x, ix> it doesn't work...

Any ideas. I am at a loss...

@generik0
Copy link
Contributor

generik0 commented Mar 9, 2018

Can't do a workaround and inject into the model property either:

image
image

@ghost
Copy link
Author

ghost commented Mar 9, 2018

@generik0 - I will have a look. Give me 1/2 days I will get back to you.

@generik0
Copy link
Contributor

generik0 commented Mar 13, 2018

@fir3pho3nixx Any luck? I have been trying to figure it out also. It gives no meaning when e.g. IHtmlLocalizer can be @Inject

@ghost
Copy link
Author

ghost commented Mar 14, 2018

I need a bit more time on this one, there is no tidy way of achieving this. There no activators and classes in this area are all sealed. When I get back to London I will see if I can implement a custom razor feature that does this.

@generik0
Copy link
Contributor

ok, No problem
I dont think the razorpage is such hi priority. I have not tested with Autofac either. This was the old asp.net way to inject into a view (i.e. though the model. But the direct injection of a interface. i.e. @Inject ICqrsDispatcher Dispatcher on the view, is very important.

Maybe we could do a xp session together and look into it if you like?

@ghost
Copy link
Author

ghost commented Mar 15, 2018

Will have to wait until I get back to London. Currently in Japan. Will email you when I am back to set something up 👍

@ghost
Copy link
Author

ghost commented Mar 26, 2018

@generik0

I have investigated this extensively over the weekend and came to the conclusion that the only way we can achieve this is by implementing a new "Razor Feature/Directive".

I followed the path of how the existing inject directive works:
https://github.com/aspnet/Razor/blob/2f79b90af5cbd9499f012c204f5bf4bb413ae61b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorExtensions.cs#L20

Any extensibility like this is offered through the RazorViewEngineOptions:
https://github.com/aspnet/Mvc/blob/dfa085afaf4ddb0b1063eed35e070d6189acaca2/src/Microsoft.AspNetCore.Mvc.Razor/RazorViewEngineOptions.cs

Unfortunately there is no interface for adding new Razor directives, there is also simply no public API for getting hold of the IRazorEngineBuilder either(which allows the adding of these features) when the IRazorEngine get's new'ed up. Specifically something like this:
https://github.com/aspnet/Razor/blob/0c6ec099584798b5222b669633604269c75d5f15/src/Microsoft.AspNetCore.Razor.Language/RazorEngine.cs#L27

This kind of leaves in a place where we have to create a custom RazorViewEngine that emulates the existing features/directives of Microsoft plus the additional one for doing Castle Windsor compliant @Inject's.

After raising this issue here:
aspnet/Mvc#7540

I also came to the conclusion that the tooling around this would lag behind and also produce errors when it really shouldn't.

To make the @Inject work with the project in it's current form, you would have to cross wire your dependencies into the ServiceCollection. This is yukky and I would never recommend people go down this path.

Here is also an issue I found with how SimpleInjector have approached this:
simpleinjector/SimpleInjector#362

I am interested to know how static service location would solve the problem, have you tried this?

@generik0
Copy link
Contributor

Hi. Great work!. Sorry it ended a little in a dead end. I guess We know now why autofac retur an IServiceCollector og the service init og startup. They re wrote the IServiceCollector, which i agere is not a viable solution.

Thanks for the simple injector link. Are you saying That this Will work:

services.AddScoped(provider => container.GetInstance());

Right now using the view bag, so anything would be better!!!

@generik0
Copy link
Contributor

This really is bad design by ms aspnetcore

@generik0
Copy link
Contributor

HI @fir3pho3nixx
This seams to work. Its fine / great.! No problem at all. I tried with an installer, but this did not work. But i ended up with this:

Questions:

  • Would it be better doing this as middleware?
  • As i see it one would always need these extra items. Maybe we should add as options the the "AddCastleWindsor" to make it easier for people. I will make a PR.

image

image

@franklevering
Copy link

franklevering commented Apr 14, 2018

Hi, I have been playing around with this new facility and I am experiencing strange problems when deploying a dotnet core application to a Windows VM. Whenever I am running my application on my local machine, I don't experience any errors. But on a Windows VM (hosted on IIS) I am getting a ComponentNotFoundException.

ComponentNotFoundException: No component for supporting the service Namespace.Controllers.ValuesController was found Castle.MicroKernel.DefaultKernel.Castle.MicroKernel.IKernelInternal.Resolve(Type service, IDictionary arguments, IReleasePolicy policy) Castle.Facilities.AspNetCore.AspNetCoreMvcExtensions+<>c__DisplayClass0_0.<AddCustomControllerActivation>b__0(ControllerContext context) Castle.Facilities.AspNetCore.Activators.DelegatingControllerActivator.Create(ControllerContext context) Microsoft.AspNetCore.Mvc.Controllers.ControllerFactoryProvider+<>c__DisplayClass5_0.<CreateControllerFactory>g__CreateController|0(ControllerContext controllerContext) Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted) Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker+<InvokeInnerFilterAsync>d__14.MoveNext() System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker+<InvokeNextResourceFilter>d__22.MoveNext() System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker+<InvokeFilterPipelineAsync>d__17.MoveNext() System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker+<InvokeAsync>d__15.MoveNext() System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNetCore.Builder.RouterMiddleware+<Invoke>d__4.MoveNext() System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware+<Invoke>d__7.MoveNext()

My Startup.cs looks the following:

public class Startup
    {
        private readonly WindsorContainer container = new WindsorContainer();
        public Startup(IConfiguration configuration)
        {
            Configuration = configuration;
        }
        public IConfiguration Configuration { get; }

        public void ConfigureServices(IServiceCollection services)
        {
            services.AddMvc();
            services.AddCastleWindsor(container);
        }

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            RegisterApplicationComponents();

            app.UseMvc();
        }

        private void RegisterApplicationComponents()
        {
            container.Install(new ContainerInstaller(Configuration));
        }
    }

ContainerInstaller.cs

 public class ContainerInstaller : IWindsorInstaller
    {
        private Microsoft.Extensions.Configuration.IConfiguration _configuration;

        public ContainerInstaller(Microsoft.Extensions.Configuration.IConfiguration configuration)
        {
            _configuration = configuration;
        }

        public void Install(IWindsorContainer container, IConfigurationStore store)
        {
            container.Register(Component.For<IBaseRepository>().ImplementedBy<MongoDBRepository>().LifestyleTransient());
            container.Register(Component.For<IConfiguration>().ImplementedBy<MongoConfiguration>().LifestyleTransient());

            container.Register(Component.For<NamespaceService>().LifestyleTransient());

            container.Register(Component.For<Namespace.PersonResolver>().LifestyleTransient());
            container.Register(Component.For<Namespace.ChildResolver>().LifestyleTransient());
            container.Register(Component.For<Namespace.HouseResolver>().LifestyleTransient());
            container.Register(Component.For<Namespace.IncomeResolver>().LifestyleTransient());

            container.Register(Component.For<NamespaceResponseConverter>().LifestyleTransient());
            container.Register(Component.For<Namespace.CalculationConverter>().LifestyleTransient());

            container.Register(
                Component.For<IMapper>().UsingFactoryMethod(x =>
                {
                    return new MapperConfiguration(c =>
                    {
                        c.ConstructServicesUsing(container.Resolve);
                        c.AddProfile<Namespace.Mapper>();
                    }).CreateMapper();
                }));

I am not sure if this is a Castle Windsor issue, but when hosting it on IIS on my local machine I don't experience the same error. I was hoping one of you would be able to help me out.

Update

I figured out it has to do with the publish configuration. When running the application in Release mode it seems to fail. When running it in Debug mode I don't experience any issues.

@ghost
Copy link
Author

ghost commented Apr 14, 2018

Any chance you could check it against https://github.com/fir3pho3nixx/Windsor/tree/aspnet-core-windsor-final

I do know we have a memory leak at the moment.

@ghost
Copy link
Author

ghost commented Apr 14, 2018

@franklevering
Copy link

Let me try that first before creating an issue. Thanks!

@franklevering
Copy link

@ghost ghost deleted the facilities-aspnetcore branch June 22, 2018 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants