Uploaded image for project: 'Couchbase Gateway'
  1. Couchbase Gateway
  2. CBG-357

GoCB Bucket Incr amt=0 logic not taking 'def' into account

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.6.0
    • Component/s: SyncGateway
    • Security Level: Public
    • Labels:
      None
    • Sprint:
      CBG Sprint 22
    • Story Points:
      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
      

        Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

          Hide
          adamf Adam Fraser added a comment -

          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.

          Show
          adamf Adam Fraser added a comment - 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.

            People

            • Assignee:
              ben.brooks Ben Brooks
              Reporter:
              ben.brooks Ben Brooks
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Gerrit Reviews

                There are no open Gerrit changes

                  PagerDuty

                  Error rendering 'com.pagerduty.jira-server-plugin:PagerDuty'. Please contact your Jira administrators.