WIP - Initial help/work to get this working for V8.0.0 #135

warrenbuckley · 2019-01-01T20:03:01Z

Hiya @Jeavon @JimBobSquarePants @lars-erik
This is a WIP PR where I will try & find some time in the evening's or during lunchtime hacks, to get this package re-targeted for V8.0.0

Will post updates/notes as I go along...

… order to update references in projects

…V8.0.0

…l TODO/implement

Jeavon · 2019-01-02T10:18:06Z

@warrenbuckley great stuff! I was discussing with @zpqrtbnk a week before the holidays about what changes would be needed to port this over. If there's anything I can do to support/test please let me know!

Do final fixes to get this to compile - still need to implement couple of things

warrenbuckley · 2019-01-02T10:24:58Z

@Jeavon it will probably be test/tidy up my initial work - it currently compiles but I need to head off for today.

…so updating tests to reflect this. Note: slight concern about non media usages of this provider such as UmbracoForms

warrenbuckley · 2019-01-02T18:19:09Z

Seems like we are close @Jeavon and I can add my first set of notes to my working document for common package upgrade notes for developers.

warrenbuckley · 2019-01-02T21:36:48Z

Think the ZIP package won't work for now as think we (HQ) have not done much work with packages in V8 Core as of yet nor know whats planned for it.

JimBobSquarePants · 2019-01-03T04:52:52Z

Man that Current stuff gives me the heebie-jeebies.

warrenbuckley · 2019-01-03T13:41:25Z

@JimBobSquarePants feel free after I submit this to use the DI Container instead - but for quickness & to help easily port stuff then Current is the best/easiest way to go.

But please lets not get into a DI debate etc here. I just wanted to help port this over as quickly as possible so that it will continue to work on V8 when it gets released.

Seems perfectly valid to return a not implemented for the new AddFile method - due to the bool CanAddPhysical is false/not implemeted as well

lars-erik · 2019-01-03T15:17:52Z

I'm not sure if @zpqrtbnk got around to filesystems yet, but it's hopefully being tweaked more. I'm making a note to revisit this. In the mean time, Current could be centralized to a bastard injection ctor to make it as seamless as possible when time comes. IE. just delete a ctor. ;)

warrenbuckley · 2019-01-03T17:48:48Z

I am not sure - will ask @zpqrtbnk when I am back at work officially on Monday to see if there is more tweaks to be done to IFileSystems

Like I said for now I will use Current as it compiles and this all works. If the three of you wanna tweak & improve it, after you merge it in then please do so :)

The only thing that is problematic is the ZIP based package & the post configuration view

JimBobSquarePants · 2019-01-06T23:34:46Z

src/UmbracoFileSystemProviders.Azure.Installer/packages.config

-  <package id="Microsoft.AspNet.WebApi.Core" version="4.0.30506.0" targetFramework="net45" />
-  <package id="Microsoft.AspNet.WebApi.WebHost" version="4.0.30506.0" targetFramework="net45" />
-  <package id="Microsoft.AspNet.WebPages" version="2.0.20710.0" targetFramework="net45" />
+  <package id="LightInject" version="5.1.2" targetFramework="net472" />


Are these explicitly required? I'm seeing a lot of references here that I would expect to be implicit.

JimBobSquarePants · 2019-01-06T23:37:28Z

src/UmbracoFileSystemProviders.Azure.Tests/packages.config

-  <package id="Microsoft.AspNet.WebApi.WebHost" version="4.0.30506.0" targetFramework="net45" />
-  <package id="Microsoft.AspNet.WebPages" version="2.0.20710.0" targetFramework="net45" />
+  <package id="LightInject" version="5.1.2" targetFramework="net472" />
+  <package id="LightInject.Annotation" version="1.1.0" targetFramework="net472" />


As above. Lot's of new explicit references.

JimBobSquarePants · 2019-01-06T23:38:50Z

src/UmbracoFileSystemProviders.Azure/AzureFileSystem.cs

