Copy over changes made to Google3 codebase in Icing.

Change-Id: Ic4e7a964ced111738cca80e078b3e03759c19ccf

3 files changed, 128 insertions, 50 deletions

diff --git a/icing/store/document-store.cc b/icing/store/document-store.cc
index f5b94d0..ce6f61c 100644
--- a/icing/store/document-store.cc
+++ b/icing/store/document-store.cc
@@ -817,11 +817,7 @@ libtextclassifier3::Status DocumentStore::Delete(
     const std::string_view name_space, const std::string_view uri) {
   // Try to get the DocumentId first
   auto document_id_or = GetDocumentId(name_space, uri);
-  if (absl_ports::IsNotFound(document_id_or.status())) {
-    // No need to delete nonexistent (name_space, uri)
-    return libtextclassifier3::Status::OK;
-  } else if (!document_id_or.ok()) {
-    // Real error
+  if (!document_id_or.ok()) {
     return absl_ports::Annotate(
         document_id_or.status(),
         absl_ports::StrCat("Failed to delete Document. namespace: ", name_space,
@@ -831,15 +827,11 @@ libtextclassifier3::Status DocumentStore::Delete(
   // Check if the DocumentId's Document still exists.
   DocumentId document_id = document_id_or.ValueOrDie();
   auto file_offset_or = DoesDocumentExistAndGetFileOffset(document_id);
-  if (absl_ports::IsNotFound(file_offset_or.status())) {
-    // No need to delete nonexistent documents
-    return libtextclassifier3::Status::OK;
-  } else if (!file_offset_or.ok()) {
-    // Real error, pass it up
+  if (!file_offset_or.ok()) {
     return absl_ports::Annotate(
         file_offset_or.status(),
-        IcingStringUtil::StringPrintf(
-            "Failed to retrieve file offset for DocumentId %d", document_id));
+        absl_ports::StrCat("Failed to delete Document. namespace: ", name_space,
+                           ", uri: ", uri));
   }
 
   // Update ground truth first.
@@ -852,10 +844,9 @@ libtextclassifier3::Status DocumentStore::Delete(
       document_log_->WriteProto(CreateDocumentTombstone(name_space, uri))
           .status();
   if (!status.ok()) {
-    ICING_LOG(ERROR) << status.error_message()
-                     << "Failed to delete Document. namespace: " << name_space
-                     << ", uri: " << uri;
-    return status;
+    return absl_ports::Annotate(
+        status, absl_ports::StrCat("Failed to delete Document. namespace: ",
+                                   name_space, ", uri: ", uri));
   }
 
   ICING_RETURN_IF_ERROR(
@@ -894,12 +885,11 @@ DocumentStore::GetDocumentFilterData(DocumentId document_id) const {
 libtextclassifier3::Status DocumentStore::DeleteByNamespace(
     std::string_view name_space) {
   auto namespace_id_or = namespace_mapper_->Get(name_space);
-  if (absl_ports::IsNotFound(namespace_id_or.status())) {
-    // Namespace doesn't exist. Don't need to delete anything.
-    return libtextclassifier3::Status::OK;
-  } else if (!namespace_id_or.ok()) {
-    // Real error, pass it up.
-    return namespace_id_or.status();
+  if (!namespace_id_or.ok()) {
+    return absl_ports::Annotate(
+        namespace_id_or.status(),
+        absl_ports::StrCat("Failed to delete by namespace. namespace: ",
+                           name_space));
   }
 
   // Update ground truth first.
@@ -916,23 +906,31 @@ libtextclassifier3::Status DocumentStore::DeleteByNamespace(
     return status;
   }
 
-  return UpdateDerivedFilesNamespaceDeleted(name_space);
+  ICING_ASSIGN_OR_RETURN(bool updated_existing_document,
+                         UpdateDerivedFilesNamespaceDeleted(name_space));
+  if (!updated_existing_document) {
+    // Treat the fact that no existing documents had this namespace to be the
+    // same as this namespace not existing at all.
+    return absl_ports::NotFoundError(
+        absl_ports::StrCat("Namespace '", name_space, "' doesn't exist"));
+  }
+  return libtextclassifier3::Status::OK;
 }
 
-libtextclassifier3::Status DocumentStore::UpdateDerivedFilesNamespaceDeleted(
-    std::string_view name_space) {
+libtextclassifier3::StatusOr<bool>
+DocumentStore::UpdateDerivedFilesNamespaceDeleted(std::string_view name_space) {
   auto namespace_id_or = namespace_mapper_->Get(name_space);
-  if (absl_ports::IsNotFound(namespace_id_or.status())) {
-    // Namespace doesn't exist. Don't need to delete anything.
-    return libtextclassifier3::Status::OK;
-  } else if (!namespace_id_or.ok()) {
-    // Real error, pass it up.
+  if (!namespace_id_or.ok()) {
     return namespace_id_or.status();
   }
 
   // Guaranteed to have a NamespaceId now.
   NamespaceId namespace_id = namespace_id_or.ValueOrDie();
 
+  // Tracks if there were any existing documents with this namespace that we
+  // will mark as deleted.
+  bool updated_existing_document = false;
+
   // Traverse FilterCache and delete all docs that match namespace_id
   for (DocumentId document_id = 0; document_id < filter_cache_->num_elements();
        ++document_id) {
@@ -941,6 +939,10 @@ libtextclassifier3::Status DocumentStore::UpdateDerivedFilesNamespaceDeleted(
     ICING_ASSIGN_OR_RETURN(const DocumentFilterData* data,
                            filter_cache_->Get(document_id));
     if (data->namespace_id() == namespace_id) {
+      if (DoesDocumentExist(document_id)) {
+        updated_existing_document = true;
+      }
+
       // docid_mapper_->Set can only fail if document_id is < 0
       // or >= docid_mapper_->num_elements. So the only possible way to get an
       // error here would be if filter_cache_->num_elements >
@@ -950,18 +952,17 @@ libtextclassifier3::Status DocumentStore::UpdateDerivedFilesNamespaceDeleted(
     }
   }
 
-  return libtextclassifier3::Status::OK;
+  return updated_existing_document;
 }
 
 libtextclassifier3::Status DocumentStore::DeleteBySchemaType(
     std::string_view schema_type) {
   auto schema_type_id_or = schema_store_->GetSchemaTypeId(schema_type);
-  if (absl_ports::IsNotFound(schema_type_id_or.status())) {
-    // SchemaType doesn't exist. Don't need to delete anything.
-    return libtextclassifier3::Status::OK;
-  } else if (!schema_type_id_or.ok()) {
-    // Real error, pass it up.
-    return schema_type_id_or.status();
+  if (!schema_type_id_or.ok()) {
+    return absl_ports::Annotate(
+        schema_type_id_or.status(),
+        absl_ports::StrCat("Failed to delete by schema type. schema_type: ",
+                           schema_type));
   }
 
   // Update ground truth first.
@@ -1076,7 +1077,11 @@ libtextclassifier3::Status DocumentStore::UpdateSchemaStore(
     } else {
       // Document is no longer valid with the new SchemaStore. Mark as
       // deleted
-      ICING_RETURN_IF_ERROR(Delete(document.namespace_(), document.uri()));
+      auto delete_status = Delete(document.namespace_(), document.uri());
+      if (!delete_status.ok() && !absl_ports::IsNotFound(delete_status)) {
+        // Real error, pass up
+        return delete_status;
+      }
     }
   }
 
@@ -1167,7 +1172,11 @@ libtextclassifier3::Status DocumentStore::OptimizedUpdateSchemaStore(
         if (!document_validator_.Validate(document).ok()) {
           // Document is no longer valid with the new SchemaStore. Mark as
           // deleted
-          ICING_RETURN_IF_ERROR(Delete(document.namespace_(), document.uri()));
+          auto delete_status = Delete(document.namespace_(), document.uri());
+          if (!delete_status.ok() && !absl_ports::IsNotFound(delete_status)) {
+            // Real error, pass up
+            return delete_status;
+          }
         }
       }
     }
diff --git a/icing/store/document-store.h b/icing/store/document-store.h
index 5bb167d..1fafd2a 100644
--- a/icing/store/document-store.h
+++ b/icing/store/document-store.h
@@ -94,6 +94,9 @@ class DocumentStore {
   //
   // Returns:
   //   A newly generated document id on success
+  //   FAILED_PRECONDITION if schema hasn't been set yet
+  //   NOT_FOUND if the schema_type or a property config of the document doesn't
+  //     exist in schema
   //   INTERNAL_ERROR on IO error
   libtextclassifier3::StatusOr<DocumentId> Put(const DocumentProto& document);
   libtextclassifier3::StatusOr<DocumentId> Put(DocumentProto&& document);
@@ -118,8 +121,11 @@ class DocumentStore {
   //   INTERNAL_ERROR on IO error
   libtextclassifier3::StatusOr<DocumentProto> Get(DocumentId document_id) const;
 
-  // Returns true if there's an existing document associated with the given
-  // document id.
+  // Check if a document exists. Existence means it hasn't been deleted and it
+  // hasn't expired yet.
+  //
+  // Returns:
+  //   boolean whether a document exists or not
   bool DoesDocumentExist(DocumentId document_id) const;
 
   // Deletes the document identified by the given namespace and uri
@@ -129,6 +135,7 @@ class DocumentStore {
   //
   // Returns:
   //   OK on success
+  //   NOT_FOUND if no document exists with namespace, uri
   //   INTERNAL_ERROR on IO error
   libtextclassifier3::Status Delete(std::string_view name_space,
                                     std::string_view uri);
@@ -178,6 +185,7 @@ class DocumentStore {
   //
   // Returns:
   //   OK on success
+  //   NOT_FOUND if namespace doesn't exist
   //   INTERNAL_ERROR on IO error
   libtextclassifier3::Status DeleteByNamespace(std::string_view name_space);
 
@@ -188,6 +196,7 @@ class DocumentStore {
   //
   // Returns:
   //   OK on success
+  //   NOT_FOUND if schema_type doesn't exist
   //   INTERNAL_ERROR on IO error
   libtextclassifier3::Status DeleteBySchemaType(std::string_view schema_type);
 
@@ -394,9 +403,9 @@ class DocumentStore {
   // called.
   //
   // Returns:
-  //   OK on success
+  //   bool on whether an existing document was actually updated to be deleted
   //   INTERNAL_ERROR on IO error
-  libtextclassifier3::Status UpdateDerivedFilesNamespaceDeleted(
+  libtextclassifier3::StatusOr<bool> UpdateDerivedFilesNamespaceDeleted(
       std::string_view name_space);
 
   // Update derived files that the schema type schema_type_id has been deleted.
diff --git a/icing/store/document-store_test.cc b/icing/store/document-store_test.cc
index e246ca9..f181f8c 100644
--- a/icing/store/document-store_test.cc
+++ b/icing/store/document-store_test.cc
@@ -340,7 +340,7 @@ TEST_F(DocumentStoreTest, GetInvalidDocumentId) {
               StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
 }
 
-TEST_F(DocumentStoreTest, Delete) {
+TEST_F(DocumentStoreTest, DeleteOk) {
   ICING_ASSERT_OK_AND_ASSIGN(
       std::unique_ptr<DocumentStore> doc_store,
       DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
@@ -352,18 +352,45 @@ TEST_F(DocumentStoreTest, Delete) {
   EXPECT_THAT(doc_store->Delete("icing", "email/1"), IsOk());
   EXPECT_THAT(doc_store->Get(document_id),
               StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
+}
+
+TEST_F(DocumentStoreTest, DeleteNonexistentDocumentNotFound) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<DocumentStore> document_store,
+      DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
+                            schema_store_.get()));
 
   // Validates that deleting something non-existing won't append anything to
   // ground truth
   int64_t ground_truth_size_before = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
-  // icing + email/1 has already been deleted.
-  EXPECT_THAT(doc_store->Delete("icing", "email/1"), IsOk());
+
+  EXPECT_THAT(
+      document_store->Delete("nonexistent_namespace", "nonexistent_uri"),
+      StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
+
   int64_t ground_truth_size_after = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
   EXPECT_THAT(ground_truth_size_before, Eq(ground_truth_size_after));
 }
 
+TEST_F(DocumentStoreTest, DeleteAlreadyDeletedDocumentNotFound) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<DocumentStore> document_store,
+      DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
+                            schema_store_.get()));
+  ICING_EXPECT_OK(document_store->Put(test_document1_));
+
+  // First time is OK
+  ICING_EXPECT_OK(document_store->Delete(test_document1_.namespace_(),
+                                         test_document1_.uri()));
+
+  // Deleting it again is NOT_FOUND
+  EXPECT_THAT(document_store->Delete(test_document1_.namespace_(),
+                                     test_document1_.uri()),
+              StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
+}
+
 TEST_F(DocumentStoreTest, DeleteByNamespaceOk) {
   ICING_ASSERT_OK_AND_ASSIGN(
       std::unique_ptr<DocumentStore> doc_store,
@@ -403,7 +430,7 @@ TEST_F(DocumentStoreTest, DeleteByNamespaceOk) {
               StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
 }
 
-TEST_F(DocumentStoreTest, DeleteByNamespaceNonexistentNamespaceOk) {
+TEST_F(DocumentStoreTest, DeleteByNamespaceNonexistentNamespaceNotFound) {
   ICING_ASSERT_OK_AND_ASSIGN(
       std::unique_ptr<DocumentStore> doc_store,
       DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
@@ -414,13 +441,30 @@ TEST_F(DocumentStoreTest, DeleteByNamespaceNonexistentNamespaceOk) {
   int64_t ground_truth_size_before = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
 
-  ICING_EXPECT_OK(doc_store->DeleteByNamespace("nonexistent_namespace"));
+  EXPECT_THAT(doc_store->DeleteByNamespace("nonexistent_namespace"),
+              StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
 
   int64_t ground_truth_size_after = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
   EXPECT_THAT(ground_truth_size_before, Eq(ground_truth_size_after));
 }
 
+TEST_F(DocumentStoreTest, DeleteByNamespaceNoExistingDocumentsNotFound) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<DocumentStore> document_store,
+      DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
+                            schema_store_.get()));
+  ICING_EXPECT_OK(document_store->Put(test_document1_));
+  ICING_EXPECT_OK(document_store->Delete(test_document1_.namespace_(),
+                                         test_document1_.uri()));
+
+  // At this point, there are no existing documents with the namespace, even
+  // though Icing's derived files know about this namespace. We should still
+  // return NOT_FOUND since nothing existing has this namespace.
+  EXPECT_THAT(document_store->DeleteByNamespace(test_document1_.namespace_()),
+              StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
+}
+
 TEST_F(DocumentStoreTest, DeleteByNamespaceRecoversOk) {
   DocumentProto document1 = test_document1_;
   document1.set_namespace_("namespace.1");
@@ -568,7 +612,7 @@ TEST_F(DocumentStoreTest, DeleteBySchemaTypeOk) {
               IsOkAndHolds(EqualsProto(person_document)));
 }
 
-TEST_F(DocumentStoreTest, DeleteBySchemaTypeNonexistentSchemaTypeOk) {
+TEST_F(DocumentStoreTest, DeleteBySchemaTypeNonexistentSchemaTypeNotFound) {
   ICING_ASSERT_OK_AND_ASSIGN(
       std::unique_ptr<DocumentStore> document_store,
       DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
@@ -579,7 +623,8 @@ TEST_F(DocumentStoreTest, DeleteBySchemaTypeNonexistentSchemaTypeOk) {
   int64_t ground_truth_size_before = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
 
-  ICING_EXPECT_OK(document_store->DeleteBySchemaType("nonexistent_type"));
+  EXPECT_THAT(document_store->DeleteBySchemaType("nonexistent_type"),
+              StatusIs(libtextclassifier3::StatusCode::NOT_FOUND));
 
   int64_t ground_truth_size_after = filesystem_.GetFileSize(
       absl_ports::StrCat(document_store_dir_, "/document_log").c_str());
@@ -587,6 +632,21 @@ TEST_F(DocumentStoreTest, DeleteBySchemaTypeNonexistentSchemaTypeOk) {
   EXPECT_THAT(ground_truth_size_before, Eq(ground_truth_size_after));
 }
 
+TEST_F(DocumentStoreTest, DeleteBySchemaTypeNoExistingDocumentsOk) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<DocumentStore> document_store,
+      DocumentStore::Create(&filesystem_, document_store_dir_, &fake_clock_,
+                            schema_store_.get()));
+  ICING_EXPECT_OK(document_store->Put(test_document1_));
+  ICING_EXPECT_OK(document_store->Delete(test_document1_.namespace_(),
+                                         test_document1_.uri()));
+
+  // At this point, there are no existing documents with the schema type, but we
+  // still return OK because the SchemaStore is the ground truth on schemas and
+  // knows about the type
+  ICING_EXPECT_OK(document_store->DeleteBySchemaType(test_document1_.schema()));
+}
+
 TEST_F(DocumentStoreTest, DeleteBySchemaTypeRecoversOk) {
   SchemaProto schema;
   auto type_config = schema.add_types();