Implement a CombinedFileProvider #142
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Microsoft.Extensions.Primitives; | ||
|
|
||
| namespace Microsoft.AspNet.FileProviders | ||
| { | ||
| /// <summary> | ||
| /// Looks up files using embedded resources in the specified assembly. | ||
| /// This file provider is case sensitive. | ||
| /// </summary> | ||
| public class CombinedFileProvider : IFileProvider | ||
|
There was a problem hiding this comment. Should this be named |
||
| { | ||
| private readonly IEnumerable<IFileProvider> _fileProviders; | ||
|
|
||
| /// <summary> | ||
| /// 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) | ||
|
There was a problem hiding this comment. I'd avoid params. Just take an There was a problem hiding this comment. A options.FileProvider = new CombinedFileSystemProvider(
new EmbeddedFileProvider(...),
new EmbeddedFileProvider(...));Since the file provider is a hot path, using There was a problem hiding this comment.
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. There was a problem hiding this comment. Hi @davidfowl, is it a general purpose to avoid params, or just in this situation ? |
||
| { | ||
| _fileProviders = fileProviders; | ||
|
There was a problem hiding this comment.
|
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Locates a file at the given path. | ||
| /// </summary> | ||
| /// <param name="subpath">The path that identifies the file. </param> | ||
| /// <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) | ||
|
There was a problem hiding this comment. nit: you could change There was a problem hiding this comment. I don't understand your point: There was a problem hiding this comment. When a variable is declared as an array, the compiler automatically replaces the There was a problem hiding this comment. Thanks, I'm happy to learn something today! |
||
| { | ||
| var fileInfo = fileProvider.GetFileInfo(subpath); | ||
| if (fileInfo != null && fileInfo.Exists) | ||
| { | ||
| return fileInfo; | ||
| } | ||
| } | ||
| return new NotFoundFileInfo(subpath); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Enumerate a directory at the given path, if any. | ||
| /// </summary> | ||
| /// <param name="subpath">The path that identifies the directory</param> | ||
| /// <returns>Contents of the directory. Caller must check Exists property. The content is a merge of the contents of the provided <see cref="IFileProvider"/>. When there is multiple <see cref="IFileInfo"/> with the same Name property, only the first one is included on the results.</returns> | ||
|
There was a problem hiding this comment. Yeah, it's the problem when VS is configured to do "Word wrap". I will add some new lines 😄 |
||
| public IDirectoryContents GetDirectoryContents(string subpath) | ||
| { | ||
| // gets the content of all directories and merged them | ||
| var existingDirectoryContentsForAllProviders = _fileProviders.Select( | ||
|
There was a problem hiding this comment. nit: for maximal performance, you could avoid using LINQ here. There was a problem hiding this comment. OK, I will change it for a |
||
| fileProvider => fileProvider.GetDirectoryContents(subpath)) | ||
| .Where(directoryContents => directoryContents != null && directoryContents.Exists) | ||
| .ToList(); | ||
|
|
||
| // There is no existing directory contents | ||
| if (existingDirectoryContentsForAllProviders.Count == 0) | ||
| { | ||
| return new NotFoundDirectoryContents(); | ||
| } | ||
| var combinedDirectoryContents = new CombinedDirectoryContents(existingDirectoryContentsForAllProviders); | ||
| return combinedDirectoryContents; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a <see cref="IChangeToken"/> for the specified <paramref name="filter"/>. | ||
| /// </summary> | ||
| /// <remarks></remarks> | ||
| /// <param name="pattern">Filter string used to determine what files or folders to monitor. Example: **/*.cs, *.*, subFolder/**/*.cshtml.</param> | ||
| /// <returns>An <see cref="IChangeToken"/> that is notified when a file matching <paramref name="filter"/> is added, modified or deleted. The change token will be notified when one of the change token returned by the provided <see cref="IFileProvider"/> will be notified.</returns> | ||
| public IChangeToken Watch(string pattern) | ||
|
There was a problem hiding this comment. No documentation for this method? I think especially for this one it is interesting. |
||
| { | ||
| // Watch all file providers | ||
| var activeChangeTokens = _fileProviders | ||
| .Select(fileProvider => fileProvider.Watch(pattern)) | ||
| .Where(changeToken => changeToken != null && changeToken.ActiveChangeCallbacks).ToList(); | ||
|
|
||
| // There is no change token with active change callbacks | ||
| if (activeChangeTokens.Count == 0) | ||
| { | ||
| return NoopChangeToken.Singleton; | ||
| } | ||
| var combinedFileChangeToken = new CombinedFileChangeToken(activeChangeTokens); | ||
| return combinedFileChangeToken; | ||
| } | ||
|
|
||
| private class CombinedDirectoryContents : IDirectoryContents | ||
|
There was a problem hiding this comment. I'm perhaps alone with this opinion and I've seen this in the other FileSystem projects already, but can't we stop with these nested classes and just make it an internal class within an own file? There was a problem hiding this comment. OK for me. There was a problem hiding this comment. You could start with this - just make these types public. |
||
| { | ||
| private readonly Dictionary<string, IFileInfo> _files = new Dictionary<string, IFileInfo>(); | ||
|
There was a problem hiding this comment. Curious: why using a dictionary here? AFAICT, the key doesn't seem to be used anywhere. There was a problem hiding this comment. The dictionary is used to filter duplicate |
||
|
|
||
| public CombinedDirectoryContents(IEnumerable<IEnumerable<IFileInfo>> listOfFiles) | ||
| { | ||
| foreach (var files in listOfFiles) | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 |
||
| { | ||
| Exists = true; | ||
| foreach (var file in files) | ||
| { | ||
| if (!_files.ContainsKey(file.Name)) | ||
| { | ||
| _files.Add(file.Name, file); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public IEnumerator<IFileInfo> GetEnumerator() | ||
| { | ||
| return _files.Values.GetEnumerator(); | ||
| } | ||
|
|
||
| IEnumerator IEnumerable.GetEnumerator() | ||
| { | ||
| return _files.Values.GetEnumerator(); | ||
| } | ||
|
|
||
| public bool Exists { get; } | ||
| } | ||
|
|
||
| private class CombinedFileChangeToken : IChangeToken | ||
| { | ||
| private readonly IEnumerable<IChangeToken> _changeTokens; | ||
|
|
||
| public CombinedFileChangeToken(IEnumerable<IChangeToken> changeTokens) | ||
|
There was a problem hiding this comment. Same remark as above: since this parameter is always used with a list, you could replace IEnumerable with a list or an array. |
||
| { | ||
| _changeTokens = changeTokens ?? new List<IChangeToken>(); | ||
|
There was a problem hiding this comment. nit: looks like |
||
| } | ||
|
|
||
| public IDisposable RegisterChangeCallback(Action<object> callback, object state) | ||
| { | ||
| return new Disposables(_changeTokens.Where(changeToken => changeToken.ActiveChangeCallbacks).Select(changeToken => changeToken.RegisterChangeCallback(callback, state)).ToList()); | ||
| } | ||
|
|
||
| public bool HasChanged | ||
| { | ||
| get { return _changeTokens.Any(_ => _.HasChanged); } | ||
|
There was a problem hiding this comment. Don't use underscores for things you're using: |
||
| } | ||
|
|
||
| public bool ActiveChangeCallbacks | ||
| { | ||
| get { return _changeTokens.Any(_ => _.ActiveChangeCallbacks); } | ||
| } | ||
|
|
||
| private class Disposables : IDisposable | ||
|
There was a problem hiding this comment.
|
||
| { | ||
| private readonly IEnumerable<IDisposable> _disposables; | ||
|
|
||
|
|
||
| public Disposables(IEnumerable<IDisposable> disposables) | ||
| { | ||
| _disposables = disposables; | ||
| } | ||
| public void Dispose() | ||
| { | ||
| if (_disposables != null) | ||
|
There was a problem hiding this comment. Isn't this guaranteed to be non null? |
||
| { | ||
| foreach (var disposable in _disposables) | ||
|
|
||
| { | ||
| disposable.Dispose(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project ToolsVersion="__ToolsVersion__" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">14.0</VisualStudioVersion> | ||
| <VSToolsPath Condition="'$(VSToolsPath)' == ''">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)</VSToolsPath> | ||
| </PropertyGroup> | ||
| <Import Project="$(VSToolsPath)\AspNet\Microsoft.Web.AspNet.Props" Condition="'$(VSToolsPath)' != ''" /> | ||
| <PropertyGroup Label="Globals"> | ||
| <ProjectGuid>caaf52ef-f91b-474d-ac56-fe9d96ff8254</ProjectGuid> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x86'" Label="Configuration"> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x86'" Label="Configuration"> | ||
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <SchemaVersion>2.0</SchemaVersion> | ||
| </PropertyGroup> | ||
| <Import Project="$(VSToolsPath)\AspNet\Microsoft.Web.AspNet.targets" Condition="'$(VSToolsPath)' != ''" /> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Reflection; | ||
| using System.Resources; | ||
|
|
||
| [assembly: AssemblyMetadata("Serviceable", "True")] | ||
| [assembly: NeutralResourcesLanguage("en-us")] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| { | ||
| "version": "1.0.0-*", | ||
| "compilationOptions": { | ||
| "warningsAsErrors": true, | ||
| "keyFile": "../../tools/Key.snk" | ||
| }, | ||
| "description": "Implementation of ASP.NET 5 file provider abstractions to combine file providers.", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git://github.com/aspnet/filesystem" | ||
| }, | ||
| "dependencies": { | ||
| "Microsoft.AspNet.FileProviders.Sources": { | ||
| "version": "1.0.0-*", | ||
| "type": "build" | ||
| }, | ||
| "Microsoft.AspNet.FileProviders.Abstractions": "1.0.0-*" | ||
| }, | ||
| "frameworks": { | ||
| "net451": {}, | ||
| "dotnet5.4": { | ||
| "dependencies": { | ||
| "System.Collections": "4.0.11-*", | ||
| "System.Reflection": "4.0.10-*", | ||
|
There was a problem hiding this comment. You might not need these. |
||
| "System.Runtime.Extensions": "4.0.11-*" | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary is incorrrect.