+
+            if (blockBlob != null)
+            {
+                return blockBlob.Properties.Length;


Properties can be null can it not?

JimBobSquarePants · 2019-01-06T23:45:21Z

I do wonder whether this should be an independant package for V8?

If I were redesigning the IFileSystem interface I would make an explicit effort to design the shape of the API with other implementations (Azure, AWS etc) in mind, checking first to see if the API is both possible and efficient to manage.

CanAddPhysical seems an odd addition to me for example. That's an implementation detail not an API one imo.

https://github.com/JimBobSquarePants/UmbracoFileSystemProviders.Azure/pull/135/files#diff-255650a04f6557f1ccf449b4c809e544R153

https://github.com/umbraco/Umbraco-CMS/blob/8ce50ca357e610222ae3201f10965f0c39bf3932/src/Umbraco.Core/IO/IFileSystem.cs#L160

JimBobSquarePants · 2019-01-08T23:58:39Z

This is all good except for the many "Umbraco implementors" that use this package it's going to be too complicated especially with many Umbraco Cloud installations etc... So the idea is to create 2 additional packages "Our.Umbraco.FileSystemProviders.Azure.MediaFileSystem" & "Our.Umbraco.FileSystemProviders.Azure.UmbracoFormsFileSystem", these will depend on "Our.Umbraco.FileSystemProviders.Azure" and essentially all they do is implement IComposer for the relevant FileSystem, these could also come with a configuration UI similar to the current installer.

What?? I don't understand why there should be any additional complexity at all. It all smells of some unfortunate architectural decisions in Umbraco. There's no way I want to suddenly maintain multiple projects.

V8 was supposed to make things simpler!!

Here's how it should work:

IFileSystem should be the basic crud implementation. IMediaFileSystem, Forms and anything else that needs to persist files should be loading the IFileSystem via constructor injection and utilizing that for all crud operations. There should be no complexity as IFileSystem should be designed in a manner to cover all basic required operations.

Our package should implement IFileSystem only. It shouldn't care about the media implementation or anything else higher up. If it suddenly needs to then there's something fundamentally wrong.

JimBobSquarePants · 2019-01-09T00:01:30Z

src/UmbracoFileSystemProviders.Azure/FileSystemVirtualFile.cs

@@ -68,7 +69,7 @@ public override Stream Open()
                // Add Accept-Ranges header to fix videos not playing on Safari
                HttpContext.Current.Response.AppendHeader("Accept-Ranges", "bytes");

-                IFileSystem azureBlobFileSystem = FileSystemProviderManager.Current.GetUnderlyingFileSystemProvider("media");
+                IFileSystem azureBlobFileSystem = Current.MediaFileSystem.Unwrap();


This looks absolutely mental to me. Unwrap()? The IFileSystem should be injectable.

zpqrtbnk · 2019-01-09T07:52:54Z

@JimBobSquarePants This is an area which we've tried to simplify, but not sure we're there yet.

It goes like this. IMediaFileSystem is implemented by MediaFileSystem which requires an underlying IFileSystem - the one that by default is a PhysicalFileSystem rooted at ~/media but can be swapped, for instance, with the Azure Blob filesystem.

All this package should provide is, indeed, an IFileSystem implementation.

Now, how do we know which IFileSystem implementation should be injected into MediaFileSystem?

In v7 this is done in a config file, with a fair amount of pseudo DI. To clean things up, we've moved to configuration by code. Also, there are many different IFileSystem services (for medias, stylesheets, templates...) and we don't want to use named services. This is what we're trying to solve and the current solution may be mental-ish ;-)

The only other proper way I see... thinking of it... is to create an interface IMediaUnderlyingFileSystem and then, MediaFileSystem would require one of these. But then, your Azure fs need to declare it implements the interface.

Do you see the problem? Am I confused by too much thinking, and do you see a much simpler solution?

lars-erik · 2019-01-09T08:06:57Z

I don't think I ever understood why "we don't want named services"?

zpqrtbnk · 2019-01-09T08:07:40Z

@lars-erik how do you indicate you want to inject the IFileSystem implementation named "foo" in a non container-specific way?

lars-erik · 2019-01-09T08:52:22Z

@zpqrtbnk With the current implementation, the only way without more component metadata is to say so in a factory method. IE. container.Register<IAbstraction>(factory => new Concrete(factory.GetService<IDependency>("name"))).
To make it work with MS.DI, we'd have to employ some more tricks, so there's research needed. I think we could add name overloads to IRegister and do it with Lightinject while researching MS.DI.
Need more discussion, and can be done in umbraco/Umbraco-CMS#3955.

lars-erik · 2019-01-09T09:09:24Z

Discussion had off-site. I'll research naming with MS.DI while @zpqrtbnk will make it work with IRegister/IFactory/LI for now.

JimBobSquarePants · 2019-01-09T10:00:54Z

Also, there are many different IFileSystem services (for medias, stylesheets, templates...) and we don't want to use named services.

@zpqrtbnk I get why you don't want to do it via named services since that would require an explicit implementation.

There's a couple of ways that you could do this. Here's an example one way using factories with keys.
https://stackoverflow.com/a/44177920/427899

That's OK but I think we can maybe do better.

First define your types and interfaces

// Used to identify where the filesystem should be used.
public enum FileSystemScope
{
    Media,
    Templates,
    StyleSheets,
    // You can keep extending this ad infinitum.
}

IFileSystem
{
   FileSystemScope Scope { get; }
}

// By scoping this file system to a particular domain we can use different containers
// for different domains if need be. 
// Any underlying CRUD code would be simple enough and could be shared across 
// the implementations.
public class AzureMediaFileSystem : IFileSystem
{
    public FileSystemScope Scope { get; } = FileSystemScope.Media;
}

Then your consuming class...

// MediaFileSystem is a bad name btw. It's mixing the caller and callee names which is confusing.
public class MediaManager : IMediaManager
{
    private readonly IFileSystem fielsystem;

    public MediaManager(IEnumerable<FileSystem> filesystems)
    {
        // You could register some sort of factory that does this lookup for you if you liked
        // and pass that through the constructor instead.
        this.filesystem = filesystems.First(x => x.Scope == FileSystemScope.Media);
    }
}

Registration in Jamestopia would be performed by the availability of a method as follows.

// IUmbracoBuilder is an extensible interface + implementation that you can pass to your startup.
public static IUmbracoBuilder ReplaceMediaFileSystem<TFileSystem, TFileSystem2>(this IUmbracoBuilder builder)
    where TFileSystem : class, IFileSystem
    where TFileSystem2 : class, IFileSystem
{
    // I dunno if it's possible to ensure at compile time whether it's the IFileSystem is using the   
    // correct FileSystemScope but you'd soon get a warning anyway.
    // Services is IServiceCollection
    builder.Services.Remove(ServiceDescriptor.Singleton<IFileSystem, TFileSystem>());
    builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IFileSystem, TFileSystem2>());
    return builder;
}

