Skip to content

Implement idempotency into the final concat request #1365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ollie-sutton wants to merge 1 commit into
base: main
Choose a base branch
from
+292 −17
Open

Implement idempotency into the final concat request #1365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 192 additions & 4 deletions pkg/handler/concat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ func TestConcat(t *testing.T) {
uploadB := NewMockFullUpload(ctrl)
uploadC := NewMockFullUpload(ctrl)

concatID := "concat-7e18f737311b2dc3b2f269dd78396b03"

gomock.InOrder(
store.EXPECT().GetUpload(gomock.Any(), "a").Return(uploadA, nil),
uploadA.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
Expand All @@ -125,15 +127,18 @@ func TestConcat(t *testing.T) {
Size: 5,
Offset: 5,
}, nil),
// Idempotency check: look up the deterministic concat ID
store.EXPECT().GetUpload(gomock.Any(), concatID).Return(nil, ErrNotFound),
store.EXPECT().NewUpload(gomock.Any(), FileInfo{
ID: concatID,
Size: 10,
IsPartial: false,
IsFinal: true,
PartialUploads: []string{"a", "b"},
MetaData: make(map[string]string),
}).Return(uploadC, nil),
uploadC.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
ID: "foo",
ID: concatID,
Size: 10,
IsPartial: false,
IsFinal: true,
Expand All @@ -149,7 +154,7 @@ func TestConcat(t *testing.T) {
StoreComposer: composer,
NotifyCompleteUploads: true,
PreFinishResponseCallback: func(hook HookEvent) (HTTPResponse, error) {
a.Equal("foo", hook.Upload.ID)
a.Equal(concatID, hook.Upload.ID)
return HTTPResponse{
Header: HTTPHeader{
"X-Custom-Resp-Header": "hello",
Expand Down Expand Up @@ -179,7 +184,7 @@ func TestConcat(t *testing.T) {

event := <-c
info := event.Upload
a.Equal("foo", info.ID)
a.Equal(concatID, info.ID)
a.EqualValues(10, info.Size)
a.EqualValues(10, info.Offset)
a.False(info.IsPartial)
Expand Down Expand Up @@ -338,6 +343,184 @@ func TestConcat(t *testing.T) {
}).Run(handler, t)
})

// Test idempotent retry when concat already completed successfully.
SubTest(t, "IdempotentRetryComplete", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) {
a := assert.New(t)

ctrl := gomock.NewController(t)
defer ctrl.Finish()
uploadA := NewMockFullUpload(ctrl)
uploadB := NewMockFullUpload(ctrl)
uploadC := NewMockFullUpload(ctrl)

concatID := "concat-7e18f737311b2dc3b2f269dd78396b03"

gomock.InOrder(
store.EXPECT().GetUpload(gomock.Any(), "a").Return(uploadA, nil),
uploadA.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
store.EXPECT().GetUpload(gomock.Any(), "b").Return(uploadB, nil),
uploadB.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
// Idempotency check: upload already exists and is complete
store.EXPECT().GetUpload(gomock.Any(), concatID).Return(uploadC, nil),
uploadC.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
ID: concatID,
Size: 10,
Offset: 10,
IsFinal: true,
PartialUploads: []string{"a", "b"},
}, nil),
// No NewUpload or ConcatUploads should be called
)

handler, _ := NewHandler(Config{
BasePath: "files",
StoreComposer: composer,
NotifyCompleteUploads: true,
})

c := make(chan HookEvent, 1)
handler.CompleteUploads = c

(&httpTest{
Method: "POST",
ReqHeader: map[string]string{
"Tus-Resumable": "1.0.0",
"Upload-Concat": "final;http://tus.io/files/a /files/b",
},
Code: http.StatusCreated,
}).Run(handler, t)

event := <-c
info := event.Upload
a.Equal(concatID, info.ID)
a.EqualValues(10, info.Size)
a.EqualValues(10, info.Offset)
a.True(info.IsFinal)
})

