Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
2.0.0
-
Security Level: Public
-
None
-
CBG Sprint 22
-
3
Description
Background
Whilst working on CBG-342 - I noticed the Get handling in Incr for when amt=0 is not taking into account the passed in "def" parameter. This seems wrong, and looks like it might cause issues when Incr is called with a zero amt non-zero def values.
A quick unit test comparing Walrus and gocb buckets show a mismatch of return values for the given Incr call.
Another thing to note is that the intial return value of Incr via Get is never persisted, which means non-zero def values are going to mess up the semantics of Incr on the bucket.
Proposed fix
When we do the initial Get with amt=0 and a non-zero def, we should write this to the bucket, using CAS to make sure we've not had the doc created underneath us in the meantime.
Unit/integration test to demonstrate issue
// TestIncrAmtZero covers the special handling in Incr for when amt=0 on an unknown key
|
func TestIncrAmtZero(t *testing.T) {
|
|
|
key := "TestIncrAmtZero"
|
|
|
bucket := GetTestBucketOrPanic()
|
defer bucket.Close()
|
|
|
// key hasn't been created, so we'll fall into the special 'Get' handling in Incr
|
val, err := bucket.Incr(key, 0, 5, 0)
|
assert.NoError(t, err)
|
assert.Equal(t, uint64(5), val)
|
|
|
// Actually increment key to create it
|
val, err = bucket.Incr(key, 1, 5, 0)
|
assert.NoError(t, err)
|
assert.Equal(t, uint64(6), val)
|
|
|
// Do another amt=0 to make sure we get the new incremented value
|
val, err = bucket.Incr(key, 0, 5, 0)
|
assert.NoError(t, err)
|
assert.Equal(t, uint64(6), val)
|
}
|
14:59 $ go test -run='^TestIncrAmtZero$' -v ./base
|
=== RUN TestIncrAmtZero
|
2019-04-30T14:59:33.996+01:00 [INF] Opening Walrus database test_data_bucket-83ff19130b65933d4dbefad8279e5ebd on <walrus:>
|
--- PASS: TestIncrAmtZero (0.00s)
|
PASS
|
ok github.com/couchbase/sync_gateway/base 0.019s
|
|
|
14:59 $ SG_TEST_BACKING_STORE=couchbase SG_TEST_COUCHBASE_SERVER_URL=couchbase://10.112.191.101 go test -run='^TestIncrAmtZero$' -v ./base
|
=== RUN TestIncrAmtZero
|
2019-04-30T14:59:21.963+01:00 [INF] Successfully opened bucket test_data_bucket
|
2019-04-30T14:59:21.972+01:00 [INF] Set query timeouts for bucket test_data_bucket to cluster:1m15s, bucket:1m15s
|
2019-04-30T14:59:21.972+01:00 [INF] Flushing bucket test_data_bucket
|
2019-04-30T14:59:24.103+01:00 [INF] GoCBCustomSGTranscoder Opening Couchbase database test_data_bucket on <couchbase://10.112.191.101> as user "test_data_bucket"
|
2019-04-30T14:59:24.567+01:00 [INF] Successfully opened bucket test_data_bucket
|
2019-04-30T14:59:24.574+01:00 [INF] Set query timeouts for bucket test_data_bucket to cluster:1m15s, bucket:1m15s
|
--- FAIL: TestIncrAmtZero (3.08s)
|
bucket_gocb_test.go:487:
|
Error Trace: bucket_gocb_test.go:487
|
Error: Not equal:
|
expected: 0x5
|
actual : 0x0
|
Test: TestIncrAmtZero
|
bucket_gocb_test.go:492:
|
Error Trace: bucket_gocb_test.go:492
|
Error: Not equal:
|
expected: 0x6
|
actual : 0x5
|
Test: TestIncrAmtZero
|
bucket_gocb_test.go:497:
|
Error Trace: bucket_gocb_test.go:497
|
Error: Not equal:
|
expected: 0x6
|
actual : 0x5
|
Test: TestIncrAmtZero
|
FAIL
|
FAIL github.com/couchbase/sync_gateway/base 3.099s
|
We don't have any non-test cases where we're using amt=0 and def != 0, but it would still be nice to clean this up to avoid any mistaken expectations around the API. Part of this comes down to the fact that we're overloading incr to be both GetCounter and IncrCounter. It might be sufficient to have our Incr return error when amt=0 and def != 0, unless we identify a real use case where we want to initialize a counter on a Get.