Implement a CombinedFileProvider #142

mgrosperrin · 2015-11-21T18:05:24Z

This PR is for the issue #49

The behavior for each method is:

GetFileInfo: returns the first IFileInfo of all provided IFileProvider where the Exists property is true,
GetDirectoryContents: merges the the contents of all provided IFileProvider. On duplicate entries (based on Name property), only the first entry is kept,
Watch: returns a IChangeToken that merges the IChangeToken of all provided IFileProvider. When one of the token raises the change notification, the returned token raises it.

The CombinedFileProvider merges the content of the directories

…yContents)

dnfclas · 2015-11-21T18:05:28Z

Hi @mgrosperrin, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

MartinJohns · 2015-11-21T19:52:43Z

global.json

-    "projects": ["src"]
+    "projects": [ "src" ],
+    "sdk": {
+        "version": "1.0.0-beta8"


I don't think this modification should be part of the PR. Especially not since the latest stable version is RC1, not beta8.

Remove the sdk version in global.json

mgrosperrin · 2015-11-22T14:11:57Z

Hi @MartinJohns,
thanks for your remarks. I've updated the global.json to remove my changes.
I have added documentation to the Watch method, and add more info on the behavior of the others methods as well. I have put it in the returns part of the documentation. Let me know if this is good, or if it will be better in a remarks element.

kevinchalet · 2015-11-23T15:47:44Z

src/Microsoft.AspNet.FileProviders.Combined/CombinedFileProvider.cs

+        /// <returns>The file information. Caller must check Exists property. This will be the first existing <see cref="IFileInfo"/> returned by the provided <see cref="IFileProvider"/> or a not found <see cref="IFileInfo"/> if no existing files is found.</returns>
+        public IFileInfo GetFileInfo(string subpath)
+        {
+            foreach (var fileProvider in _fileProviders)


nit: you could change _fileProviders's type to IFileProvider[] to avoid allocating an enumerator here (you don't have to explicitly use a for loop, since the compiler is supposed to optimize that for you).

I don't understand your point: IFileProvider[] implements IEnumerable<IFileProvider> so when an enumerator will be allocated (which will not be in your case) ?

When a variable is declared as an array, the compiler automatically replaces the foreach loop by a for loop and directly accesses the items via array[index] without having to instantiate a new enumerator. The compiler can't apply the same optimization when the type is IEnumerable<>.

Thanks, I'm happy to learn something today!

kevinchalet · 2015-11-23T15:56:40Z

Definitely a must have! 😄

kevinchalet · 2015-11-23T16:06:54Z

src/Microsoft.AspNet.FileProviders.Combined/CombinedFileProvider.cs

+
+            public CombinedDirectoryContents(IEnumerable<IEnumerable<IFileInfo>> listOfFiles)
+            {
+                foreach (var files in listOfFiles)


This code path will immediately enumerate all the files exposed by the different providers, which may take some time. You should maybe consider deferring this operation to the last moment (in GetEnumerator) and using an iterator.

I have made the assumption that the directory contents will be pre-calculated (based on the two other providers), but you're right. I can move it to a Lazy<IEnumerable<IFileInfo>> (to avoiding dealing with the thread-safe and evaluating only once issues).

davidfowl · 2015-11-24T08:46:51Z

src/Microsoft.AspNet.FileProviders.Combined/CombinedFileProvider.cs

+        /// Initializes a new instance of the <see cref="CombinedFileProvider" /> class using a list of file provider.
+        /// </summary>
+        /// <param name="fileProviders"></param>
+        public CombinedFileProvider(params IFileProvider[] fileProviders)


I'd avoid params. Just take an IEnumerable<IFileProvider>

A params overload definitely makes sense, because that's how people will use this combined thingy:

options.FileProvider = new CombinedFileSystemProvider( new EmbeddedFileProvider(...), new EmbeddedFileProvider(...));

Since the file provider is a hot path, using IEnumerable<IFileProvider> is not really appropriate. Not to mention that, semantically, it's not immediately clear that the order matters when using IEnumerable. IMHO, an array or a list is much more appropriate in this case. That said, nothing prevents you from adding an IEnumerable<> overload that calls ToArray() or ToList() to avoid allocating enumerators in GetFileInfo/GetDirectoryContents.

A params overload definitely makes sense, because that's how people will use this combined thingy:

Not really. Just make an array if you need that:

new CombinedFileSystemProvider(new[] { new EmbeddedFileProvider(...), new EmbeddedFileProvider(...) });

I'd keep the params array overload for a static factory method instead of baking it into the ctor.

Converting it to an array or list is more appropriate than enforcing an array be passed in.

Hi @davidfowl, is it a general purpose to avoid params, or just in this situation ?

VaclavElias · 2015-11-24T09:19:12Z

Just a question. Is this provider going to allow to have Views in Class Library? If yes, how can we use it? If not, please ignore my question :). Thanks.

MartinJohns · 2015-11-24T09:25:25Z

@VaclavElias You can do that already.

services.Configure<RazorViewEngineOptions>(options =>
{
    options.FileProvider = yourFileProvider;
});

By the default the used file provider is a PhysicalFileProvider pointing to your views directory. You can replace this with a custom IFileProvider, e.g. the EmbeddedFileProvider to support views embedded in class libraries.

The here proposed CombinedFileProvider approach let's you use a PhysicalFileProvider as a first option, and the EmbeddedFileProvider as a second option. This would use views present in the local file system if they exist, with a fall back to the views embedded in the assembly.

VaclavElias · 2015-11-24T09:35:37Z

Thanks @MartinJohns making it clear for me!

kevinchalet · 2015-11-24T11:12:14Z

src/Microsoft.AspNet.FileProviders.Combined/CombinedFileProvider.cs

+
+            public CombinedFileChangeToken(List<IChangeToken> changeTokens)
+            {
+                _changeTokens = changeTokens ?? new List<IChangeToken>();


nit: looks like changeTokens cannot be null when used by Watch. I'd personally removed the null coalescing operator 😄

…f IFileProvider and files

pranavkm · 2015-11-26T20:08:50Z

src/Microsoft.AspNet.FileProviders.Combined/CombinedDisposable.cs

+        }
+        public void Dispose()
+        {
+            for (int i = 0; i < _disposables.Count; i++)


Couple of other places with the same issue.

pranavkm · 2015-11-26T20:17:23Z

⌚

pranavkm · 2015-11-26T20:19:02Z

cc @Eilon for CompositeFileProvider versus CombinedFileProvider. Mvc has the habit of using Composite so it sounded like it would be a familiar pattern for users.

…tead of Lazy) Add tests for the combined IDsposable Fix formatting and typos

Eilon · 2015-11-28T21:21:43Z

Yeah Composite sounds better to me for this.

mgrosperrin · 2015-12-07T19:57:55Z

Hi,
is there anything else I can do to respond to your remarks?

pranavkm · 2015-12-07T20:06:32Z

Sorry, GitHub doesn't send an email when you push an update. A comment saying you had it updated would be great (for future use). I'll have a look in a bit.

mgrosperrin · 2015-12-07T20:26:51Z

Oh OK, didn't know that 😄
I'll know for the next time.

muratg · 2015-12-08T17:25:45Z

@pranavkm Could you merge once it's good?

@mgrosperrin Thanks for the contribution!

pranavkm · 2015-12-08T17:38:20Z

src/Microsoft.AspNet.FileProviders.Composite/CompositeDirectoryContents.cs

+
+namespace Microsoft.AspNet.FileProviders
+{
+    internal class CompositeDirectoryContents : IDirectoryContents


Could you make these types public? No reason to have them be internal.

pranavkm · 2015-12-08T17:42:44Z

Looks good. If you could get the last few changes.

MartinJohns · 2015-12-08T18:14:17Z

This might be a crazy idea given the state of so much code in the repositories.. But can't we start to comment the code? Add some documentation?

Add a constructor to CompositeFileProvider with IEnumerable

mgrosperrin · 2015-12-08T19:37:52Z

Hi,
I have made the changes.
For the constructor of CompositeFileProvider, I have added an overload with an IEnumerable and kept the current one.

pranavkm · 2015-12-08T22:20:26Z

I'll get this merged in soon.

pranavkm · 2015-12-09T18:06:07Z

Merged in 15c9f1d

pranavkm · 2015-12-09T18:06:15Z

Thanks for the PR!

mgrosperrin · 2015-12-09T21:21:11Z

You're welcome!
Thanks for your reviews.

mgrosperrin added 3 commits November 19, 2015 16:27

Add CombinedFileProvider

ad7154e

The CombinedFileProvider merges the content of the directories

Add tests for the combined file provider (GetFileInfo and GetDirector…

d1b8237

…yContents)

Add tests for the Watch method

e58209c

dnfclas added the cla-already-signed label Nov 21, 2015

MartinJohns reviewed Nov 21, 2015
View reviewed changes

Add documentation to explicit how the combinaison is done.

7de02de

Remove the sdk version in global.json

kevinchalet reviewed Nov 23, 2015
View reviewed changes

lszomoru mentioned this pull request Nov 23, 2015

Packaging Content Files with dnu pack dotnet/aspnetcore#651

Closed

Replace IEnumerable by more concrete type

0bfeaaa

davidfowl reviewed Nov 24, 2015
View reviewed changes

lszomoru mentioned this pull request Nov 24, 2015

Guidance for loading external views aspnet/Mvc#3445

Closed

kevinchalet reviewed Nov 24, 2015
View reviewed changes

Rewrite the CombinedDirectoryContents to be lazy in the enumeration o…

1043d92

…f IFileProvider and files

pranavkm reviewed Nov 26, 2015
View reviewed changes

Deferring evaluation of files when combining mutiple directories (ins…

6cd24d2

…tead of Lazy) Add tests for the combined IDsposable Fix formatting and typos

Rename CombiendFileProvider to CompositeFileProvider

093fc42

muratg added this to the 1.0.0-rc2 milestone Dec 8, 2015

muratg assigned pranavkm Dec 8, 2015

pranavkm reviewed Dec 8, 2015
View reviewed changes

Make CompositeXXX types public

3576eac

Add a constructor to CompositeFileProvider with IEnumerable

pranavkm closed this Dec 9, 2015

mgrosperrin mentioned this pull request Dec 14, 2015

Un-nest private implentations of IFileInfo and IDirectoryContents #156

Closed

Implement a CombinedFileProvider #142

Implement a CombinedFileProvider #142

Conversation

mgrosperrin commented Nov 21, 2015

dnfclas commented Nov 21, 2015

Choose a reason for hiding this comment

mgrosperrin commented Nov 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaclavElias commented Nov 24, 2015

MartinJohns commented Nov 24, 2015

VaclavElias commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Nov 26, 2015

pranavkm commented Nov 26, 2015

Eilon commented Nov 28, 2015

mgrosperrin commented Dec 7, 2015

pranavkm commented Dec 7, 2015

mgrosperrin commented Dec 7, 2015

muratg commented Dec 8, 2015

Choose a reason for hiding this comment

pranavkm commented Dec 8, 2015

MartinJohns commented Dec 8, 2015

mgrosperrin commented Dec 8, 2015

pranavkm commented Dec 8, 2015

pranavkm commented Dec 9, 2015

pranavkm commented Dec 9, 2015

mgrosperrin commented Dec 9, 2015