// Test idempotent retry when upload was created but concat didn't complete.
SubTest(t, "IdempotentRetryIncomplete", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) {
a := assert.New(t)

ctrl := gomock.NewController(t)
defer ctrl.Finish()
uploadA := NewMockFullUpload(ctrl)
uploadB := NewMockFullUpload(ctrl)
uploadC := NewMockFullUpload(ctrl)

concatID := "concat-7e18f737311b2dc3b2f269dd78396b03"

gomock.InOrder(
store.EXPECT().GetUpload(gomock.Any(), "a").Return(uploadA, nil),
uploadA.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
store.EXPECT().GetUpload(gomock.Any(), "b").Return(uploadB, nil),
uploadB.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
// Idempotency check: upload exists but concat hasn't happened yet
store.EXPECT().GetUpload(gomock.Any(), concatID).Return(uploadC, nil),
uploadC.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
ID: concatID,
Size: 10,
Offset: 0,
IsFinal: true,
PartialUploads: []string{"a", "b"},
}, nil),
// Should retry the concatenation
store.EXPECT().AsConcatableUpload(uploadC).Return(uploadC),
uploadC.EXPECT().ConcatUploads(gomock.Any(), []Upload{uploadA, uploadB}).Return(nil),
)

handler, _ := NewHandler(Config{
BasePath: "files",
StoreComposer: composer,
NotifyCompleteUploads: true,
})

c := make(chan HookEvent, 1)
handler.CompleteUploads = c

(&httpTest{
Method: "POST",
ReqHeader: map[string]string{
"Tus-Resumable": "1.0.0",
"Upload-Concat": "final;http://tus.io/files/a /files/b",
},
Code: http.StatusCreated,
}).Run(handler, t)

event := <-c
info := event.Upload
a.Equal(concatID, info.ID)
a.EqualValues(10, info.Size)
a.EqualValues(10, info.Offset)
a.True(info.IsFinal)
})

// Test that a partially corrupted concat (0 < offset < size) returns an error.
SubTest(t, "IdempotentRetryCorrupted", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
uploadA := NewMockFullUpload(ctrl)
uploadB := NewMockFullUpload(ctrl)
uploadC := NewMockFullUpload(ctrl)

concatID := "concat-7e18f737311b2dc3b2f269dd78396b03"

gomock.InOrder(
store.EXPECT().GetUpload(gomock.Any(), "a").Return(uploadA, nil),
uploadA.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
store.EXPECT().GetUpload(gomock.Any(), "b").Return(uploadB, nil),
uploadB.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
IsPartial: true,
Size: 5,
Offset: 5,
}, nil),
// Idempotency check: upload exists with partial data (corrupted)
store.EXPECT().GetUpload(gomock.Any(), concatID).Return(uploadC, nil),
uploadC.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
ID: concatID,
Size: 10,
Offset: 3,
IsFinal: true,
PartialUploads: []string{"a", "b"},
}, nil),
)

handler, _ := NewHandler(Config{
BasePath: "files",
StoreComposer: composer,
})

(&httpTest{
Method: "POST",
ReqHeader: map[string]string{
"Tus-Resumable": "1.0.0",
"Upload-Concat": "final;http://tus.io/files/a /files/b",
},
Code: http.StatusInternalServerError,
ResBody: "ERR_CONCAT_CORRUPTED: previous concatenation attempt was partially completed and left the upload in an inconsistent state\n",
}).Run(handler, t)
})

// Test that we can concatenate uploads, whose IDs contain slashes.
SubTest(t, "UploadIDsWithSlashes", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) {
ctrl := gomock.NewController(t)
Expand All @@ -346,6 +529,8 @@ func TestConcat(t *testing.T) {
uploadB := NewMockFullUpload(ctrl)
uploadC := NewMockFullUpload(ctrl)

concatID := "concat-0f2c19317a7803781021b9b987dc84e7"

gomock.InOrder(
store.EXPECT().GetUpload(gomock.Any(), "aaa/123").Return(uploadA, nil),
uploadA.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
Expand All @@ -359,15 +544,18 @@ func TestConcat(t *testing.T) {
Size: 5,
Offset: 5,
}, nil),
// Idempotency check: look up the deterministic concat ID
store.EXPECT().GetUpload(gomock.Any(), concatID).Return(nil, ErrNotFound),
store.EXPECT().NewUpload(gomock.Any(), FileInfo{
ID: concatID,
Size: 10,
IsPartial: false,
IsFinal: true,
PartialUploads: []string{"aaa/123", "bbb/123"},
MetaData: make(map[string]string),
}).Return(uploadC, nil),
uploadC.EXPECT().GetInfo(gomock.Any()).Return(FileInfo{
ID: "foo",
ID: concatID,
Size: 10,
IsPartial: false,
IsFinal: true,
Expand Down
Loading