Commit 834e175b authored by Sam Bowen's avatar Sam Bowen Committed by Commit Bot

Check that entity properties match thing type in the validator.

* Generate new code in entity_names to check whether an entity type is
  descended from another (including itself).
* Use generated code in validator to validate nested entity types. They
  should match or be descended from one of the thing_types in the
  property config.

Bug: 1065513
Change-Id: I0e2cfecf29e7b85a3f5f89c98b00ff708fb4ee19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134932Reviewed-by: default avatarBecca Hughes <beccahughes@chromium.org>
Commit-Queue: Sam Bowen <sgbowen@google.com>
Cr-Commit-Position: refs/heads/master@{#756306}
parent a784aab8
...@@ -133,9 +133,47 @@ def merge_with_schema(schema, overrides, thing): ...@@ -133,9 +133,47 @@ def merge_with_schema(schema, overrides, thing):
schema['@graph'].append(thing) schema['@graph'].append(thing)
def lookup_parents(thing, schema, lookup_table):
"""Recursively looks up all the parents of thing in the schema.
Returns the parents and populates them in lookup_table. The parents list may
contain duplicates if thing has multiple inheritance trees.
"""
obj_name = object_name_from_id(thing['@id'])
if obj_name in lookup_table:
return lookup_table[obj_name]
lookup_table[obj_name] = []
if 'rdfs:subClassOf' in thing:
parent_classes = thing['rdfs:subClassOf']
if not isinstance(parent_classes, list):
parent_classes = [parent_classes]
parent_classes = [
get_schema_obj(parent['@id'], schema) for parent in parent_classes
]
parent_classes = [
parent for parent in parent_classes if parent is not None
]
found_parents = [
lookup_parents(parent_thing, schema, lookup_table)
for parent_thing in parent_classes
]
# flatten the list
found_parents = [item for sublist in found_parents for item in sublist]
lookup_table[obj_name].extend(found_parents)
lookup_table[obj_name].append(obj_name)
return lookup_table[obj_name]
def get_template_vars(schema_file_path, overrides_file_path): def get_template_vars(schema_file_path, overrides_file_path):
"""Read the needed template variables from the schema file.""" """Read the needed template variables from the schema file."""
template_vars = {'entities': [], 'properties': [], 'enums': []} template_vars = {
'entities': [],
'properties': [],
'enums': [],
'entity_parent_lookup': {}
}
with open(schema_file_path) as schema_file: with open(schema_file_path) as schema_file:
schema = json.loads(schema_file.read()) schema = json.loads(schema_file.read())
...@@ -149,6 +187,8 @@ def get_template_vars(schema_file_path, overrides_file_path): ...@@ -149,6 +187,8 @@ def get_template_vars(schema_file_path, overrides_file_path):
for thing in schema['@graph']: for thing in schema['@graph']:
if thing['@type'] == 'rdfs:Class': if thing['@type'] == 'rdfs:Class':
template_vars['entities'].append(object_name_from_id(thing['@id'])) template_vars['entities'].append(object_name_from_id(thing['@id']))
lookup_parents(thing, schema,
template_vars['entity_parent_lookup'])
if is_enum_type(thing): if is_enum_type(thing):
template_vars['enums'].append({ template_vars['enums'].append({
'name': 'name':
......
...@@ -92,4 +92,17 @@ TEST(GenerateSchemaOrgCodeTest, GetPropertyConfigurationUsesOverridesFile) { ...@@ -92,4 +92,17 @@ TEST(GenerateSchemaOrgCodeTest, GetPropertyConfigurationUsesOverridesFile) {
testing::UnorderedElementsAre("http://schema.org/PropertyValue")); testing::UnorderedElementsAre("http://schema.org/PropertyValue"));
} }
TEST(GenerateSchemaOrgCodeTest, IsDescendedFromSucceeds) {
EXPECT_TRUE(entity::IsDescendedFrom(entity::kThing, entity::kVideoObject));
}
TEST(GenerateSchemaOrgCodeTest, IsDescendedFromSucceedsMultipleInheritance) {
EXPECT_TRUE(
entity::IsDescendedFrom(entity::kPlace, entity::kCafeOrCoffeeShop));
}
TEST(GenerateSchemaOrgCodeTest, IsDescendedFromFails) {
EXPECT_FALSE(entity::IsDescendedFrom(entity::kVideoObject, entity::kThing));
}
} // namespace schema_org } // namespace schema_org
...@@ -48,8 +48,27 @@ class GenerateSchemaOrgCodeTest(unittest.TestCase): ...@@ -48,8 +48,27 @@ class GenerateSchemaOrgCodeTest(unittest.TestCase):
'enum_types': [] 'enum_types': []
}], }],
'enums': [], 'enums': [],
'entity_parent_lookup': {
'MediaObject': ['MediaObject']
}
}) })
def test_lookup_parents(self):
thing = {'@id': schema_org_id('Thing')}
intangible = {
'@id': schema_org_id('Intangible'),
'rdfs:subClassOf': thing
}
structured_value = {
'@id': schema_org_id('StructuredValue'),
'rdfs:subClassOf': intangible
}
brand = {'@id': schema_org_id('Brand'), 'rdfs:subClassOf': intangible}
schema = {'@graph': [thing, intangible, structured_value, brand]}
self.assertListEqual(
generate_schema_org_code.lookup_parents(brand, schema, {}),
['Thing', 'Intangible', 'Brand'])
def test_get_root_type_thing(self): def test_get_root_type_thing(self):
thing = {'@id': schema_org_id('Thing')} thing = {'@id': schema_org_id('Thing')}
intangible = { intangible = {
......
...@@ -27,5 +27,26 @@ bool IsValidEntityName(const std::string& entity_name) { ...@@ -27,5 +27,26 @@ bool IsValidEntityName(const std::string& entity_name) {
return kValidEntityNames->find(entity_name) != kValidEntityNames->end(); return kValidEntityNames->find(entity_name) != kValidEntityNames->end();
} }
bool IsDescendedFrom(const std::string& possible_parent,
const std::string& possible_child) {
static const base::NoDestructor<std::map<std::string, std::vector<std::string>>>
kParentEntities(std::map<std::string, std::vector<std::string>>({
{%for key in entity_parent_lookup %}
{ "{{key}}", {
{% for parent in entity_parent_lookup[key] %}
"{{parent}}",
{% endfor %}
}
},
{% endfor %}
}));
auto parents = kParentEntities->find(possible_child);
if (parents == kParentEntities->end())
return false;
auto it = std::find_if(parents->second.begin(), parents->second.end(), [&possible_parent](const std::string& parent) { return parent == possible_parent; });
return it != parents->second.end();
}
} // entity } // entity
} // schema_org } // schema_org
...@@ -19,6 +19,11 @@ extern const char k{{entity[0]|upper}}{{entity[1:]}}[]; ...@@ -19,6 +19,11 @@ extern const char k{{entity[0]|upper}}{{entity[1:]}}[];
bool IsValidEntityName(const std::string& entity_name); bool IsValidEntityName(const std::string& entity_name);
// Returns true if the entity with name possible_child inherits from
// possible_parent.
bool IsDescendedFrom(const std::string& possible_parent,
const std::string& possible_child);
} // namespace entity } // namespace entity
} // namespace schema_org } // namespace schema_org
......
...@@ -17,6 +17,24 @@ namespace schema_org { ...@@ -17,6 +17,24 @@ namespace schema_org {
using improved::mojom::Entity; using improved::mojom::Entity;
using improved::mojom::EntityPtr; using improved::mojom::EntityPtr;
base::Optional<std::string> ObjectNameFromId(const std::string& id) {
GURL id_url = GURL(id);
if (!id_url.SchemeIsHTTPOrHTTPS() || id_url.host() != "schema.org")
return base::nullopt;
return id_url.path().substr(1);
}
bool EntityPropertyIsValidType(const property::PropertyConfiguration& config,
const std::string& type) {
for (const auto& thing_type_id : config.thing_types) {
auto thing_type_name = ObjectNameFromId(thing_type_id);
DCHECK(thing_type_name.has_value());
if (entity::IsDescendedFrom(thing_type_name.value(), type))
return true;
}
return false;
}
// static // static
bool ValidateEntity(Entity* entity) { bool ValidateEntity(Entity* entity) {
if (!entity::IsValidEntityName(entity->type)) { if (!entity::IsValidEntityName(entity->type)) {
...@@ -49,7 +67,8 @@ bool ValidateEntity(Entity* entity) { ...@@ -49,7 +67,8 @@ bool ValidateEntity(Entity* entity) {
auto nested_it = (*it)->values->entity_values.begin(); auto nested_it = (*it)->values->entity_values.begin();
while (nested_it != (*it)->values->entity_values.end()) { while (nested_it != (*it)->values->entity_values.end()) {
auto& nested_entity = *nested_it; auto& nested_entity = *nested_it;
if (!ValidateEntity(nested_entity.get())) { if (!ValidateEntity(nested_entity.get()) ||
!EntityPropertyIsValidType(config, nested_entity->type)) {
nested_it = (*it)->values->entity_values.erase(nested_it); nested_it = (*it)->values->entity_values.erase(nested_it);
} else { } else {
has_valid_entities = true; has_valid_entities = true;
......
...@@ -204,6 +204,27 @@ TEST_F(SchemaOrgValidatorTest, InvalidEntityPropertyValue) { ...@@ -204,6 +204,27 @@ TEST_F(SchemaOrgValidatorTest, InvalidEntityPropertyValue) {
EXPECT_TRUE(entity->properties.empty()); EXPECT_TRUE(entity->properties.empty());
} }
TEST_F(SchemaOrgValidatorTest, ValidEntityPropertyValueDescendedType) {
EntityPtr entity = Entity::New();
entity->type = entity::kVideoObject;
PropertyPtr property = Property::New();
property->name = property::kThumbnail;
property->values = Values::New();
// Barcode is descended from ImageObject, an acceptable type for the thumbnail
// property. So barcode should also be accepted.
EntityPtr value = Entity::New();
value->type = entity::kBarcode;
property->values->entity_values.push_back(std::move(value));
entity->properties.push_back(std::move(property));
bool validated_entity = ValidateEntity(entity.get());
EXPECT_TRUE(validated_entity);
EXPECT_EQ(1u, entity->properties.size());
}
TEST_F(SchemaOrgValidatorTest, ValidRepeatedEntityPropertyValue) { TEST_F(SchemaOrgValidatorTest, ValidRepeatedEntityPropertyValue) {
EntityPtr entity = Entity::New(); EntityPtr entity = Entity::New();
entity->type = entity::kRestaurant; entity->type = entity::kRestaurant;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment