From b7939328b8cce972dd461a1a42b0e19775c8e159 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sat, 29 Sep 2018 18:42:38 -0700 Subject: [PATCH] feedback --- .../EndpointRoutingMiddleware.cs | 2 +- .../Matching/CandidateSet.cs | 68 +++++++++++++------ .../Matching/CandidateState.cs | 4 +- .../Matching/DefaultEndpointSelector.cs | 1 - .../Matching/CandidateSetTest.cs | 14 +++- 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs b/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs index 518d3b1202..b0d740b440 100644 --- a/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs +++ b/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs @@ -80,7 +80,7 @@ namespace Microsoft.AspNetCore.Routing private static void SetFeatures(HttpContext httpContext, EndpointSelectorContext context) { - // For back-compat EndpointRouteValuesFeature implements IEndpointFeature, + // For back-compat EndpointSelectorContext implements IEndpointFeature, // IRouteValuesFeature and IRoutingFeature httpContext.Features.Set(context); httpContext.Features.Set(context); diff --git a/src/Microsoft.AspNetCore.Routing/Matching/CandidateSet.cs b/src/Microsoft.AspNetCore.Routing/Matching/CandidateSet.cs index 0f38c8690d..4a004018b6 100644 --- a/src/Microsoft.AspNetCore.Routing/Matching/CandidateSet.cs +++ b/src/Microsoft.AspNetCore.Routing/Matching/CandidateSet.cs @@ -18,8 +18,9 @@ namespace Microsoft.AspNetCore.Routing.Matching { private const int BitVectorSize = 32; + // Cannot be readonly because we need to modify it in place. private BitVector32 _validity; - private BitArray _largeCapactityValidity; + private readonly BitArray _largeCapactityValidity; // We inline storage for 4 candidates here to avoid allocations in common // cases. There's no real reason why 4 is important, it just seemed like @@ -33,7 +34,8 @@ namespace Microsoft.AspNetCore.Routing.Matching /// /// - /// Initializes a new instances of the candidate set structure with the provided data. + /// Initializes a new instances of the class with the provided , + /// , and . /// /// /// The constructor is provided to enable unit tests of implementations of @@ -45,6 +47,26 @@ namespace Microsoft.AspNetCore.Routing.Matching /// The list of endpoint scores. . public CandidateSet(Endpoint[] endpoints, RouteValueDictionary[] values, int[] scores) { + if (endpoints == null) + { + throw new ArgumentNullException(nameof(endpoints)); + } + + if (values == null) + { + throw new ArgumentNullException(nameof(values)); + } + + if (scores == null) + { + throw new ArgumentNullException(nameof(scores)); + } + + if (endpoints.Length != values.Length || endpoints.Length != scores.Length) + { + throw new ArgumentException($"The provided {nameof(endpoints)}, {nameof(values)}, and {nameof(scores)} must have the same length."); + } + Count = endpoints.Length; switch (endpoints.Length) @@ -53,37 +75,37 @@ namespace Microsoft.AspNetCore.Routing.Matching return; case 1: - _state0 = new CandidateState(endpoints[0], score: scores[0]); + _state0 = new CandidateState(endpoints[0], values[0], scores[0]); break; case 2: - _state0 = new CandidateState(endpoints[0], score: scores[0]); - _state1 = new CandidateState(endpoints[1], score: scores[1]); + _state0 = new CandidateState(endpoints[0], values[0], scores[0]); + _state1 = new CandidateState(endpoints[1], values[1], scores[1]); break; case 3: - _state0 = new CandidateState(endpoints[0], score: scores[0]); - _state1 = new CandidateState(endpoints[1], score: scores[1]); - _state2 = new CandidateState(endpoints[2], score: scores[2]); + _state0 = new CandidateState(endpoints[0], values[0], scores[0]); + _state1 = new CandidateState(endpoints[1], values[1], scores[1]); + _state2 = new CandidateState(endpoints[2], values[2], scores[2]); break; case 4: - _state0 = new CandidateState(endpoints[0], score: scores[0]); - _state1 = new CandidateState(endpoints[1], score: scores[1]); - _state2 = new CandidateState(endpoints[2], score: scores[2]); - _state3 = new CandidateState(endpoints[3], score: scores[3]); + _state0 = new CandidateState(endpoints[0], values[0], scores[0]); + _state1 = new CandidateState(endpoints[1], values[1], scores[1]); + _state2 = new CandidateState(endpoints[2], values[2], scores[2]); + _state3 = new CandidateState(endpoints[3], values[3], scores[3]); break; default: - _state0 = new CandidateState(endpoints[0], score: scores[0]); - _state1 = new CandidateState(endpoints[1], score: scores[1]); - _state2 = new CandidateState(endpoints[2], score: scores[2]); - _state3 = new CandidateState(endpoints[3], score: scores[3]); + _state0 = new CandidateState(endpoints[0], values[0], scores[0]); + _state1 = new CandidateState(endpoints[1], values[1], scores[1]); + _state2 = new CandidateState(endpoints[2], values[2], scores[2]); + _state3 = new CandidateState(endpoints[3], values[3], scores[3]); _additionalCandidates = new CandidateState[endpoints.Length - 4]; for (var i = 4; i < endpoints.Length; i++) { - _additionalCandidates[i - 4] = new CandidateState(endpoints[i], score: scores[i]); + _additionalCandidates[i - 4] = new CandidateState(endpoints[i], values[i], scores[i]); } break; } @@ -161,7 +183,7 @@ namespace Microsoft.AspNetCore.Routing.Matching /// Gets the count of candidates in the set. /// public int Count { get; } - + /// /// Gets the associated with the candidate /// at . @@ -206,10 +228,14 @@ namespace Microsoft.AspNetCore.Routing.Matching } /// - /// Gets or sets a value which indicates where the is considered - /// a valid candiate for the current request. Set this value to false to exclude an - /// from consideration. + /// Gets a value which indicates where the is considered + /// a valid candiate for the current request. /// + /// The candidate index. + /// + /// true if the candidate at position is considered value + /// for the current request, otherwise false. + /// public bool IsValidCandidate(int index) { // Friendliness for inlining diff --git a/src/Microsoft.AspNetCore.Routing/Matching/CandidateState.cs b/src/Microsoft.AspNetCore.Routing/Matching/CandidateState.cs index 03bccd517b..5b198beb8e 100644 --- a/src/Microsoft.AspNetCore.Routing/Matching/CandidateState.cs +++ b/src/Microsoft.AspNetCore.Routing/Matching/CandidateState.cs @@ -6,7 +6,7 @@ using Microsoft.AspNetCore.Http; namespace Microsoft.AspNetCore.Routing.Matching { /// - /// The mutable state associated with a candidate in a . + /// The state associated with a candidate in a . /// public struct CandidateState { @@ -47,7 +47,7 @@ namespace Microsoft.AspNetCore.Routing.Matching public int Score { get; } /// - /// Gets or sets the associated with the + /// Gets associated with the /// and the current request. /// public RouteValueDictionary Values { get; internal set; } diff --git a/src/Microsoft.AspNetCore.Routing/Matching/DefaultEndpointSelector.cs b/src/Microsoft.AspNetCore.Routing/Matching/DefaultEndpointSelector.cs index cf38ffbb51..bd670d86f5 100644 --- a/src/Microsoft.AspNetCore.Routing/Matching/DefaultEndpointSelector.cs +++ b/src/Microsoft.AspNetCore.Routing/Matching/DefaultEndpointSelector.cs @@ -116,7 +116,6 @@ namespace Microsoft.AspNetCore.Routing.Matching var isValid = candidateSet.IsValidCandidate(i); if (isValid && foundScore == null) { - // This is the first match we've seen - speculatively assign it. endpoint = state.Endpoint; values = state.Values; diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Matching/CandidateSetTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Matching/CandidateSetTest.cs index a80e4dd119..b94923df3c 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Matching/CandidateSetTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Matching/CandidateSetTest.cs @@ -76,8 +76,17 @@ namespace Microsoft.AspNetCore.Routing.Matching endpoints[i] = CreateEndpoint($"/{i}"); } + var values = new RouteValueDictionary[count]; + for (var i = 0; i < endpoints.Length; i++) + { + values[i] = new RouteValueDictionary() + { + { "i", i } + }; + } + // Act - var candidateSet = new CandidateSet(endpoints, new RouteValueDictionary[count], Enumerable.Range(0, count).ToArray()); + var candidateSet = new CandidateSet(endpoints, values, Enumerable.Range(0, count).ToArray()); // Assert for (var i = 0; i < candidateSet.Count; i++) @@ -86,7 +95,8 @@ namespace Microsoft.AspNetCore.Routing.Matching Assert.True(candidateSet.IsValidCandidate(i)); Assert.Same(endpoints[i], state.Endpoint); Assert.Equal(i, state.Score); - Assert.Null(state.Values); + Assert.NotNull(state.Values); + Assert.Equal(i, state.Values["i"]); candidateSet.SetValidity(i, false); Assert.False(candidateSet.IsValidCandidate(i));