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

Remove Infrastructure Dependency on Web #54

Draft
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@sdepouw
Copy link
Collaborator

sdepouw commented Feb 21, 2019

  • Fixes #53
  • CleanArchitecture.Web no longer has a dependency on CleanArchitecture.Infrastructure
    • The DLL is copied to the Web's output/bin directory via a post-build command.
  • The Infrastructure project is now referenced by loading the assembly in Startup.cs, by loading the physical file
    • Loading the assembly via Assembly.Load() does not appear to work, since the Web project does not directly reference it
    • AutoFac does not support scanning assemblies for Modules (the AutoFac equivalent of StructureMap's Registry classes) using string assembly names
  • All Infrastructure convention dependencies are still registered via RegisterAssemblyTypes
  • AppDbContext is registered via DatabaseModule, which is referenced in Startup.cs via builder.RegisterAssemblyModules.
@@ -58,14 +51,22 @@ private static IServiceProvider BuildDependencyInjectionProvider(IServiceCollect
// Populate the container using the service collection
builder.Populate(services);

// TODO: Add Registry Classes to eliminate reference to Infrastructure
// Reference all CleanArchitecture assemblies for AutoFac.
Assembly webAssembly = Assembly.GetExecutingAssembly();
Assembly coreAssembly = Assembly.GetAssembly(typeof(BaseEntity));

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

Rather than using BaseEntity as "hook" to reference core assembly, we should consider creating a pattern to reference this kind of assemblies for example by an empty class called CoreReference, InfrastructureReference, InfrastructureOrmReference(just names to address the problem). A person without knowledge about reflection will have a problem to understand why in this case BaseEntity when you will need to add additional Infrastructure's projects and use reflection in another place and you will use a random class from the library.

This comment has been minimized.

@ardalis

ardalis Feb 22, 2019

Owner

I think a comment would suffice. That's what I usually do is specify a class I know to be in the given assembly and then add a comment with the project/assembly name.

Assembly infrastructureAssembly = Assembly.GetAssembly(typeof(EfRepository)); // TODO: Move to Infrastucture Registry

string location = AppDomain.CurrentDomain.BaseDirectory;
Assembly infrastructureAssembly = GetInfrastructureAssembly(location);
builder.RegisterAssemblyTypes(webAssembly, coreAssembly, infrastructureAssembly).AsImplementedInterfaces();

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

Somehow it's possible to add functionality that will register additional interfaces that have custom classes? I mean autofac auto-scanning is able to register interfaces when the classes have different names and you don't want to do this manually?
Manually you can do something like that, but at this moment you are splitting functionality into two places and it can be misleading.

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

In other words, "put all custom registrations here, autofac will handle this for you".

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

Another question is, what about dynamic loading of other subprojects from Infrastructure, for example, we want to split ORM logic into CleanArchitecture.Infrastructure.Orm. And question is, we will add now this functionality?

This comment has been minimized.

@sdepouw

sdepouw Feb 22, 2019

Author Collaborator

Right now there are no custom registrations that need to be made. But to register interfaces whose concrete classes live in Infrastructure, you can create a new Module in the Infrastructure project (or any other project where modules are registered (and as of right now, just Infrastructure is)).

An example module in this PR would be the DatabaseModule, where we register AppDbContext in there instead of in Startup.cs (since we no longer have an Infrastructure reference in Web. You could create your own Module (or just add more to DatabaseModule) to add registrations. A Module in Autofac is similar to a Registry in StructureMap.

Here's a sample of what could be made. builder.RegisterAssemblyModules(infrastructureAssembly); in Startup.cs will automatically scoop it in, if this class is in the Infrastructure project.

public class InfrastructureModule : Module
{
    protected override void Load(ContainerBuilder builder)
    {
        builder.RegisterType<ConcreteThing>().As<IThing>();
    }
}

If you have registrations that are available in the Web project (i.e. can be put in Startup.cs), you can add them directly after the assembly registration lines.

builder.RegisterAssemblyTypes(webAssembly, coreAssembly, infrastructureAssembly).AsImplementedInterfaces();
builder.RegisterAssemblyModules(infrastructureAssembly);

// Add custom registrations here. Or in modules in their respective assemblies.
builder.RegisterType<ConcreteThing>().As<IThing>();

This comment has been minimized.

@sdepouw

sdepouw Feb 22, 2019

Author Collaborator

To answer your question about loading of other dynamic projects, it would just be a matter of repeating the same steps demonstrated here to include the Infrastructure project. Adding the post-build command to copy the DLL to the Web project's output directory, then modify the code in Startup.cs.

// Make "GetOtherAssembly" grab the full file name to the other assembly.
Assembly otherAssembly = GetOtherAssembly(location);

// Add your other assembly to the list of registered assembly types.
builder.RegisterAssemblyTypes(webAssembly, coreAssembly, infrastructureAssembly, otherAssembly).AsImplementedInterfaces();

// If you have a custom Module in your other assembly, add it here too.
builder.RegisterAssemblyModules(infrastructureAssembly, otherAssembly);
return new AutofacServiceProvider(applicationContainer);
private static Assembly GetInfrastructureAssembly(string location)
{
const string infrastructureDll = "CleanArchitecture.Infrastructure.dll";

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

Rather than magic string, you tried this approach?

string infrastructureAssemblyName = AppDomain.CurrentDomain.GetAssemblies()
                .Where(a => a.FullName.Contains("Infrastructure")).Select(x => x.FullName).FirstOrDefault();

With that, we don't need to worry about part CleanArchitecture when we will change the name of the project.

This moment I don't have the possibility to check that AppDomain.CurrentDomain contains .dll of Infrastructure without reference to the web to have 100% sure it's working.

This comment has been minimized.

@ardalis

ardalis Feb 22, 2019

Owner

I agree it would be helpful if whatever approach is used here, it survives renaming of the project and its associated DLLs.

This comment has been minimized.

@sdepouw

sdepouw Feb 22, 2019

Author Collaborator

I actually started by trying to fetch the dynamic infrastructure assembly name via AppDomain.CurrentDomain.GetAssemblies(), but GetAssemblies() doesn't return assemblies that aren't loaded. And because the Web project does not reference the Infrastructure project anymore, it does not get returned from GetAssemblies():

image

After several approaches, referencing the DLL by exact file name was the only way to get the required assembly for AutoFac.

This comment has been minimized.

@Xeinaemm

Xeinaemm Feb 22, 2019

Contributor

Hmm, so then just nameof of the namespace to fail fast.
string infrastructureDll = $"{nameof(CleanArchitecture)}.Infrastructure.dll";

It will be a very big problem if Application Domain will not contain information about Infrastructure, tomorrow I will check something that is related to performance investigation because without that some tools may be broken or inaccurate.

@ardalis

This comment has been minimized.

Copy link
Owner

ardalis commented Feb 22, 2019

Build is failing, probably because it's running on linux not Windows and the build script is using the windows copy command?

/usr/share/dotnet/sdk/2.2.103/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.DefaultItems.targets(153,5): warning NETSDK1071: A PackageReference to 'Microsoft.AspNetCore.App' specified a Version of `2.1.5`. Specifying the version of this package is not recommended. For more information, see https://aka.ms/sdkimplicitrefs [/home/vsts/work/1/s/src/CleanArchitecture.Infrastructure/CleanArchitecture.Infrastructure.csproj]
/usr/share/dotnet/sdk/2.2.103/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.DefaultItems.targets(153,5): warning NETSDK1071: A PackageReference to 'Microsoft.AspNetCore.App' specified a Version of `2.1.5`. Specifying the version of this package is not recommended. For more information, see https://aka.ms/sdkimplicitrefs [/home/vsts/work/1/s/src/CleanArchitecture.Web/CleanArchitecture.Web.csproj]
/home/vsts/work/1/s/src/CleanArchitecture.Infrastructure/CleanArchitecture.Infrastructure.csproj(22,5): error MSB3073: The command "copy /Y "/home/vsts/work/1/s/src/CleanArchitecture.Infrastructure/bin/Release/netcoreapp2.1/CleanArchitecture.Infrastructure.dll" "/home/vsts/work/1/s/src\CleanArchitecture.Web\bin/Release/netcoreapp2.1//CleanArchitecture.Infrastructure.dll"" exited with code 127.
    2 Warning(s)
    1 Error(s)
@sdepouw

This comment has been minimized.

Copy link
Collaborator Author

sdepouw commented Feb 22, 2019

@ardalis yeah looks like it. Digging into the build log I found:

/bin/sh: 2: /tmp/tmp87e8284eeabb412ead5c221728fcb344.exec.cmd: copy: not found

And after Infrastructure builds we indeed call that post-build event:

copy /Y "$(TargetDir)$(TargetName).dll" "$(SolutionDir)src\CleanArchitecture.Web\$(OutDir)\$(TargetName).dll"

It looks like the post-build step to manually copy the Infrastructure DLL to the output directory needs to be updated to a universal copy command of some kind.

@sdepouw

This comment has been minimized.

Copy link
Collaborator Author

sdepouw commented Feb 22, 2019

@ardalis we could also update the build to run against vmImage: 'VS2017-Win2016' instead of vmImage: 'Ubuntu-16.04', unless we're running the Azure DevOps build against Linux for a specific reason?

@Xeinaemm
Copy link
Contributor

Xeinaemm left a comment

I wonder that its appropriate way of thinking about removing infrastructure reference.

We can have two different approaches to building an app with this architecture.

First, self-hosted REST API + self-hosted SPA, that definitely remove reference to the infrastructure and keep all necessary references in place without hacks.

Second, both in bulk as now, but what about the situation when you will add some logic to infrastructure that belongs to configuration or helpers(with 3rd part references) and you need to use them in Web API?
I have this problem because I've created helpers for a data shaping mechanism, HATEOAS that lives inside infrastructure due to 3rd part references and used in both, infrastructure and web.

You can create an adapter to your own facade that hides implementation under core's service interfaces, but this overhead to helpers is appropriate?

As we know, developers are lazy and creating an abstraction over common mechanisms like extensions will be too much for some people.

I don't really see that web should have any additional logic despite display a result or create endpoints because we will just copy and paste this same 3rd part references and it can create friction because it's used in two places(infrastructure and web).

On the other hand, it completely hides all logic under services or repositories, you cannot force to break that and I agree that is the best way of thinking but with some boilerplate code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.