diff --git a/api/handlers/documents.go b/api/handlers/documents.go index 5a56e89..b91e1a0 100644 --- a/api/handlers/documents.go +++ b/api/handlers/documents.go @@ -67,7 +67,7 @@ func (h *Handlers) DeleteDocument(c *gin.Context) { collectionId := c.Param("collId") documentId := c.Param("docId") - status := h.dataStore.DeleteDocument(databaseId, collectionId, documentId) + status := h.dataStore.DeleteDocumentIfMatch(databaseId, collectionId, documentId, c.GetHeader(headers.IfMatch)) if status == datastore.StatusOk { c.Status(http.StatusNoContent) return @@ -78,6 +78,12 @@ func (h *Handlers) DeleteDocument(c *gin.Context) { return } + if status == datastore.PreconditionFailed { + c.Header(headers.ErrorCode, "PreconditionFailed") + c.JSON(http.StatusPreconditionFailed, constants.PreconditionFailedResponse) + return + } + c.IndentedJSON(http.StatusInternalServerError, constants.UnknownErrorResponse) } @@ -424,6 +430,8 @@ func dataStoreStatusToResponseCode(status datastore.DataStoreStatus) int { return http.StatusConflict case datastore.BadRequest: return http.StatusBadRequest + case datastore.PreconditionFailed: + return http.StatusPreconditionFailed default: return http.StatusInternalServerError } diff --git a/api/tests/documents_test.go b/api/tests/documents_test.go index 54d2f7a..13b2618 100644 --- a/api/tests/documents_test.go +++ b/api/tests/documents_test.go @@ -670,3 +670,128 @@ func Test_Documents(t *testing.T) { }) }) } + +func Test_Documents_Delete_IfMatch(t *testing.T) { + presets := []testPreset{PresetJsonStore, PresetBadgerStore} + + runTestsWithPresets(t, "Test_Documents_Delete_IfMatch", presets, func(t *testing.T, ts *TestServer, client *azcosmos.Client) { + collectionClient := documents_InitializeDb(t, ts) + + t.Run("Should fail delete with stale etag", func(t *testing.T) { + ctx := context.TODO() + + readResponse, err := collectionClient.ReadItem(ctx, azcosmos.PartitionKey{}, "12345", nil) + assert.Nil(t, err) + staleETag := readResponse.ETag + assert.NotEmpty(t, staleETag) + + item := map[string]interface{}{"id": "12345", "pk": "123", "isCool": true} + bytes, err := json.Marshal(item) + assert.Nil(t, err) + + _, err = collectionClient.ReplaceItem(ctx, azcosmos.PartitionKey{}, "12345", bytes, nil) + assert.Nil(t, err) + + updated, err := collectionClient.ReadItem(ctx, azcosmos.PartitionKey{}, "12345", nil) + assert.Nil(t, err) + assert.NotEmpty(t, updated.ETag) + assert.NotEqual(t, string(staleETag), string(updated.ETag)) + + _, err = collectionClient.DeleteItem( + ctx, + azcosmos.PartitionKey{}, + "12345", + &azcosmos.ItemOptions{IfMatchEtag: &staleETag}, + ) + assert.NotNil(t, err) + + var respErr *azcore.ResponseError + if errors.As(err, &respErr) { + assert.Equal(t, http.StatusPreconditionFailed, respErr.StatusCode) + assert.Equal(t, "PreconditionFailed", respErr.RawResponse.Header.Get("x-ms-error-code")) + + responseBody, readErr := io.ReadAll(respErr.RawResponse.Body) + assert.Nil(t, readErr) + assert.JSONEq(t, + `{"code":"PreconditionFailed","message":"Operation cannot be performed because one of the specified precondition is not met."}`, + string(responseBody), + ) + } else { + panic(err) + } + + document, status := ts.DataStore.GetDocument(testDatabaseName, testCollectionName, "12345") + assert.Equal(t, datastore.StatusOk, status) + assert.Equal(t, true, document["isCool"]) + assert.Equal(t, string(updated.ETag), document["_etag"]) + }) + + t.Run("Should delete with matching etag", func(t *testing.T) { + ctx := context.TODO() + + readResponse, err := collectionClient.ReadItem(ctx, azcosmos.PartitionKey{}, "67890", nil) + assert.Nil(t, err) + etag := readResponse.ETag + assert.NotEmpty(t, etag) + + response, err := collectionClient.DeleteItem( + ctx, + azcosmos.PartitionKey{}, + "67890", + &azcosmos.ItemOptions{IfMatchEtag: &etag}, + ) + assert.Nil(t, err) + assert.Equal(t, http.StatusNoContent, response.RawResponse.StatusCode) + + _, status := ts.DataStore.GetDocument(testDatabaseName, testCollectionName, "67890") + assert.Equal(t, datastore.StatusNotFound, status) + }) + + t.Run("Should delete unconditionally", func(t *testing.T) { + ctx := context.TODO() + _, status := ts.DataStore.CreateDocument(testDatabaseName, testCollectionName, map[string]interface{}{ + "id": "unconditional-delete", "pk": "pk", "isCool": false, + }) + assert.Equal(t, datastore.StatusOk, status) + + response, err := collectionClient.DeleteItem(ctx, azcosmos.PartitionKey{}, "unconditional-delete", nil) + assert.Nil(t, err) + assert.Equal(t, http.StatusNoContent, response.RawResponse.StatusCode) + + _, status = ts.DataStore.GetDocument(testDatabaseName, testCollectionName, "unconditional-delete") + assert.Equal(t, datastore.StatusNotFound, status) + }) + + t.Run("Should return not found when deleting a missing document", func(t *testing.T) { + _, err := collectionClient.DeleteItem(context.TODO(), azcosmos.PartitionKey{}, "missing-document", nil) + assertDocumentDeleteNotFound(t, err) + }) + + t.Run("Should return not found when deleting a missing document with if-match", func(t *testing.T) { + etag := azcore.ETag("\"stale-etag\"") + _, err := collectionClient.DeleteItem( + context.TODO(), + azcosmos.PartitionKey{}, + "missing-document-if-match", + &azcosmos.ItemOptions{IfMatchEtag: &etag}, + ) + assertDocumentDeleteNotFound(t, err) + }) + }) +} + +func assertDocumentDeleteNotFound(t *testing.T, err error) { + t.Helper() + assert.NotNil(t, err) + + var respErr *azcore.ResponseError + if errors.As(err, &respErr) { + assert.Equal(t, http.StatusNotFound, respErr.StatusCode) + responseBody, readErr := io.ReadAll(respErr.RawResponse.Body) + assert.Nil(t, readErr) + assert.JSONEq(t, `{"message":"NotFound"}`, string(responseBody)) + return + } + + panic(err) +} diff --git a/internal/datastore/badger_datastore/documents.go b/internal/datastore/badger_datastore/documents.go index 89f42ac..03d6650 100644 --- a/internal/datastore/badger_datastore/documents.go +++ b/internal/datastore/badger_datastore/documents.go @@ -64,20 +64,36 @@ func (r *BadgerDataStore) GetDocument(databaseId string, collectionId string, do } func (r *BadgerDataStore) DeleteDocument(databaseId string, collectionId string, documentId string) datastore.DataStoreStatus { + return r.deleteDocument(databaseId, collectionId, documentId, "") +} + +func (r *BadgerDataStore) DeleteDocumentIfMatch(databaseId string, collectionId string, documentId string, ifMatch string) datastore.DataStoreStatus { + return r.deleteDocument(databaseId, collectionId, documentId, ifMatch) +} + +func (r *BadgerDataStore) deleteDocument(databaseId string, collectionId string, documentId string, ifMatch string) datastore.DataStoreStatus { documentKey := generateDocumentKey(databaseId, collectionId, documentId) txn := r.db.NewTransaction(true) defer txn.Discard() - exists, err := keyExists(txn, documentKey) - if err != nil { - return datastore.Unknown - } - if !exists { + var document datastore.Document + status := getKey(txn, documentKey, &document) + if status == datastore.StatusNotFound { return datastore.StatusNotFound } + if status != datastore.StatusOk { + return status + } - err = txn.Delete([]byte(documentKey)) + if ifMatch != "" { + etag, _ := document["_etag"].(string) + if etag != ifMatch { + return datastore.PreconditionFailed + } + } + + err := txn.Delete([]byte(documentKey)) if err != nil { logger.ErrorLn("Error while deleting document:", err) return datastore.Unknown diff --git a/internal/datastore/datastore.go b/internal/datastore/datastore.go index cbd3594..9d0cca0 100644 --- a/internal/datastore/datastore.go +++ b/internal/datastore/datastore.go @@ -15,6 +15,11 @@ type DataStore interface { GetDocumentIterator(databaseId string, collectionId string) (DocumentIterator, DataStoreStatus) GetDocument(databaseId string, collectionId string, documentId string) (Document, DataStoreStatus) DeleteDocument(databaseId string, collectionId string, documentId string) DataStoreStatus + // DeleteDocumentIfMatch removes a document when ifMatch is empty or equals its _etag. + // The comparison and delete happen together, so a concurrent update cannot land between them. + // A mismatched etag returns PreconditionFailed and leaves the document in place. + // A missing document returns StatusNotFound. + DeleteDocumentIfMatch(databaseId string, collectionId string, documentId string, ifMatch string) DataStoreStatus CreateDocument(databaseId string, collectionId string, document map[string]interface{}) (Document, DataStoreStatus) GetAllTriggers(databaseId string, collectionId string) ([]Trigger, DataStoreStatus) diff --git a/internal/datastore/json_datastore/documents.go b/internal/datastore/json_datastore/documents.go index d9884a2..6a636fb 100644 --- a/internal/datastore/json_datastore/documents.go +++ b/internal/datastore/json_datastore/documents.go @@ -45,6 +45,14 @@ func (r *JsonDataStore) GetDocument(databaseId string, collectionId string, docu } func (r *JsonDataStore) DeleteDocument(databaseId string, collectionId string, documentId string) datastore.DataStoreStatus { + return r.deleteDocument(databaseId, collectionId, documentId, "") +} + +func (r *JsonDataStore) DeleteDocumentIfMatch(databaseId string, collectionId string, documentId string, ifMatch string) datastore.DataStoreStatus { + return r.deleteDocument(databaseId, collectionId, documentId, ifMatch) +} + +func (r *JsonDataStore) deleteDocument(databaseId string, collectionId string, documentId string, ifMatch string) datastore.DataStoreStatus { r.storeState.Lock() defer r.storeState.Unlock() @@ -56,10 +64,18 @@ func (r *JsonDataStore) DeleteDocument(databaseId string, collectionId string, d return datastore.StatusNotFound } - if _, ok := r.storeState.Documents[databaseId][collectionId][documentId]; !ok { + document, ok := r.storeState.Documents[databaseId][collectionId][documentId] + if !ok { return datastore.StatusNotFound } + if ifMatch != "" { + etag, _ := document["_etag"].(string) + if etag != ifMatch { + return datastore.PreconditionFailed + } + } + delete(r.storeState.Documents[databaseId][collectionId], documentId) return datastore.StatusOk diff --git a/internal/datastore/models.go b/internal/datastore/models.go index 07e1fa8..174b462 100644 --- a/internal/datastore/models.go +++ b/internal/datastore/models.go @@ -11,12 +11,13 @@ type Database struct { type DataStoreStatus int const ( - StatusOk DataStoreStatus = 1 - StatusNotFound DataStoreStatus = 2 - Conflict DataStoreStatus = 3 - BadRequest DataStoreStatus = 4 - IterEOF DataStoreStatus = 5 - Unknown DataStoreStatus = 6 + StatusOk DataStoreStatus = 1 + StatusNotFound DataStoreStatus = 2 + Conflict DataStoreStatus = 3 + BadRequest DataStoreStatus = 4 + IterEOF DataStoreStatus = 5 + Unknown DataStoreStatus = 6 + PreconditionFailed DataStoreStatus = 7 ) type TriggerOperation string