Fix PR comments and issue with empty string slices

acme/db/nosql/eab.go

@@ -89,7 +89,7 @@ func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerID, refer
 			Reference:            dbeak.Reference,
 			ExternalAccountKeyID: dbeak.ID,
 		}
-		if err := db.save(ctx, referenceKey(provisionerID, dbeak.Reference), dbExternalAccountKeyReference, nil, "external_account_key_reference", externalAccountKeysByReferenceTable); err != nil {
+		if err := db.save(ctx, referenceKey(provisionerID, dbeak.Reference), dbExternalAccountKeyReference, nil, "external_account_key_reference", externalAccountKeyIDsByReferenceTable); err != nil {
 			return nil, err
 		}
 	}
@@ -144,7 +144,7 @@ func (db *DB) DeleteExternalAccountKey(ctx context.Context, provisionerID, keyID
 	}
 
 	if dbeak.Reference != "" {
-		if err := db.db.Del(externalAccountKeysByReferenceTable, []byte(referenceKey(provisionerID, dbeak.Reference))); err != nil {
+		if err := db.db.Del(externalAccountKeyIDsByReferenceTable, []byte(referenceKey(provisionerID, dbeak.Reference))); err != nil {
 			return errors.Wrapf(err, "error deleting ACME EAB Key reference with Key ID %s and reference %s", keyID, dbeak.Reference)
 		}
 	}
@@ -212,7 +212,7 @@ func (db *DB) GetExternalAccountKeyByReference(ctx context.Context, provisionerI
 		return nil, nil
 	}
 
-	k, err := db.db.Get(externalAccountKeysByReferenceTable, []byte(referenceKey(provisionerID, reference)))
+	k, err := db.db.Get(externalAccountKeyIDsByReferenceTable, []byte(referenceKey(provisionerID, reference)))
 	if nosqlDB.IsErrNotFound(err) {
 		return nil, acme.ErrNotFound
 	} else if err != nil {
@@ -291,11 +291,19 @@ func (db *DB) addEAKID(ctx context.Context, provisionerID, eakID string) error {
 	var newEAKIDs []string
 	newEAKIDs = append(newEAKIDs, eakIDs...)
 	newEAKIDs = append(newEAKIDs, eakID)
+
 	var (
 		_old interface{} = eakIDs
 		_new interface{} = newEAKIDs
 	)
 
+	// ensure that the DB gets the expected value when the slice is empty; otherwise
+	// it'll return with an error that indicates that the DBs view of the data is
+	// different from the last read (i.e. _old is different from what the DB has).
+	if len(eakIDs) == 0 {
+		_old = nil
+	}
+
 	if err = db.save(ctx, provisionerID, _new, _old, "externalAccountKeyIDsByProvisionerID", externalAccountKeyIDsByProvisionerIDTable); err != nil {
 		return errors.Wrapf(err, "error saving eakIDs index for provisioner %s", provisionerID)
 	}
@@ -326,6 +334,13 @@ func (db *DB) deleteEAKID(ctx context.Context, provisionerID, eakID string) erro
 		_new interface{} = newEAKIDs
 	)
 
+	// ensure that the DB gets the expected value when the slice is empty; otherwise
+	// it'll return with an error that indicates that the DBs view of the data is
+	// different from the last read (i.e. _old is different from what the DB has).
+	if len(eakIDs) == 0 {
+		_old = nil
+	}
+
 	if err = db.save(ctx, provisionerID, _new, _old, "externalAccountKeyIDsByProvisionerID", externalAccountKeyIDsByProvisionerIDTable); err != nil {
 		return errors.Wrapf(err, "error saving eakIDs index for provisioner %s", provisionerID)
 	}

acme/db/nosql/eab_test.go

@@ -271,7 +271,7 @@ func TestDB_GetExternalAccountKeyByReference(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -306,7 +306,7 @@ func TestDB_GetExternalAccountKeyByReference(t *testing.T) {
 				ref: ref,
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
-						assert.Equals(t, string(bucket), string(externalAccountKeysByReferenceTable))
+						assert.Equals(t, string(bucket), string(externalAccountKeyIDsByReferenceTable))
 						assert.Equals(t, string(key), provID+"."+ref)
 						return nil, nosqldb.ErrNotFound
 					},
@@ -319,7 +319,7 @@ func TestDB_GetExternalAccountKeyByReference(t *testing.T) {
 				ref: ref,
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
-						assert.Equals(t, string(bucket), string(externalAccountKeysByReferenceTable))
+						assert.Equals(t, string(bucket), string(externalAccountKeyIDsByReferenceTable))
 						assert.Equals(t, string(key), provID+"."+ref)
 						return nil, errors.New("force")
 					},
@@ -332,7 +332,7 @@ func TestDB_GetExternalAccountKeyByReference(t *testing.T) {
 				ref: ref,
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
-						assert.Equals(t, string(bucket), string(externalAccountKeysByReferenceTable))
+						assert.Equals(t, string(bucket), string(externalAccountKeyIDsByReferenceTable))
 						assert.Equals(t, string(key), provID+"."+ref)
 						return []byte{0}, nil
 					},
@@ -352,7 +352,7 @@ func TestDB_GetExternalAccountKeyByReference(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -642,7 +642,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -660,7 +660,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 					},
 					MDel: func(bucket, key []byte) error {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return nil
 						case string(externalAccountKeyTable):
@@ -674,7 +674,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 					MCmpAndSwap: func(bucket, key, old, new []byte) ([]byte, bool, error) {
 						fmt.Println(string(bucket))
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, provID+"."+ref, string(key))
 							return nil, true, nil
 						case string(externalAccountKeyIDsByProvisionerIDTable):
@@ -745,7 +745,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -758,7 +758,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 					},
 					MDel: func(bucket, key []byte) error {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return errors.New("force")
 						case string(externalAccountKeyTable):
@@ -795,7 +795,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -808,7 +808,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 					},
 					MDel: func(bucket, key []byte) error {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return nil
 						case string(externalAccountKeyTable):
@@ -845,7 +845,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MGet: func(bucket, key []byte) ([]byte, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), ref)
 							return dbrefBytes, nil
 						case string(externalAccountKeyTable):
@@ -860,7 +860,7 @@ func TestDB_DeleteExternalAccountKey(t *testing.T) {
 					},
 					MDel: func(bucket, key []byte) error {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), provID+"."+ref)
 							return nil
 						case string(externalAccountKeyTable):
@@ -939,7 +939,7 @@ func TestDB_CreateExternalAccountKey(t *testing.T) {
 						case string(externalAccountKeyIDsByProvisionerIDTable):
 							assert.Equals(t, provID, string(key))
 							return nu, true, nil
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, provID+"."+ref, string(key))
 							assert.Equals(t, nil, old)
 							return nu, true, nil
@@ -973,7 +973,7 @@ func TestDB_CreateExternalAccountKey(t *testing.T) {
 				db: &certdb.MockNoSQLDB{
 					MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), ref)
 							assert.Equals(t, old, nil)
 							return nu, true, nil
@@ -999,7 +999,7 @@ func TestDB_CreateExternalAccountKey(t *testing.T) {
 					},
 					MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) {
 						switch string(bucket) {
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, string(key), ref)
 							assert.Equals(t, old, nil)
 							return nu, true, nil
@@ -1029,7 +1029,7 @@ func TestDB_CreateExternalAccountKey(t *testing.T) {
 						case string(externalAccountKeyIDsByProvisionerIDTable):
 							assert.Equals(t, provID, string(key))
 							return nu, true, nil
-						case string(externalAccountKeysByReferenceTable):
+						case string(externalAccountKeyIDsByReferenceTable):
 							assert.Equals(t, provID+"."+ref, string(key))
 							assert.Equals(t, old, nil)
 							return nu, true, errors.New("force")
@@ -1348,7 +1348,7 @@ func TestDB_addEAKID(t *testing.T) {
 					MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) {
 						assert.Equals(t, bucket, externalAccountKeyIDsByProvisionerIDTable)
 						assert.Equals(t, string(key), provID)
-						assert.Equals(t, old, []byte{110, 117, 108, 108})
+						assert.Equals(t, old, nil)
 						b, _ := json.Marshal([]string{eakID})
 						assert.Equals(t, nu, b)
 						return b, true, nil
@@ -1482,7 +1482,7 @@ func TestDB_deleteEAKID(t *testing.T) {
 					MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) {
 						assert.Equals(t, bucket, externalAccountKeyIDsByProvisionerIDTable)
 						assert.Equals(t, string(key), provID)
-						assert.Equals(t, old, []byte{110, 117, 108, 108})
+						assert.Equals(t, old, nil)
 						b, _ := json.Marshal([]string{})
 						assert.Equals(t, nu, b)
 						return b, true, nil
@@ -1579,7 +1579,7 @@ func TestDB_addAndDeleteEAKID(t *testing.T) {
 						assert.Equals(t, string(key), provID)
 						switch callCounter {
 						case 0:
-							assert.Equals(t, old, []byte{110, 117, 108, 108})
+							assert.Equals(t, old, nil)
 							newB, _ := json.Marshal([]string{"eakID"})
 							assert.Equals(t, nu, newB)
 							return newB, true, nil
@@ -1589,8 +1589,7 @@ func TestDB_addAndDeleteEAKID(t *testing.T) {
 							newB, _ := json.Marshal([]string{})
 							return newB, true, nil
 						case 2:
-							oldB, _ := json.Marshal([]string{})
-							assert.Equals(t, old, oldB)
+							assert.Equals(t, old, nil)
 							newB, _ := json.Marshal([]string{"eakID1"})
 							assert.Equals(t, nu, newB)
 							return newB, true, nil

acme/db/nosql/nosql.go

@@ -21,7 +21,7 @@ var (
 	certTable                                 = []byte("acme_certs")
 	certBySerialTable                         = []byte("acme_serial_certs_index")
 	externalAccountKeyTable                   = []byte("acme_external_account_keys")
-	externalAccountKeysByReferenceTable       = []byte("acme_external_account_key_reference_index")
+	externalAccountKeyIDsByReferenceTable     = []byte("acme_external_account_keyID_reference_index")
 	externalAccountKeyIDsByProvisionerIDTable = []byte("acme_external_account_keyID_provisionerID_index")
 )
 
@@ -35,7 +35,7 @@ func New(db nosqlDB.DB) (*DB, error) {
 	tables := [][]byte{accountTable, accountByKeyIDTable, authzTable,
 		challengeTable, nonceTable, orderTable, ordersByAccountIDTable,
 		certTable, certBySerialTable, externalAccountKeyTable,
-		externalAccountKeysByReferenceTable, externalAccountKeyIDsByProvisionerIDTable,
+		externalAccountKeyIDsByReferenceTable, externalAccountKeyIDsByProvisionerIDTable,
 	}
 	for _, b := range tables {
 		if err := db.CreateTable(b); err != nil {