diff options
author | Tim Barron <tjbarron@google.com> | 2022-05-12 17:55:53 +0000 |
---|---|---|
committer | Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com> | 2022-05-12 17:55:53 +0000 |
commit | d02bccf6fd9472f816fe7c1b71d9f188eef84987 (patch) | |
tree | 4b51fd643cd092948d6179db9280ec741ebe1619 | |
parent | c4647e82f4a65ae9de994b7db2e1ca8052b88b62 (diff) | |
parent | 5fa917ce715d831c2bb1a504e8b3d63d01354bf2 (diff) | |
download | icing-d02bccf6fd9472f816fe7c1b71d9f188eef84987.tar.gz |
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>
-rw-r--r-- | icing/schema/schema-store_test.cc | 176 | ||||
-rw-r--r-- | icing/schema/schema-util.cc | 64 |
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) { |