Fix bug overriding nested incompatible types. am: 5fa917ce71

Original change: https://googleplex-android-review.googlesource.com/c/platform/external/icing/+/18328263

Change-Id: Id7f5471b03eb15647229361b90bb1d4f71c7fd15
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 211 insertions, 29 deletions

diff --git a/icing/schema/schema-store_test.cc b/icing/schema/schema-store_test.cc
index 113084e..541918f 100644
--- a/icing/schema/schema-store_test.cc
+++ b/icing/schema/schema-store_test.cc
@@ -57,6 +57,7 @@ constexpr StringIndexingConfig::TokenizerType::Code TOKENIZER_PLAIN =
     StringIndexingConfig::TokenizerType::PLAIN;
 
 constexpr TermMatchType::Code MATCH_EXACT = TermMatchType::EXACT_ONLY;
+constexpr TermMatchType::Code MATCH_PREFIX = TermMatchType::PREFIX;
 
 constexpr PropertyConfigProto::DataType::Code TYPE_STRING =
     PropertyConfigProto::DataType::STRING;
@@ -678,6 +679,181 @@ TEST_F(SchemaStoreTest, SetSchemaWithIncompatibleTypesOk) {
   EXPECT_THAT(*actual_schema, EqualsProto(schema));
 }
 
+TEST_F(SchemaStoreTest, SetSchemaWithIncompatibleNestedTypesOk) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<SchemaStore> schema_store,
+      SchemaStore::Create(&filesystem_, test_dir_, &fake_clock_));
+
+  // 1. Create a ContactPoint type with a repeated property and set that schema
+  SchemaTypeConfigBuilder contact_point_repeated_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_PREFIX, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_REPEATED));
+  SchemaProto old_schema =
+      SchemaBuilder().AddType(contact_point_repeated_label).Build();
+  ICING_EXPECT_OK(schema_store->SetSchema(old_schema));
+  ICING_ASSERT_OK_AND_ASSIGN(SchemaTypeId old_contact_point_type_id,
+                             schema_store->GetSchemaTypeId("ContactPoint"));
+
+  // 2. Create a type that references the ContactPoint type and make a backwards
+  // incompatible change to ContactPoint
+  SchemaTypeConfigBuilder contact_point_optional_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_PREFIX, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_OPTIONAL));
+  SchemaTypeConfigBuilder person =
+      SchemaTypeConfigBuilder().SetType("Person").AddProperty(
+          PropertyConfigBuilder()
+              .SetName("contactPoints")
+              .SetDataTypeDocument("ContactPoint",
+                                   /*index_nested_properties=*/true)
+              .SetCardinality(CARDINALITY_REPEATED));
+  SchemaProto new_schema = SchemaBuilder()
+                               .AddType(contact_point_optional_label)
+                               .AddType(person)
+                               .Build();
+
+  // 3. SetSchema should fail with ignore_errors_and_delete_documents=false and
+  // the old schema should remain
+  SchemaStore::SetSchemaResult expected_result;
+  expected_result.success = false;
+  expected_result.schema_types_incompatible_by_name.insert("ContactPoint");
+  expected_result.schema_types_incompatible_by_id.insert(
+      old_contact_point_type_id);
+  expected_result.schema_types_new_by_name.insert("Person");
+  EXPECT_THAT(
+      schema_store->SetSchema(new_schema,
+                              /*ignore_errors_and_delete_documents=*/false),
+      IsOkAndHolds(EqualsSetSchemaResult(expected_result)));
+  ICING_ASSERT_OK_AND_ASSIGN(const SchemaProto* actual_schema,
+                             schema_store->GetSchema());
+  EXPECT_THAT(*actual_schema, EqualsProto(old_schema));
+
+  // 4. SetSchema should succeed with ignore_errors_and_delete_documents=true
+  // and the new schema should be set
+  expected_result.success = true;
+  EXPECT_THAT(
+      schema_store->SetSchema(new_schema,
+                              /*ignore_errors_and_delete_documents=*/true),
+      IsOkAndHolds(EqualsSetSchemaResult(expected_result)));
+  ICING_ASSERT_OK_AND_ASSIGN(actual_schema, schema_store->GetSchema());
+  EXPECT_THAT(*actual_schema, EqualsProto(new_schema));
+}
+
+TEST_F(SchemaStoreTest, SetSchemaWithIndexIncompatibleNestedTypesOk) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<SchemaStore> schema_store,
+      SchemaStore::Create(&filesystem_, test_dir_, &fake_clock_));
+
+  // 1. Create a ContactPoint type with label that matches prefix and set that
+  // schema
+  SchemaTypeConfigBuilder contact_point_prefix_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_PREFIX, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_REPEATED));
+  SchemaProto old_schema =
+      SchemaBuilder().AddType(contact_point_prefix_label).Build();
+  ICING_EXPECT_OK(schema_store->SetSchema(old_schema));
+
+  // 2. Create a type that references the ContactPoint type and make a index
+  // backwards incompatible change to ContactPoint
+  SchemaTypeConfigBuilder contact_point_exact_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_EXACT, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_REPEATED));
+  SchemaTypeConfigBuilder person =
+      SchemaTypeConfigBuilder().SetType("Person").AddProperty(
+          PropertyConfigBuilder()
+              .SetName("contactPoints")
+              .SetDataTypeDocument("ContactPoint",
+                                   /*index_nested_properties=*/true)
+              .SetCardinality(CARDINALITY_REPEATED));
+  SchemaProto new_schema = SchemaBuilder()
+                               .AddType(contact_point_exact_label)
+                               .AddType(person)
+                               .Build();
+
+  // SetSchema should succeed, and only ContactPoint should be in
+  // schema_types_index_incompatible_by_name.
+  SchemaStore::SetSchemaResult expected_result;
+  expected_result.success = true;
+  expected_result.schema_types_index_incompatible_by_name.insert(
+      "ContactPoint");
+  expected_result.schema_types_new_by_name.insert("Person");
+  EXPECT_THAT(
+      schema_store->SetSchema(new_schema,
+                              /*ignore_errors_and_delete_documents=*/false),
+      IsOkAndHolds(EqualsSetSchemaResult(expected_result)));
+  ICING_ASSERT_OK_AND_ASSIGN(const SchemaProto* actual_schema,
+                             schema_store->GetSchema());
+  EXPECT_THAT(*actual_schema, EqualsProto(new_schema));
+}
+
+TEST_F(SchemaStoreTest, SetSchemaWithCompatibleNestedTypesOk) {
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<SchemaStore> schema_store,
+      SchemaStore::Create(&filesystem_, test_dir_, &fake_clock_));
+
+  // 1. Create a ContactPoint type with a optional property and set that schema
+  SchemaTypeConfigBuilder contact_point_optional_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_PREFIX, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_OPTIONAL));
+  SchemaProto old_schema =
+      SchemaBuilder().AddType(contact_point_optional_label).Build();
+  ICING_EXPECT_OK(schema_store->SetSchema(old_schema));
+
+  // 2. Create a type that references the ContactPoint type and make a backwards
+  // compatible change to ContactPoint
+  SchemaTypeConfigBuilder contact_point_repeated_label =
+      SchemaTypeConfigBuilder()
+          .SetType("ContactPoint")
+          .AddProperty(PropertyConfigBuilder()
+                           .SetName("label")
+                           .SetDataTypeString(MATCH_PREFIX, TOKENIZER_PLAIN)
+                           .SetCardinality(CARDINALITY_REPEATED));
+  SchemaTypeConfigBuilder person =
+      SchemaTypeConfigBuilder().SetType("Person").AddProperty(
+          PropertyConfigBuilder()
+              .SetName("contactPoints")
+              .SetDataTypeDocument("ContactPoint",
+                                   /*index_nested_properties=*/true)
+              .SetCardinality(CARDINALITY_REPEATED));
+  SchemaProto new_schema = SchemaBuilder()
+                               .AddType(contact_point_repeated_label)
+                               .AddType(person)
+                               .Build();
+
+  // 3. SetSchema should succeed, and only ContactPoint should be in
+  // schema_types_changed_fully_compatible_by_name.
+  SchemaStore::SetSchemaResult expected_result;
+  expected_result.success = true;
+  expected_result.schema_types_changed_fully_compatible_by_name.insert(
+      "ContactPoint");
+  expected_result.schema_types_new_by_name.insert("Person");
+  EXPECT_THAT(schema_store->SetSchema(
+                  new_schema, /*ignore_errors_and_delete_documents=*/false),
+              IsOkAndHolds(EqualsSetSchemaResult(expected_result)));
+  ICING_ASSERT_OK_AND_ASSIGN(const SchemaProto* actual_schema,
+                             schema_store->GetSchema());
+  EXPECT_THAT(*actual_schema, EqualsProto(new_schema));
+}
+
 TEST_F(SchemaStoreTest, GetSchemaTypeId) {
   ICING_ASSERT_OK_AND_ASSIGN(
       std::unique_ptr<SchemaStore> schema_store,
diff --git a/icing/schema/schema-util.cc b/icing/schema/schema-util.cc
index 22bc3f6..88b6946 100644
--- a/icing/schema/schema-util.cc
+++ b/icing/schema/schema-util.cc
@@ -107,6 +107,33 @@ bool IsTermMatchTypeCompatible(const StringIndexingConfig& old_indexed,
          old_indexed.tokenizer_type() == new_indexed.tokenizer_type();
 }
 
+void AddIncompatibleChangeToDelta(
+    std::unordered_set<std::string>& incompatible_delta,
+    const SchemaTypeConfigProto& old_type_config,
+    const SchemaUtil::DependencyMap& new_schema_dependency_map,
+    const SchemaUtil::TypeConfigMap& old_type_config_map,
+    const SchemaUtil::TypeConfigMap& new_type_config_map) {
+  // If this type is incompatible, then every type that depends on it might
+  // also be incompatible. Use the dependency map to mark those ones as
+  // incompatible too.
+  incompatible_delta.insert(old_type_config.schema_type());
+  auto parent_types_itr =
+      new_schema_dependency_map.find(old_type_config.schema_type());
+  if (parent_types_itr != new_schema_dependency_map.end()) {
+    for (std::string_view parent_type : parent_types_itr->second) {
+      // The types from new_schema that depend on the current
+      // old_type_config may not present in old_schema.
+      // Those types will be listed at schema_delta.schema_types_new
+      // instead.
+      std::string parent_type_str(parent_type);
+      if (old_type_config_map.find(parent_type_str) !=
+          old_type_config_map.end()) {
+        incompatible_delta.insert(std::move(parent_type_str));
+      }
+    }
+  }
+}
+
 }  // namespace
 
 libtextclassifier3::Status ExpandTranstiveDependencies(
@@ -447,7 +474,8 @@ const SchemaUtil::SchemaDelta SchemaUtil::ComputeCompatibilityDelta(
     const DependencyMap& new_schema_dependency_map) {
   SchemaDelta schema_delta;
 
-  TypeConfigMap new_type_config_map;
+  TypeConfigMap old_type_config_map, new_type_config_map;
+  BuildTypeConfigMap(old_schema, &old_type_config_map);
   BuildTypeConfigMap(new_schema, &new_type_config_map);
 
   // Iterate through and check each field of the old schema
@@ -566,37 +594,15 @@ const SchemaUtil::SchemaDelta SchemaUtil::ComputeCompatibilityDelta(
     }
 
     if (is_incompatible) {
-      // If this type is incompatible, then every type that depends on it might
-      // also be incompatible. Use the dependency map to mark those ones as
-      // incompatible too.
-      schema_delta.schema_types_incompatible.insert(
-          old_type_config.schema_type());
-      auto parent_types_itr =
-          new_schema_dependency_map.find(old_type_config.schema_type());
-      if (parent_types_itr != new_schema_dependency_map.end()) {
-        schema_delta.schema_types_incompatible.reserve(
-            schema_delta.schema_types_incompatible.size() +
-            parent_types_itr->second.size());
-        schema_delta.schema_types_incompatible.insert(
-            parent_types_itr->second.begin(), parent_types_itr->second.end());
-      }
+      AddIncompatibleChangeToDelta(schema_delta.schema_types_incompatible,
+                                   old_type_config, new_schema_dependency_map,
+                                   old_type_config_map, new_type_config_map);
     }
 
     if (is_index_incompatible) {
-      // If this type is index incompatible, then every type that depends on it
-      // might also be index incompatible. Use the dependency map to mark those
-      // ones as index incompatible too.
-      schema_delta.schema_types_index_incompatible.insert(
-          old_type_config.schema_type());
-      auto parent_types_itr =
-          new_schema_dependency_map.find(old_type_config.schema_type());
-      if (parent_types_itr != new_schema_dependency_map.end()) {
-        schema_delta.schema_types_index_incompatible.reserve(
-            schema_delta.schema_types_index_incompatible.size() +
-            parent_types_itr->second.size());
-        schema_delta.schema_types_index_incompatible.insert(
-            parent_types_itr->second.begin(), parent_types_itr->second.end());
-      }
+      AddIncompatibleChangeToDelta(schema_delta.schema_types_index_incompatible,
+                                   old_type_config, new_schema_dependency_map,
+                                   old_type_config_map, new_type_config_map);
     }
 
     if (!is_incompatible && !is_index_incompatible && has_property_changed) {