serve/restic: Disallow overwriting files in append-only mode

[fd0]

This commits add tests for the HTTP handler for rclone serve restic in append-only mode. It is tested that files cannot be overwritten or deleted. See #2186 for some background.

Closes #2195

[fd0]

Hm. The failing test for AppVeyor on Windows seems to not be caused by my changes. @ncw can you please restart it?

[ncw]

Very nice, and particularly the tests - not familiar with httptest.ResponseRecorder - it is a very nice way of testing http servers I'll add to my toolkit! I like your functional testing style for testing lots of optional things too.

I had to add this

diff --git a/cmd/serve/restic/restic_appendonly_test.go b/cmd/serve/restic/restic_appendonly_test.go
index c7ae4116..cef033e1 100644
--- a/cmd/serve/restic/restic_appendonly_test.go
+++ b/cmd/serve/restic/restic_appendonly_test.go
@@ -175,6 +175,9 @@ func TestResticHandler(t *testing.T) {
 
 	// globally set append-only mode
 	appendOnly = true
+	defer func() {
+		appendOnly = false // reset when done
+	}()
 
 	// make a new file system in the temp dir
 	f := cmd.NewFsSrc([]string{tempdir})

To make the integration tests pass too (ie where rclone calls restics REST tests). Everyone loves global variables :-(

I normally use assert and require to check results from tests which are a very thin wrapper about the if / log, just saving a bit of typing and making consistent log outputs.

So

	if err != nil {
		t.Fatal(err)
	}

Could become

        require.NoError(err)

And

		if res.Code != code {
			t.Errorf("wrong response code, want %v, got %v", code, res.Code)
		}

could become (note params are t, want, got)

                assert.Equal(t, code, res.Code)

Anyway that is a minor thing and if you hate assert/require then don't bother changing them :-)

[fd0]

Thanks for the comments! I'll change the few minor things you mentioned. I'm fine with whatever testing style is preferred for the project :)

[fd0]

One thing that bugs me every time I see assert is that I lose the compile-time type checking. if uint64(0) == int(0) fails to compile, whereas for assert.Equal(t, uint64(0), int(0)) is only a runtime error (hopefully).

[fd0]

done, feel free to squash the commits if you like.

[fd0]

FYI, the testing style is not my idea, but is derived from some tests in stdlib (obviously), in this case here:

https://github.com/golang/go/blob/0250ef910f9a979a2151bd8e02b2641b74cf2f27/src/net/http/httptest/recorder_test.go#L114-L123

[ncw]

One thing that bugs me every time I see assert is that I lose the compile-time type checking. if uint64(0) == int(0) fails to compile, whereas for assert.Equal(t, uint64(0), int(0)) is only a runtime error (hopefully).

Yes that is certainly a slight annoyance of assert and one I've come across a few times. There are a few other little niggles too. For me, they outweight the repetitive typing though! Conceptually assert/require are very close to the standard go testing style, unlike goconvey (for instance) which always hurts my head when I have to contribute to a project using it!

Thanksf or the updates - will review/merge later today when I have more time!

[ncw]

Thank you very much for that - I've merged it now :-)