It's a pretty simple approach really that you can build easily enough since you know the areas of scope. The same principle could be applied to other areas of the codebase I'm sure also.

kjac · 2019-01-11T08:44:10Z

You guys are heroes ❤️ let me know if you need help testing or whatever.

warrenbuckley · 2019-01-13T17:20:08Z

Any updates on this @zpqrtbnk @lars-erik or @JimBobSquarePants ?!

Can I carry working on this or you still discussing some changes for V8 core related to this?!

lars-erik · 2019-01-13T17:41:19Z

@warrenbuckley Things are happening. See #v8-di on community slack.

warrenbuckley · 2019-01-13T22:26:13Z

Will go & have a nose and catch-up what’s been going on

…anges

…aller project will need reworking anyway

… changes

warrenbuckley · 2019-01-15T11:02:05Z

OK after the recent V8 core FileSystem changes that @lars-erik & @JimBobSquarePants have been in discussion with @zpqrtbnk that has been merged into the V8 core.

umbraco/Umbraco-CMS#4028
umbraco/Umbraco-CMS#4059

I have updated this PR again to make it work, currently it works with AppSetting keys.
You may wanna change the implementation I have done and do some tidying up and discuss if you want to keep/still use the installer project etc... /cc @Jeavon

src/UmbracoFileSystemProviders.Azure/AzureFileSystemComponent.cs

warrenbuckley · 2019-01-15T14:05:56Z

@lars-erik I have done the change you suggested & this currently all works.

However it feels I am getting deeper and deeper into the existing codebase, so I would love it if one of the three of you who are collaborators on this project, are able to do any further refactoring or changes that you would like to see, direct on this PR please.

Cheers,
Warren :)

JimBobSquarePants · 2019-01-15T23:28:36Z

