Add ByteRangeHelper #807

jbagga · 2017-03-31T21:31:46Z

Moved RangeHelper from StaticFiles and added tests

dnfclas · 2017-03-31T21:31:50Z

@jbagga,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

pranavkm · 2017-03-31T23:33:47Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+
+namespace Microsoft.AspNetCore.WebUtilities
+{
+    internal static class ByteRangeHelper


Aren't you using this type outside this repo?

pranavkm · 2017-03-31T23:33:56Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+        // Adjusts ranges to be absolute and within bounds.
+        internal static IList<RangeItemHeaderValue> NormalizeRanges(ICollection<RangeItemHeaderValue> ranges, long length)
+        {
+            IList<RangeItemHeaderValue> normalizedRanges = new List<RangeItemHeaderValue>(ranges.Count);


var all day everyday

jbagga · 2017-04-03T18:22:32Z

@pranavkm @Tratcher Updated

pranavkm

Once @Tratcher is happy

pranavkm · 2017-04-03T19:15:24Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+        // Adjusts ranges to be absolute and within bounds.
+        public static IList<RangeItemHeaderValue> NormalizeRanges(ICollection<RangeItemHeaderValue> ranges, long length)
+        {
+            var normalizedRanges = new List<RangeItemHeaderValue>(ranges.Count);


if (ranges.Count == 0) { return Array.Empty<RangeItemHeaderValue>(); } var normalizedRanges = new List<RangeItemHeaderValue>(ranges.Count); ...

Tratcher · 2017-04-03T20:50:53Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+        // first-byte-pos is less than the current length of the entity-body, or at least one suffix-byte-range-spec
+        // with a non-zero suffix-length, then the byte-range-set is satisfiable.
+        // Adjusts ranges to be absolute and within bounds.
+        public static IList<RangeItemHeaderValue> NormalizeRanges(ICollection<RangeItemHeaderValue> ranges, long length)


Split out a second method public static RangeItemHeaderValue NormalizeRange(RangeItemHeaderValue range, long length). Most of your tests only apply to this subset of the functionality.

jbagga · 2017-04-03T22:34:40Z

@pranavkm @Tratcher Updated

pranavkm · 2017-04-03T22:39:32Z

test/Microsoft.AspNetCore.WebUtilities.Tests/ByteRangeHelperTest.cs

@@ -2,13 +2,27 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

 using Microsoft.Net.Http.Headers;
+using System.Collections.Generic;


System.* goes first

Tratcher · 2017-04-03T23:19:44Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+
+            var normalizedRanges = new List<RangeItemHeaderValue>(ranges.Count);
+
+            if (length == 0)


Why doesn't this return Array.Empty<RangeItemHeaderValue>(); too?

Tratcher · 2017-04-03T23:23:24Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+            {
+                if (start.Value >= length)
+                {
+                    // Not satisfiable, skip/discard.


Hmm, my idea to make this a sub method just regressed this bit. You removed the continue that would have skipped adding the result. The behavior is now quite different. Is there a test case for this condition?
https://github.com/aspnet/StaticFiles/blob/f73dc5cebea6c08485662f1703d2aba1fc0cd408/src/Microsoft.AspNetCore.StaticFiles/Infrastructure/RangeHelpers.cs#L34

jbagga · 2017-04-03T23:54:15Z

@Tratcher Added test cases to skip invalid ranges. Thank you for pointing it out!

jbagga · 2017-04-06T18:52:16Z

@Tratcher @pranavkm Added another method and tests

Tratcher · 2017-04-06T19:01:02Z

src/Microsoft.AspNetCore.WebUtilities/ByteRangeHelper.cs

+            return normalizedRange;
+        }
+
+        public static bool ParseRange(HttpContext context, DateTimeOffset lastModified, EntityTagHeaderValue etag)


This is less efficient the the original in StaticFiles. GetTypedHeaders creates a new structure (which you call multiple times) and .Range parses the header again without any caching. If you want to call it ParseRange then at least return the Range object.

Doc comments

Tratcher · 2017-04-06T19:03:52Z

src/Microsoft.AspNetCore.WebUtilities/Microsoft.AspNetCore.WebUtilities.csproj

@@ -13,6 +13,8 @@

  <ItemGroup>
    <ProjectReference Include="..\Microsoft.Net.Http.Headers\Microsoft.Net.Http.Headers.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Http.Abstractions\Microsoft.AspNetCore.Http.Abstractions.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Http.Extensions\Microsoft.AspNetCore.Http.Extensions.csproj" />


This layering is problematic. WebUtilities should not add these dependencies which makes it impossible to write your method here... I think you'll have to move it to Http.Extensions reduce the inputs.

jbagga · 2017-04-06T21:44:16Z

@Tratcher @pranavkm Added doc comments and moved ParseRange to Extensions

Tratcher · 2017-04-06T23:07:15Z

src/Microsoft.AspNetCore.Http.Extensions/RangeParser.cs

+        /// <param name="lastModified">The <see cref="DateTimeOffset"/> representation of the last modified date of the file.</param>
+        /// <param name="etag">The <see cref="EntityTagHeaderValue"/> provided in the <see cref="HttpContext.Request"/>.</param>
+        /// <returns>A collection of <see cref="RangeItemHeaderValue"/> containing the ranges parsed from the <paramref name="requestHeaders"/>.</returns>
+        public static ICollection<RangeItemHeaderValue> ParseRange(HttpContext context, RequestHeaders requestHeaders, DateTimeOffset lastModified, EntityTagHeaderValue etag)


Some of your MVC file result types won't always have an etag or lastmodified right? What will you do there? or will null work as expected?

Tratcher · 2017-04-06T23:17:51Z

src/Microsoft.AspNetCore.Http.Extensions/RangeParser.cs

+                {
+                    ignoreRangeHeader = true;
+                }
+                if (ignoreRangeHeader)


new line so this doesn't look like an else-if

jbagga · 2017-04-07T00:34:44Z

@Tratcher I added null checks and made lastModified and etag optional params. Is that acceptable?

Tratcher · 2017-04-07T02:54:22Z

src/Microsoft.AspNetCore.Http.Extensions/RangeParser.cs

+        /// <param name="lastModified">The <see cref="DateTimeOffset"/> representation of the last modified date of the file.</param>
+        /// <param name="etag">The <see cref="EntityTagHeaderValue"/> provided in the <see cref="HttpContext.Request"/>.</param>
+        /// <returns>A collection of <see cref="RangeItemHeaderValue"/> containing the ranges parsed from the <paramref name="requestHeaders"/>.</returns>
+        public static ICollection<RangeItemHeaderValue> ParseRange(HttpContext context, RequestHeaders requestHeaders, DateTimeOffset? lastModified = null, EntityTagHeaderValue etag = null)


Default params are not allowed in most of our APIs.

Tratcher · 2017-04-07T02:54:54Z

src/Microsoft.AspNetCore.Http.Extensions/RangeParser.cs

+                bool ignoreRangeHeader = false;
+                if (ifRangeHeader.LastModified.HasValue)
+                {
+                    if (lastModified != null && lastModified > ifRangeHeader.LastModified)


!lastModified.HasValue

jbagga · 2017-04-07T21:17:09Z

Closing as this has been addressed in aspnet/StaticFiles#185

dnfclas added the cla-already-signed label Mar 31, 2017

jbagga requested a review from Tratcher March 31, 2017 21:32

jbagga assigned Tratcher Mar 31, 2017

pranavkm suggested changes Mar 31, 2017

View reviewed changes

pranavkm approved these changes Apr 3, 2017

View reviewed changes

Tratcher reviewed Apr 3, 2017

View reviewed changes

jbagga force-pushed the jbagga/ByteRangeHelper branch from b3a42c8 to 4cdc928 Compare April 3, 2017 22:18

pranavkm reviewed Apr 3, 2017

View reviewed changes

Tratcher suggested changes Apr 3, 2017

View reviewed changes

Tratcher approved these changes Apr 4, 2017

View reviewed changes

jbagga added 6 commits April 4, 2017 14:18

Add ByteRangeHelper

4cad645

Change accessibility to public; use var

fa18958

Split method into two

8a7a8e3

Range count check

f0294de

Sort usings

9f29dcf

Skip invalid ranges

a6b7e99

Tratcher removed their assignment Apr 5, 2017

jbagga added 3 commits April 6, 2017 11:14

Add ParseRange

083279c

Add project reference

c6ce6eb

Add necessary ProjectReferences

640e0ac

jbagga force-pushed the jbagga/ByteRangeHelper branch from 3cbdada to 640e0ac Compare April 6, 2017 18:51

jbagga mentioned this pull request Apr 6, 2017

Move RangeHelper to a shared sources package aspnet/StaticFiles#182

Closed

Tratcher suggested changes Apr 6, 2017

View reviewed changes

jbagga added 2 commits April 6, 2017 13:46

Feedback addressed

98021aa

add copyright license

efe8262

jbagga added 2 commits April 6, 2017 14:17

Removed test

6507259

Added doc comments

d67900c

Tratcher approved these changes Apr 6, 2017

View reviewed changes

Tratcher reviewed Apr 6, 2017

View reviewed changes

Added null checks and test

02ff081

Tratcher approved these changes Apr 7, 2017

View reviewed changes

jbagga closed this Apr 7, 2017

natemcmaster deleted the jbagga/ByteRangeHelper branch November 20, 2018 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ByteRangeHelper #807

Add ByteRangeHelper #807

jbagga commented Mar 31, 2017 •

edited

Loading

dnfclas commented Mar 31, 2017

pranavkm Mar 31, 2017

pranavkm Mar 31, 2017

jbagga commented Apr 3, 2017

pranavkm left a comment

pranavkm Apr 3, 2017

Tratcher Apr 3, 2017

jbagga commented Apr 3, 2017

pranavkm Apr 3, 2017

Tratcher Apr 3, 2017

Tratcher Apr 3, 2017

jbagga commented Apr 3, 2017

jbagga commented Apr 6, 2017

Tratcher Apr 6, 2017

Tratcher Apr 6, 2017

Tratcher Apr 6, 2017

jbagga commented Apr 6, 2017

Tratcher Apr 6, 2017

Tratcher Apr 6, 2017

jbagga commented Apr 7, 2017

Tratcher Apr 7, 2017

Tratcher Apr 7, 2017

jbagga commented Apr 7, 2017


		var normalizedRanges = new List<RangeItemHeaderValue>(ranges.Count);

		if (length == 0)

Add ByteRangeHelper #807

Add ByteRangeHelper #807

Conversation

jbagga commented Mar 31, 2017 • edited Loading

dnfclas commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 3, 2017

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 3, 2017

jbagga commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Apr 7, 2017

jbagga commented Mar 31, 2017 •

edited

Loading