From ac945a0bcf43334f163a66223dd4d7bb10cff6aa Mon Sep 17 00:00:00 2001 From: Chris R Date: Tue, 28 Jul 2015 12:24:42 -0700 Subject: [PATCH] #339 Reduce IFeatureCollection surface area. --- .../FeatureCollection.cs | 167 ++++++------------ .../FeatureReference.cs | 14 +- .../IFeatureCollection.cs | 18 +- .../project.json | 1 + .../DefaultHttpContext.cs | 3 +- .../OwinFeatureCollection.cs | 167 +++++------------- .../FeatureCollectionTests.cs | 48 +++++ .../InterfaceDictionaryTests.cs | 83 --------- .../QueryFeatureTests.cs | 2 +- .../OwinFeatureCollectionTests.cs | 20 +-- 10 files changed, 174 insertions(+), 349 deletions(-) create mode 100644 test/Microsoft.AspNet.Http.Features.Tests/FeatureCollectionTests.cs delete mode 100644 test/Microsoft.AspNet.Http.Features.Tests/InterfaceDictionaryTests.cs diff --git a/src/Microsoft.AspNet.Http.Features/FeatureCollection.cs b/src/Microsoft.AspNet.Http.Features/FeatureCollection.cs index 1fbbaf4288..16b4d6fb44 100644 --- a/src/Microsoft.AspNet.Http.Features/FeatureCollection.cs +++ b/src/Microsoft.AspNet.Http.Features/FeatureCollection.cs @@ -4,15 +4,16 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Linq; using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Http.Features { public class FeatureCollection : IFeatureCollection { + private static KeyComparer FeatureKeyComparer = new FeatureCollection.KeyComparer(); private readonly IFeatureCollection _defaults; - private readonly IDictionary _featureByFeatureType = new Dictionary(); - private readonly object _containerSync = new object(); + private IDictionary _features; private volatile int _containerRevision; public FeatureCollection() @@ -24,53 +25,38 @@ namespace Microsoft.AspNet.Http.Features _defaults = defaults; } - public object GetInterface() - { - return GetInterface(null); - } - - public object GetInterface([NotNull] Type type) - { - object feature; - if (_featureByFeatureType.TryGetValue(type, out feature)) - { - return feature; - } - - if (_defaults != null && _defaults.TryGetValue(type, out feature)) - { - return feature; - } - return null; - } - - void SetInterface([NotNull] Type type, object feature) - { - if (feature == null) - { - Remove(type); - return; - } - - lock (_containerSync) - { - _featureByFeatureType[type] = feature; - _containerRevision++; - } - } - public virtual int Revision { - get { return _containerRevision; } + get { return _containerRevision + (_defaults?.Revision ?? 0); } } - public void Dispose() - { - } + public bool IsReadOnly { get { return false; } } - public IEnumerator> GetEnumerator() + public object this[[NotNull] Type key] { - throw new NotImplementedException(); + get + { + object result; + return _features != null && _features.TryGetValue(key, out result) ? result : _defaults?[key]; + } + set + { + if (value == null) + { + if (_features != null && _features.Remove(key)) + { + _containerRevision++; + } + return; + } + + if (_features == null) + { + _features = new Dictionary(); + } + _features[key] = value; + _containerRevision++; + } } IEnumerator IEnumerable.GetEnumerator() @@ -78,89 +64,42 @@ namespace Microsoft.AspNet.Http.Features return GetEnumerator(); } - public void Add(KeyValuePair item) + public IEnumerator> GetEnumerator() { - SetInterface(item.Key, item.Value); - } - - public void Clear() - { - throw new NotImplementedException(); - } - - public bool Contains(KeyValuePair item) - { - object value; - return TryGetValue(item.Key, out value) && Equals(item.Value, value); - } - - public void CopyTo(KeyValuePair[] array, int arrayIndex) - { - throw new NotImplementedException(); - } - - public bool Remove(KeyValuePair item) - { - return Contains(item) && Remove(item.Key); - } - - public int Count - { - get { throw new NotImplementedException(); } - } - - public bool IsReadOnly - { - get { return false; } - } - - public bool ContainsKey([NotNull] Type key) - { - return GetInterface(key) != null; - } - - public void Add([NotNull] Type key, [NotNull] object value) - { - if (ContainsKey(key)) + if (_features != null) { - throw new ArgumentException(); - } - SetInterface(key, value); - } - - public bool Remove([NotNull] Type key) - { - lock (_containerSync) - { - if (_featureByFeatureType.Remove(key)) + foreach (var pair in _features) { - _containerRevision++; - return true; + yield return pair; + } + } + + if (_defaults != null) + { + // Don't return features masked by the wrapper. + foreach (var pair in _features == null ? _defaults : _defaults.Except(_features, FeatureKeyComparer)) + { + yield return pair; } - return false; } } - public bool TryGetValue([NotNull] Type key, out object value) + public virtual void Dispose() { - value = GetInterface(key); - return value != null; + _defaults?.Dispose(); } - public object this[Type key] + private class KeyComparer : IEqualityComparer> { - get { return GetInterface(key); } - set { SetInterface(key, value); } - } + public bool Equals(KeyValuePair x, KeyValuePair y) + { + return x.Key.Equals(y.Key); + } - public ICollection Keys - { - get { throw new NotImplementedException(); } - } - - public ICollection Values - { - get { throw new NotImplementedException(); } + public int GetHashCode(KeyValuePair obj) + { + throw new NotImplementedException(); + } } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Http.Features/FeatureReference.cs b/src/Microsoft.AspNet.Http.Features/FeatureReference.cs index 7ffb94d555..538297a3a0 100644 --- a/src/Microsoft.AspNet.Http.Features/FeatureReference.cs +++ b/src/Microsoft.AspNet.Http.Features/FeatureReference.cs @@ -18,23 +18,19 @@ namespace Microsoft.AspNet.Http.Features public T Fetch(IFeatureCollection features) { - if (_revision == features.Revision) return _feature; - object value; - if (features.TryGetValue(typeof(T), out value)) + if (_revision == features.Revision) { - _feature = (T)value; - } - else - { - _feature = default(T); + return _feature; } + _feature = (T)features[typeof(T)]; _revision = features.Revision; return _feature; } public T Update(IFeatureCollection features, T feature) { - features[typeof(T)] = _feature = feature; + features[typeof(T)] = feature; + _feature = feature; _revision = features.Revision; return feature; } diff --git a/src/Microsoft.AspNet.Http.Features/IFeatureCollection.cs b/src/Microsoft.AspNet.Http.Features/IFeatureCollection.cs index 70f91770e1..ba612b10c6 100644 --- a/src/Microsoft.AspNet.Http.Features/IFeatureCollection.cs +++ b/src/Microsoft.AspNet.Http.Features/IFeatureCollection.cs @@ -3,11 +3,27 @@ using System; using System.Collections.Generic; +using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Http.Features { - public interface IFeatureCollection : IDictionary, IDisposable + public interface IFeatureCollection : IEnumerable>, IDisposable { + /// + /// Indicates if the collection can be modified. + /// + bool IsReadOnly { get; } + + /// + /// Incremented for each modification and can be used verify cached results. + /// int Revision { get; } + + /// + /// Gets or sets a given feature. Setting a null value removes the feature. + /// + /// + /// The requested feature, or null if it is not present. + object this[[NotNull] Type key] { get; set; } } } diff --git a/src/Microsoft.AspNet.Http.Features/project.json b/src/Microsoft.AspNet.Http.Features/project.json index 26fb66f00a..a44172292c 100644 --- a/src/Microsoft.AspNet.Http.Features/project.json +++ b/src/Microsoft.AspNet.Http.Features/project.json @@ -13,6 +13,7 @@ "dnxcore50": { "dependencies": { "System.Collections": "4.0.10-beta-*", + "System.Linq": "4.0.0-beta-*", "System.Net.Primitives": "4.0.10-beta-*", "System.Net.WebSockets": "4.0.0-beta-*", "System.Runtime.Extensions": "4.0.10-beta-*", diff --git a/src/Microsoft.AspNet.Http/DefaultHttpContext.cs b/src/Microsoft.AspNet.Http/DefaultHttpContext.cs index 0ff8ae2b89..c3caf8ec66 100644 --- a/src/Microsoft.AspNet.Http/DefaultHttpContext.cs +++ b/src/Microsoft.AspNet.Http/DefaultHttpContext.cs @@ -172,8 +172,7 @@ namespace Microsoft.AspNet.Http.Internal public override object GetFeature(Type type) { - object value; - return _features.TryGetValue(type, out value) ? value : null; + return _features[type]; } public override void SetFeature(Type type, object instance) diff --git a/src/Microsoft.AspNet.Owin/OwinFeatureCollection.cs b/src/Microsoft.AspNet.Owin/OwinFeatureCollection.cs index ca1cca500b..6dd44e52e6 100644 --- a/src/Microsoft.AspNet.Owin/OwinFeatureCollection.cs +++ b/src/Microsoft.AspNet.Owin/OwinFeatureCollection.cs @@ -2,10 +2,10 @@ // 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.Globalization; using System.IO; -using System.Linq; using System.Net; using System.Net.WebSockets; using System.Reflection; @@ -16,7 +16,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNet.Http.Features; using Microsoft.AspNet.Http.Features.Authentication; -using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Owin { @@ -111,6 +110,12 @@ namespace Microsoft.AspNet.Owin set { Prop(OwinConstants.RequestHeaders, value); } } + string IHttpRequestIdentifierFeature.TraceIdentifier + { + get { return Prop(OwinConstants.RequestId); } + set { Prop(OwinConstants.RequestId, value); } + } + Stream IHttpRequestFeature.Body { get { return Prop(OwinConstants.RequestBody); } @@ -266,7 +271,7 @@ namespace Microsoft.AspNet.Owin /// /// Gets or sets if the underlying server supports WebSockets. This is enabled by default. - /// The value should be consistant across requests. + /// The value should be consistent across requests. /// public bool SupportsWebSockets { get; set; } @@ -290,17 +295,25 @@ namespace Microsoft.AspNet.Owin return accept(context); } + // IFeatureCollection + public int Revision { get { return 0; } // Not modifiable } - public void Add(Type key, object value) + public bool IsReadOnly { - throw new NotSupportedException(); + get { return true; } } - public bool ContainsKey(Type key) + public object this[Type key] + { + get { return Get(key); } + set { throw new NotSupportedException(); } + } + + private bool SupportsInterface(Type key) { // Does this type implement the requested interface? if (key.GetTypeInfo().IsAssignableFrom(GetType().GetTypeInfo())) @@ -325,136 +338,48 @@ namespace Microsoft.AspNet.Owin return false; } - public ICollection Keys + public object Get(Type key) { - get + if (SupportsInterface(key)) { - var keys = new List() - { - typeof(IHttpRequestFeature), - typeof(IHttpResponseFeature), - typeof(IHttpConnectionFeature), - typeof(IOwinEnvironmentFeature), - typeof(IHttpRequestLifetimeFeature), - typeof(IHttpAuthenticationFeature), - }; - if (SupportsSendFile) - { - keys.Add(typeof(IHttpSendFileFeature)); - } - if (SupportsClientCerts) - { - keys.Add(typeof(ITlsConnectionFeature)); - } - if (SupportsWebSockets) - { - keys.Add(typeof(IHttpWebSocketFeature)); - } - return keys; + return this; } + return null; } - public bool Remove(Type key) + public void Set(Type key, object value) { throw new NotSupportedException(); } - public bool TryGetValue(Type key, out object value) + IEnumerator IEnumerable.GetEnumerator() { - if (ContainsKey(key)) - { - value = this; - return true; - } - value = null; - return false; - } - - public ICollection Values - { - get { throw new NotSupportedException(); } - } - - public object this[Type key] - { - get - { - object value; - if (TryGetValue(key, out value)) - { - return value; - } - throw new KeyNotFoundException(key.FullName); - } - set - { - throw new NotSupportedException(); - } - } - - public void Add(KeyValuePair item) - { - throw new NotSupportedException(); - } - - public void Clear() - { - throw new NotSupportedException(); - } - - public bool Contains(KeyValuePair item) - { - object result; - return TryGetValue(item.Key, out result) && result.Equals(item.Value); - } - - public void CopyTo([NotNull] KeyValuePair[] array, int arrayIndex) - { - if (arrayIndex < 0 || arrayIndex > array.Length) - { - throw new ArgumentOutOfRangeException(nameof(arrayIndex), arrayIndex, string.Empty); - } - var keys = Keys; - if (keys.Count > array.Length - arrayIndex) - { - throw new ArgumentException(); - } - - foreach (var key in keys) - { - array[arrayIndex++] = new KeyValuePair(key, this[key]); - } - } - - public int Count - { - get { return Keys.Count; } - } - - public bool IsReadOnly - { - get { return true; } - } - - string IHttpRequestIdentifierFeature.TraceIdentifier - { - get { return Prop(OwinConstants.RequestId); } - set { Prop(OwinConstants.RequestId, value); } - } - - public bool Remove(KeyValuePair item) - { - throw new NotSupportedException(); + return GetEnumerator(); } public IEnumerator> GetEnumerator() { - return Keys.Select(type => new KeyValuePair(type, this[type])).GetEnumerator(); - } + yield return new KeyValuePair(typeof(IHttpRequestFeature), this); + yield return new KeyValuePair(typeof(IHttpResponseFeature), this); + yield return new KeyValuePair(typeof(IHttpConnectionFeature), this); + yield return new KeyValuePair(typeof(IHttpRequestIdentifierFeature), this); + yield return new KeyValuePair(typeof(IHttpRequestLifetimeFeature), this); + yield return new KeyValuePair(typeof(IHttpAuthenticationFeature), this); + yield return new KeyValuePair(typeof(IOwinEnvironmentFeature), this); - System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() - { - return GetEnumerator(); + // Check for conditional features + if (SupportsSendFile) + { + yield return new KeyValuePair(typeof(IHttpSendFileFeature), this); + } + if (SupportsClientCerts) + { + yield return new KeyValuePair(typeof(ITlsConnectionFeature), this); + } + if (SupportsWebSockets) + { + yield return new KeyValuePair(typeof(IHttpWebSocketFeature), this); + } } public void Dispose() diff --git a/test/Microsoft.AspNet.Http.Features.Tests/FeatureCollectionTests.cs b/test/Microsoft.AspNet.Http.Features.Tests/FeatureCollectionTests.cs new file mode 100644 index 0000000000..2cc0ed6448 --- /dev/null +++ b/test/Microsoft.AspNet.Http.Features.Tests/FeatureCollectionTests.cs @@ -0,0 +1,48 @@ +// 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 Xunit; + +namespace Microsoft.AspNet.Http.Features +{ + public class FeatureCollectionTests + { + [Fact] + public void AddedInterfaceIsReturned() + { + var interfaces = new FeatureCollection(); + var thing = new Thing(); + + interfaces[typeof(IThing)] = thing; + + object thing2 = interfaces[typeof(IThing)]; + Assert.Equal(thing2, thing); + } + + [Fact] + public void IndexerAlsoAddsItems() + { + var interfaces = new FeatureCollection(); + var thing = new Thing(); + + interfaces[typeof(IThing)] = thing; + + Assert.Equal(interfaces[typeof(IThing)], thing); + } + + [Fact] + public void SetNullValueRemoves() + { + var interfaces = new FeatureCollection(); + var thing = new Thing(); + + interfaces[typeof(IThing)] = thing; + Assert.Equal(interfaces[typeof(IThing)], thing); + + interfaces[typeof(IThing)] = null; + + object thing2 = interfaces[typeof(IThing)]; + Assert.Null(thing2); + } + } +} diff --git a/test/Microsoft.AspNet.Http.Features.Tests/InterfaceDictionaryTests.cs b/test/Microsoft.AspNet.Http.Features.Tests/InterfaceDictionaryTests.cs deleted file mode 100644 index 3d4879f0a3..0000000000 --- a/test/Microsoft.AspNet.Http.Features.Tests/InterfaceDictionaryTests.cs +++ /dev/null @@ -1,83 +0,0 @@ -// 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 Xunit; - -namespace Microsoft.AspNet.Http.Features -{ - public class InterfaceDictionaryTests - { - [Fact] - public void AddedInterfaceIsReturned() - { - var interfaces = new FeatureCollection(); - var thing = new Thing(); - - interfaces.Add(typeof(IThing), thing); - - Assert.Equal(interfaces[typeof(IThing)], thing); - - object thing2; - Assert.True(interfaces.TryGetValue(typeof(IThing), out thing2)); - Assert.Equal(thing2, thing); - } - - [Fact] - public void IndexerAlsoAddsItems() - { - var interfaces = new FeatureCollection(); - var thing = new Thing(); - - interfaces[typeof(IThing)] = thing; - - Assert.Equal(interfaces[typeof(IThing)], thing); - - object thing2; - Assert.True(interfaces.TryGetValue(typeof(IThing), out thing2)); - Assert.Equal(thing2, thing); - } - - [Fact] - public void SecondCallToAddThrowsException() - { - var interfaces = new FeatureCollection(); - var thing = new Thing(); - - interfaces.Add(typeof(IThing), thing); - - Assert.Throws(() => interfaces.Add(typeof(IThing), thing)); - } - - [Fact] - public void RemovedInterfaceIsRemoved() - { - var interfaces = new FeatureCollection(); - var thing = new Thing(); - - interfaces.Add(typeof(IThing), thing); - - Assert.Equal(interfaces[typeof(IThing)], thing); - - Assert.True(interfaces.Remove(typeof(IThing))); - - object thing2; - Assert.False(interfaces.TryGetValue(typeof(IThing), out thing2)); - } - - [Fact] - public void SetNullValueRemoves() - { - var interfaces = new FeatureCollection(); - var thing = new Thing(); - - interfaces.Add(typeof(IThing), thing); - Assert.Equal(interfaces[typeof(IThing)], thing); - - interfaces[typeof(IThing)] = null; - - object thing2; - Assert.False(interfaces.TryGetValue(typeof(IThing), out thing2)); - } - } -} diff --git a/test/Microsoft.AspNet.Http.Tests/QueryFeatureTests.cs b/test/Microsoft.AspNet.Http.Tests/QueryFeatureTests.cs index 61d2ed4d34..9c5a84a9f5 100644 --- a/test/Microsoft.AspNet.Http.Tests/QueryFeatureTests.cs +++ b/test/Microsoft.AspNet.Http.Tests/QueryFeatureTests.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Http.Features.Internal var features = new FeatureCollection(); var request = new HttpRequestFeature(); request.QueryString = "foo=bar"; - features.Add(typeof(IHttpRequestFeature), request); + features[typeof(IHttpRequestFeature)] = request; var provider = new QueryFeature(features); diff --git a/test/Microsoft.AspNet.Owin.Tests/OwinFeatureCollectionTests.cs b/test/Microsoft.AspNet.Owin.Tests/OwinFeatureCollectionTests.cs index 74ddec0d46..b4958629f7 100644 --- a/test/Microsoft.AspNet.Owin.Tests/OwinFeatureCollectionTests.cs +++ b/test/Microsoft.AspNet.Owin.Tests/OwinFeatureCollectionTests.cs @@ -12,9 +12,9 @@ namespace Microsoft.AspNet.Owin { private T Get(IFeatureCollection features) { - object value; - return features.TryGetValue(typeof(T), out value) ? (T)value : default(T); + return (T)features[typeof(T)]; } + private T Get(IDictionary env, string key) { object value; @@ -63,22 +63,6 @@ namespace Microsoft.AspNet.Owin Assert.Equal("/pathBase2", Get(env, "owin.RequestPathBase")); Assert.Equal("name=value2", Get(env, "owin.RequestQueryString")); } - - [Fact] - public void ImplementedInterfacesAreEnumerated() - { - var env = new Dictionary - { - {"owin.RequestMethod", "POST"} - }; - var features = new OwinFeatureCollection(env); - - var entries = features.ToArray(); - var keys = features.Keys.ToArray(); - - Assert.Contains(typeof(IHttpRequestFeature), keys); - Assert.Contains(typeof(IHttpResponseFeature), keys); - } } }