However it feels I am getting deeper and deeper into the existing codebase, so I would love it if one of the three of you who are collaborators on this project, are able to do any further refactoring or changes that you would like to see, direct on this PR please.

Tbh mate I'd like to refactor the entire library from scratch no that we will have proper DI to utilize. If it's working now I'm happy to merge current progress in.

~~If I make a developV8 branch here could you retarget your PR at that?~~

Totally forgot I could change the base!

JimBobSquarePants · 2019-01-16T00:02:47Z

@warrenbuckley While I have your attention I do have some further questions regarding IFileSystem implementations that I'd really to get cleared up.

We currently have to use a virtual path provider in order to serve relative files. It's really shitty for us to have to do so because it shouldn't be a concern of ours - We should just return the values, Umbraco should serve them. Does Umbraco still save the paths in the DB? if so, can it not? Relative files should simply be the way it works. I want to delete our VPP.

Have any updates been made to ensure that the correct response headers are served? We currently have to do a hack, again in the virtual path provider, adding headers like Accept-Ranges to the output to correctly serve videos on Safari. I would really like to drop any header augmentation, leaving that to Umbraco. We have existing issues #78 and #108 that should not exist.

JimBobSquarePants · 2019-01-16T09:50:45Z

src/UmbracoFileSystemProviders.Azure/AzureBlobFileSystemConfig.cs

+        /// <summary>
+        /// The maximum number of days to cache blob items for in the browser
+        /// </summary>
+        public string MaxDays { get; set; }


This is because the Pseudo-DI that V7 uses when parsing the configuration originally passes the parameters as strings. Change them to the real deal anytime.

warrenbuckley · 2019-01-17T08:21:07Z

@JimBobSquarePants to answer your couple of questions I don't have any definitive answers for you now. As far as I know, this will have to stay the same for now,

How shall we move this forward, are you happy to merge it in and then build/work upon the initial changes I have done so you can refactor & tweak as you please.

JimBobSquarePants · 2019-01-17T12:20:45Z

@warrenbuckley Yeah, now I've changed the target we can merge this as. Thanks for making a start! 👍

Warren Buckley added 10 commits January 1, 2019 19:50

Adds in Nuget.config so we can get the V8 UmbracoCore nightly feed in…

5f5abfb

… order to update references in projects

Make StyleCop happy

6a7627a

Update all projects to target .NET 4.7.2 to ensure it will work with …

b881a81

…V8.0.0

Update UmbracoCms.Core to 8.0.0-alpha.52.1449 - WILL BREAK BUILD

45fdfae

Adds in Umbraco.Web V8 Nuget reference needed for WebAPI stuff

052b951

Updates the ApplicationEventHandler to be a user definied component

daba3ae

The test project needs a reference to UmbracoCms.Web nupkg

be7df06

Uses Current.Logger & updated namespaces

d2eefa5

Uses Current.Logger & updated namespaces

33f3f9d

Updates the underlying filesystem to use the new methods added - Stil…

c71571a

…l TODO/implement

Remove the logwrapper for now, it can be added back in if really needed

870aa07

Do final fixes to get this to compile - still need to implement couple of things

Jeavon added 4 commits January 2, 2019 11:58

Added .editorconfig

d78231b

The change made in Umbraco 7.5.15 to relative paths is now permanent …

ed98ba1

…so updating tests to reflect this. Note: slight concern about non media usages of this provider such as UmbracoForms

Updating version to v2 and min Umbraco dependency to v8

a07625a

chore: trying to get the umbraco dependency pre-release to work

4809d3e

Update gitignore for .vs folder

425b26c

Implement GetSize in provider

7b00e6a

Move the methods around before private to please StyleCop

6658900

Seems perfectly valid to return a not implemented for the new AddFile method - due to the bool CanAddPhysical is false/not implemeted as well

JimBobSquarePants reviewed Jan 6, 2019

View reviewed changes

JimBobSquarePants reviewed Jan 9, 2019

View reviewed changes

Warren Buckley added 4 commits January 15, 2019 10:52

Update Umbraco nupkg - that includes the recent Core V8 FileSystem ch…

323d3b0

…anges

Comment out in-accessible code that no longer public in V8 - the inst…

7173490

…aller project will need reworking anyway

Implements new changes to work for V8 File Systems

5e9e2b3

