From 75fc3fe389efc5270acd4da62ae6fb40e0be97d2 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Fri, 24 Apr 2020 11:24:57 +0100 Subject: [PATCH] fs: fix FixRangeOption so it doesn't add HTTPOptions in place of bad Ranges Before this fix, FixRangeOption would substitute RangeOptions it wanted to get rid of with with empty HTTPOption. This caused a problem now that backends interpret HTTPOptions. This fix subsitutes those with NullOptions which aren't interpreted as HTTPOptions. This patch also improves the unit tests. --- fs/options.go | 34 +++++--- fs/options_test.go | 196 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 11 deletions(-) diff --git a/fs/options.go b/fs/options.go index 56eaea4f2..399b0a561 100644 --- a/fs/options.go +++ b/fs/options.go @@ -42,8 +42,8 @@ type OpenOption interface { // // RangeOption{Start: 0, End: 99} - fetch the first 100 bytes // RangeOption{Start: 100, End: 199} - fetch the second 100 bytes -// RangeOption{Start: 100} - fetch bytes from offset 100 to the end -// RangeOption{End: 100} - fetch the last 100 bytes +// RangeOption{Start: 100, End: -1} - fetch bytes from offset 100 to the end +// RangeOption{Start: -1, End: 100} - fetch the last 100 bytes // // A RangeOption implements a single byte-range-spec from // https://tools.ietf.org/html/rfc7233#section-2.1 @@ -141,10 +141,10 @@ func (o *RangeOption) Decode(size int64) (offset, limit int64) { func FixRangeOption(options []OpenOption, size int64) { if size == 0 { // if size 0 then remove RangeOption~s - // replacing with an empty HTTPOption~s which won't be rendered + // replacing with an NullOptions~s which won't be rendered for i := range options { if _, ok := options[i].(*RangeOption); ok { - options[i] = &HTTPOption{} + options[i] = NullOption{} } } @@ -230,6 +230,25 @@ func (o *HashesOption) Mandatory() bool { return false } +// NullOption defines an Option which does nothing +type NullOption struct { +} + +// Header formats the option as an http header +func (o NullOption) Header() (key string, value string) { + return "", "" +} + +// String formats the option into human readable form +func (o NullOption) String() string { + return fmt.Sprintf("NullOption()") +} + +// Mandatory returns whether the option must be parsed or can be ignored +func (o NullOption) Mandatory() bool { + return false +} + // OpenOptionAddHeaders adds each header found in options to the // headers map provided the key was non empty. func OpenOptionAddHeaders(options []OpenOption, headers map[string]string) { @@ -264,10 +283,3 @@ func OpenOptionAddHTTPHeaders(headers http.Header, options []OpenOption) { } } } - -// check interface -var ( - _ OpenOption = (*RangeOption)(nil) - _ OpenOption = (*SeekOption)(nil) - _ OpenOption = (*HTTPOption)(nil) -) diff --git a/fs/options_test.go b/fs/options_test.go index bad992390..d02531078 100644 --- a/fs/options_test.go +++ b/fs/options_test.go @@ -2,8 +2,10 @@ package fs import ( "fmt" + "net/http" "testing" + "github.com/rclone/rclone/fs/hash" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -58,3 +60,197 @@ func TestRangeOptionDecode(t *testing.T) { assert.Equal(t, test.wantLimit, gotLimit, "limit "+what) } } + +func TestRangeOption(t *testing.T) { + opt := &RangeOption{Start: 1, End: 10} + var _ OpenOption = opt // check interface + assert.Equal(t, "RangeOption(1,10)", opt.String()) + key, value := opt.Header() + assert.Equal(t, "Range", key) + assert.Equal(t, "bytes=1-10", value) + assert.Equal(t, true, opt.Mandatory()) + + opt = &RangeOption{Start: -1, End: 10} + assert.Equal(t, "RangeOption(-1,10)", opt.String()) + key, value = opt.Header() + assert.Equal(t, "Range", key) + assert.Equal(t, "bytes=-10", value) + assert.Equal(t, true, opt.Mandatory()) + + opt = &RangeOption{Start: 1, End: -1} + assert.Equal(t, "RangeOption(1,-1)", opt.String()) + key, value = opt.Header() + assert.Equal(t, "Range", key) + assert.Equal(t, "bytes=1-", value) + assert.Equal(t, true, opt.Mandatory()) + + opt = &RangeOption{Start: -1, End: -1} + assert.Equal(t, "RangeOption(-1,-1)", opt.String()) + key, value = opt.Header() + assert.Equal(t, "Range", key) + assert.Equal(t, "bytes=-", value) + assert.Equal(t, true, opt.Mandatory()) +} + +func TestSeekOption(t *testing.T) { + opt := &SeekOption{Offset: 1} + var _ OpenOption = opt // check interface + assert.Equal(t, "SeekOption(1)", opt.String()) + key, value := opt.Header() + assert.Equal(t, "Range", key) + assert.Equal(t, "bytes=1-", value) + assert.Equal(t, true, opt.Mandatory()) +} + +func TestHTTPOption(t *testing.T) { + opt := &HTTPOption{Key: "k", Value: "v"} + var _ OpenOption = opt // check interface + assert.Equal(t, `HTTPOption("k","v")`, opt.String()) + key, value := opt.Header() + assert.Equal(t, "k", key) + assert.Equal(t, "v", value) + assert.Equal(t, false, opt.Mandatory()) +} + +func TestHashesOption(t *testing.T) { + opt := &HashesOption{hash.Set(hash.MD5 | hash.SHA1)} + var _ OpenOption = opt // check interface + assert.Equal(t, `HashesOption([MD5, SHA-1])`, opt.String()) + key, value := opt.Header() + assert.Equal(t, "", key) + assert.Equal(t, "", value) + assert.Equal(t, false, opt.Mandatory()) +} + +func TestNullOption(t *testing.T) { + opt := NullOption{} + var _ OpenOption = opt // check interface + assert.Equal(t, "NullOption()", opt.String()) + key, value := opt.Header() + assert.Equal(t, "", key) + assert.Equal(t, "", value) + assert.Equal(t, false, opt.Mandatory()) +} + +func TestFixRangeOptions(t *testing.T) { + for _, test := range []struct { + name string + in []OpenOption + size int64 + want []OpenOption + }{ + { + name: "Nil options", + in: nil, + want: nil, + }, + { + name: "Empty options", + in: []OpenOption{}, + want: []OpenOption{}, + }, + { + name: "Fetch a range with size=0", + in: []OpenOption{ + &HTTPOption{Key: "a", Value: "1"}, + &RangeOption{Start: 1, End: 10}, + &HTTPOption{Key: "b", Value: "2"}, + }, + want: []OpenOption{ + &HTTPOption{Key: "a", Value: "1"}, + NullOption{}, + &HTTPOption{Key: "b", Value: "2"}, + }, + size: 0, + }, + { + name: "Fetch a range", + in: []OpenOption{ + &HTTPOption{Key: "a", Value: "1"}, + &RangeOption{Start: 1, End: 10}, + &HTTPOption{Key: "b", Value: "2"}, + }, + want: []OpenOption{ + &HTTPOption{Key: "a", Value: "1"}, + &RangeOption{Start: 1, End: 10}, + &HTTPOption{Key: "b", Value: "2"}, + }, + size: 100, + }, + { + name: "Fetch to end", + in: []OpenOption{ + &RangeOption{Start: 1, End: -1}, + }, + want: []OpenOption{ + &RangeOption{Start: 1, End: -1}, + }, + size: 100, + }, + { + name: "Fetch the last 10 bytes", + in: []OpenOption{ + &RangeOption{Start: -1, End: 10}, + }, + want: []OpenOption{ + &RangeOption{Start: 90, End: -1}, + }, + size: 100, + }, + { + name: "Fetch with end bigger than size", + in: []OpenOption{ + &RangeOption{Start: 10, End: 200}, + }, + want: []OpenOption{ + &RangeOption{Start: 10, End: 99}, + }, + size: 100, + }, + } { + FixRangeOption(test.in, test.size) + assert.Equal(t, test.want, test.in, test.name) + } +} + +var testOpenOptions = []OpenOption{ + &HTTPOption{Key: "a", Value: "1"}, + &RangeOption{Start: 1, End: 10}, + &HTTPOption{Key: "b", Value: "2"}, + NullOption{}, + &HashesOption{hash.Set(hash.MD5 | hash.SHA1)}, +} + +func TestOpenOptionAddHeaders(t *testing.T) { + m := map[string]string{} + want := map[string]string{ + "a": "1", + "Range": "bytes=1-10", + "b": "2", + } + OpenOptionAddHeaders(testOpenOptions, m) + assert.Equal(t, want, m) +} + +func TestOpenOptionHeaders(t *testing.T) { + want := map[string]string{ + "a": "1", + "Range": "bytes=1-10", + "b": "2", + } + m := OpenOptionHeaders(testOpenOptions) + assert.Equal(t, want, m) + assert.Nil(t, OpenOptionHeaders([]OpenOption{})) +} + +func TestOpenOptionAddHTTPHeaders(t *testing.T) { + headers := http.Header{} + want := http.Header{ + "A": {"1"}, + "Range": {"bytes=1-10"}, + "B": {"2"}, + } + OpenOptionAddHTTPHeaders(headers, testOpenOptions) + assert.Equal(t, want, headers) + +}