Copied to clipboard

Flag this post as spam?

This post will be reported to the moderators as potential spam to be looked at


  • James Jackson-South 489 posts 1746 karma points c-trib
    May 28, 2018 @ 10:53
    James Jackson-South
    0

    Improving IFileSystem abstraction in V8

    This thread is prompted by a tweet I made a few days ago while doing some investigation into a customized Azure IFileSystem implementation.

    https://twitter.com/JamesMSouth/status/999487383375527936

    The Problem.

    There are multiple IFileSystem\IFileSystem2 implementations within the Umbraco codebase.

    Method calls to these implementations are not restricted to the interface contract. For example. GetMediaPath() in MediaFileSystem is called from FileUploadPropertyValueEditor

    This is, to me, a problem because IFileSystem should be a contract.

    The purpose of an interface is to ensure that the class fulfills the required functionality allowing a programming style called "programming to the interface."

    To quote Design Patterns: Elements of Reusable Object-Oriented Software:

    The idea behind this approach is to base programming logic on the interfaces of the objects used, rather than on internal implementation details. Programming to the interface reduces dependency on implementation specifics and makes code more reusable.

    If you take this functionality to it's extreme you end up with IOC (Inversion of Control)

    IOC and code re-usability is exactly what a complex solution like Umbraco requires in order to remain extensible and simple to navigate. It improves testability and reduces the number of bugs in a codebase.

    Now... I understand that V7 had to maintain some of the more interesting design decisions due to backwards compatibility concerns but V8 is a fantastic opportunity to improve on what I see as a fundamental design issue which, if, implemented well could dramatically improve the flexibility and overall performance of Umbraco.

    Imagine being able to really simply configure a filesystem for Azure or AWS that just works. Or add your own events, caching, or logging to the filesystem by simply inheriting an existing implementation and adding additional code in the contracted methods.

    The Solution

    Redesign IFileSystem with reuse in mind, reducing the built-in implementation count to a single PhysicalFileSystem, removing any non-contract methods and ensuring that the implementation reflects the single responsibility of managing file IO (input, output). Any calling code should call the interface, not the implementation. Use IOC to wire it all up.

    I'd like to help with this.

    If I had my way this would be the way Umbraco works for much of its codebase - IOC for reusable, interchangeable scoped components with a dramatic reduction in external configuration. (Too many XML files in my opinion for things that we would only change via code).

    If we make the changes now, the eventual change to ASP.NET Core (it's the future, we should be embracing it asap) will be much easier and hopefully much sooner than it appears at present.

    Please add your thoughts below.

    P.S. We should be able to choose V8 here.

  • Morten Bock 1867 posts 2140 karma points MVP 2x admin c-trib
    May 28, 2018 @ 13:23
    Morten Bock
    0

    Hi James

    There was a session on V8 at Codegarden, and they did show that IoC is indeed a part of the core of V8. I do not know to which extent it is used yet, but it is there.

    I agree, that if something like an IFileSystem is exposed for third parties to implement, then the core should only use the interface, to ensure it is actually pluggable.

    With regards to xml config vs code, I think there does need to be a way for people to compose a site out of packages, without having to write any C# code. So that would mean that replacing the FileSystem implementation should be possible from a config file.

  • James Jackson-South 489 posts 1746 karma points c-trib
    May 30, 2018 @ 01:55
    James Jackson-South
    0

    Hey Morten, thanks for pitching in!

    I think there's a limit to configuration and we should be careful to draw the lines. There are different developmental levels within Umbraco some of which should be code based.

    Using configuration files to pass options to code-registered dependencies - yes, registering dependencies via configuration - no.

    You'll see the former pattern often in ASP.NET Core projects, the latter you will not.

    Switching out the entire file system is a fundamental operation and should not be lumped in with config driven exercises like adding new datatypes.

    The amount of configuration parsing and reflection used in Umbraco's API must certainly hurt startup performance. (On that note I think there should be explicit registry code within data types to reduce startup times)

    On the point of IOC, I think going with LightInject is a risky decision, Microsoft's DI package works with .NET Framework projects and is forward compatible.

  • Shannon Deminick 1522 posts 5242 karma points
    May 31, 2018 @ 03:57
    Shannon Deminick
    0

    I'm all for redesigning this in v8. There's no IFileSystem2 in v8, much of it has already been cleaned up. I urge you to clone the v8 code and see what is there currently and how things are setup.

    Stephan has done a considerably amount of work in v8 over the past few years to clean things up and streamline things and I'm sure he has his own opinions of this and how things might work. I myself and not fully familiar with the IFileSystem changes already done in v8 so please have a look to see what is there now.

    On the point of IOC, I think going with LightInject is a risky decision, Microsoft's DI package works with .NET Framework projects and is forward compatible.

    I disagree - MS's DI framework offer the most basic DI functionality but we need more than just basic functionality which is already in use in many places in v8. LightInject is also forward compatible. I don't really see why this is 'risky' but we can argue that point on another thread if necessary.

  • Shannon Deminick 1522 posts 5242 karma points
    May 31, 2018 @ 04:35
    Shannon Deminick
    0

    Just for some background info in case it helps, I'd like to mention how IFileSystem is structured in Umbraco today : IFileSystem is the underlying file system that can be replaced but there are strongly typed file systems like MediaFileSystem that "extends" the interface by wrapping it. As you mentioned MediaFileSystem.GetMediaPath is not part of IFileSystem, but that was by design. Sure we could instead make an interface for a MediaFileSystem but in v6 (it dates way back to then) it was decided to make concrete IFileSystems wrap an underlying swap-able IFileSystem

    So in the current file system config, you can replace the underlying IFileSystem for a given strongly typed file system.

    The purpose is that a strongly typed file system might have specific functionality and that also allows for adding extension methods to the strongly typed file system while wrapping the basic IO IFileSystem implementation for the reading/writing of the actual files.

  • Stephen 767 posts 2271 karma points c-trib
    May 31, 2018 @ 06:45
    Stephen
    2

    Comments:

    I am all (as in, absolutely totally convinced) for structuring the codebase with clear contract interfaces, and then implementations. There are tons of places where we totally don't respect this, today. Just look for all the places where we do:

    public void DoSomething(IContent content)
    {
       var c = (Content) content;
       // ... and then use c everywhere
    }
    

    Now, well, there are tons of these, and we can only cleanup so fast. They are not a choice, there are legacy.

    The IFileSystem thing (yes, IFileSystem2 is gone in v8) is definitively one that I don't like - although, as Shannon mentioned, one need to pay attention that (1) we may want a media "super filesystem" that adds some functionality for dealing with media and (2) there's the whole shadowing thing.

    Totally happy to discuss here proposals for IFileSystem refactoring. Better discuss first, so we all agree on changes.

    On configuration files vs. code: to me it's the same as pure DI vs service locator, obviously I like the "pure" approaches but we need to transition and be friendly. There has to be a way to get things done, quick (and maybe dirty).

    The way it's solved for DI is... v8 is all DI, we try to inject everything, and then there's this Current service locator. Don't use it if you don't like it, but it's there.

    For configuration files vs. code, methink we need a clean code API for configuration (what James really wants ;-) and then, we can totally write a configuration file-parsing thing on top of it for when/if it's needed.

    TL;DR:

    • Totally wanting to cleanup IFileSystem
    • Other things to do before, community can help
    • Discuss target architecture here beforehand
    • Configuration API + file on top of it

    Thoughts?

  • James Jackson-South 489 posts 1746 karma points c-trib
    Jun 05, 2018 @ 03:40
    James Jackson-South
    0
    public void DoSomething(IContent content)
    {
       var c = (Content) content;
       // ... and then use c everywhere
    }
    

    Mein Gott!! That should have come with a warning! :)

    Glad to hear that you are on board.

    @shannon There's a breaking change in MediaFileSystem somewhere in 7.6+ that alters the naming of replaced media files. That has broken my Azure provider in a way I cannot (without help from HQ) fix. This exemplifies, to me, why wrapping in this manner is unwise. (Though I do wish the entire naming convention for media was different and simply meant passing the uid and extension to the IFileSystem).

    Regarding DI, I meant Microsoft.Extensions.DependencyInjection which, as I understand it can be used within classic applications and is fully featured (I've yet to find a limitation in .NET Core projects but I'm no expert so I could be talking crap).

    @stephan Regarding configuration. Yes, I would love something like IUmbracoBuilder that handled (Add\Remove\Replace)(Service\PropertyValueConverter) etc. It's a common pattern in the new fangled way of doing things, I'm using that approach in the ImageSharp.Web middleware and it's just really neat for the consumer.

    Another benefit of course is that if you have a great configuration API you can dramatically reduce the amount of work you need to do at startup (No more looking through dll's to find plugins for starters)

    Adding a configuration file binding API on top of that should be much easier, Off the top of my head, imagine something in your API like:

    // For standard provider setup
    IUmbracoBuilder.ReplaceFileSystem<TFileSystemOld, TFileSystemNew> ()
        where TFileSystemOld : IFileSystem
        where TFileSystemNew : IFileSystem
    

    and

    // For something that requires additional setup on init.
    IUmbracoBuilder.ReplaceFileSystem<TFileSystem>(Function<IFileSystem> setupFunc) 
        where TFileSystem : IFileSystem
    

    where I could then write a plugin extension method that did something like this:

    IUmbracoBuilder UseAzureFileSystem(this IUmbracoBuilder builder,  Action<AzureFileSystemOptions> setupAction)
    {
        // This method in your API would register the options as a singleton. 
        // It could theoretically be used for all options.
        builder.Services.Configure(setupAction);
    
        // We call your registration method now.
        builder.ReplaceFileSystem<PhysicalFileSystem, AzureFileSystem>();
        return builder; 
    }
    

    Calling this would then involve your addition configuration API that could parse XML or Json. This would be super powerful then.

    builder.UseAzureFileSystem(options => this.Configuration.Bind("AzureFileSystemOptions", options));
    

    You could use the same approach for adding IPropertyValueConverter implementation and all sorts.

    Let me know how I can help.

  • Shannon Deminick 1522 posts 5242 karma points
    Jun 05, 2018 @ 05:51
    Shannon Deminick
    0

    There's a breaking change in MediaFileSystem somewhere in 7.6+ that alters the naming of replaced media files. That has broken my Azure provider in a way I cannot (without help from HQ) fix. This exemplifies, to me, why wrapping in this manner is unwise. (Though I do wish the entire naming convention for media was different and simply meant passing the uid and extension to the IFileSystem

    I was only mentioning how it currently works with the wrapping, not how it probably should work ;) Off the top of my head I don't recall the name change thing but happy to help if i can. The naming convention for media must be changed in v8. The integer thing doesn't work well since it's just a simple INT counter which also leads to issues in LB env. There's a flag already in v7/v8 to change this naming convention so that media is stored in GUID based folders that are based on the property ID, so you would always know what the files in a folder belong to. We'll need to flip this switch for v8 and this will also allow us to proxy media better (i.e. we'll store in App_Data and proxy via /media).

    Microsoft.Extensions.DependencyInjection which, as I understand it can be used within classic applications and is fully featured

    It is a basic DI implementation and works well but if you require property injection or custom injection than it doesn't suffice (and we are using some of these techniques). LightInject is forward compatible and integrates with the DI framework for .NET core, it was one of the first to support .NET Core.

    And yup i'm all for the code approach you've listed above and this is how v8 is structure already (more or less). If config files are needed for anything, these should be added/supported later.

  • James Jackson-South 489 posts 1746 karma points c-trib
    Jun 05, 2018 @ 06:47
    James Jackson-South
    0

    Sweet!

    Property Injection though! You scoundrel! :)

  • Shannon Deminick 1522 posts 5242 karma points
    Jun 05, 2018 @ 08:43
    Shannon Deminick
    0

    Yeah i know! I never thought I'd ever have a need for that but it turns out for a couple particular scenarios in v8 it's actually perfect. Who knew :)

  • James Jackson-South 489 posts 1746 karma points c-trib
    Jun 06, 2018 @ 02:02
    James Jackson-South
    0

    Oh I forgot to ask, What does the ShadowFileSystem do? I had a look at the code but couldn't figure it out.

  • James Jackson-South 489 posts 1746 karma points c-trib
    Jun 08, 2018 @ 16:40
    James Jackson-South
    0

    Chaps,

    I've created an issue regarding the DI approach taken in the codebase. It's troubling me and I don't want you to have problems down the line.

    http://issues.umbraco.org/issue/U4-11427

Please Sign in or register to post replies

Write your reply to:

Draft