Update build script to depend on Umbraco 8 build that contains the FS…

493cafb

… changes

lars-erik reviewed Jan 15, 2019

View reviewed changes

src/UmbracoFileSystemProviders.Azure/AzureFileSystemComponent.cs Outdated Show resolved Hide resolved

Refactor based from Lars comment

a121412

Fix csproj

d4ded1c

JimBobSquarePants changed the base branch from develop to develop-umbraco-version-8 January 15, 2019 23:31

Forgot to commit config file

a2229b5

JimBobSquarePants reviewed Jan 16, 2019

View reviewed changes

JimBobSquarePants merged commit 65448c6 into umbraco-community:develop-umbraco-version-8 Jan 17, 2019

warrenbuckley deleted the develop8 branch January 18, 2019 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Initial help/work to get this working for V8.0.0 #135

WIP - Initial help/work to get this working for V8.0.0 #135

warrenbuckley commented Jan 1, 2019

Jeavon commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

JimBobSquarePants commented Jan 3, 2019

warrenbuckley commented Jan 3, 2019

lars-erik commented Jan 3, 2019

warrenbuckley commented Jan 3, 2019

JimBobSquarePants Jan 6, 2019 •

edited

Loading

JimBobSquarePants Jan 6, 2019

JimBobSquarePants Jan 6, 2019

JimBobSquarePants commented Jan 6, 2019

JimBobSquarePants commented Jan 8, 2019

JimBobSquarePants Jan 9, 2019

zpqrtbnk commented Jan 9, 2019

lars-erik commented Jan 9, 2019

zpqrtbnk commented Jan 9, 2019

lars-erik commented Jan 9, 2019

lars-erik commented Jan 9, 2019

JimBobSquarePants commented Jan 9, 2019 •

edited

Loading

kjac commented Jan 11, 2019

warrenbuckley commented Jan 13, 2019

lars-erik commented Jan 13, 2019

warrenbuckley commented Jan 13, 2019

warrenbuckley commented Jan 15, 2019

warrenbuckley commented Jan 15, 2019

JimBobSquarePants commented Jan 15, 2019 •

edited

Loading

JimBobSquarePants commented Jan 16, 2019

JimBobSquarePants Jan 16, 2019

warrenbuckley commented Jan 17, 2019

JimBobSquarePants commented Jan 17, 2019

WIP - Initial help/work to get this working for V8.0.0 #135

WIP - Initial help/work to get this working for V8.0.0 #135

Conversation

warrenbuckley commented Jan 1, 2019

Jeavon commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

warrenbuckley commented Jan 2, 2019

JimBobSquarePants commented Jan 3, 2019

warrenbuckley commented Jan 3, 2019

lars-erik commented Jan 3, 2019

warrenbuckley commented Jan 3, 2019

JimBobSquarePants Jan 6, 2019 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Jan 6, 2019

Choose a reason for hiding this comment

JimBobSquarePants Jan 6, 2019

Choose a reason for hiding this comment

JimBobSquarePants commented Jan 6, 2019

JimBobSquarePants commented Jan 8, 2019

JimBobSquarePants Jan 9, 2019

Choose a reason for hiding this comment

zpqrtbnk commented Jan 9, 2019

lars-erik commented Jan 9, 2019

zpqrtbnk commented Jan 9, 2019

lars-erik commented Jan 9, 2019

lars-erik commented Jan 9, 2019

JimBobSquarePants commented Jan 9, 2019 • edited Loading

kjac commented Jan 11, 2019

warrenbuckley commented Jan 13, 2019

lars-erik commented Jan 13, 2019

warrenbuckley commented Jan 13, 2019

warrenbuckley commented Jan 15, 2019

warrenbuckley commented Jan 15, 2019

JimBobSquarePants commented Jan 15, 2019 • edited Loading

JimBobSquarePants commented Jan 16, 2019

JimBobSquarePants Jan 16, 2019

Choose a reason for hiding this comment

warrenbuckley commented Jan 17, 2019

JimBobSquarePants commented Jan 17, 2019

JimBobSquarePants Jan 6, 2019 •

edited

Loading

JimBobSquarePants commented Jan 9, 2019 •

edited

Loading

JimBobSquarePants commented Jan 15, 2019 •

edited

Loading