Skip to content

Commit

Permalink
H-3380, H-3381: Improve error handling and data type ID inference (#5284
Browse files Browse the repository at this point in the history
)
  • Loading branch information
TimDiekmann authored Sep 30, 2024
1 parent f69f236 commit 49a3f9c
Show file tree
Hide file tree
Showing 13 changed files with 87 additions and 126 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions apps/hash-graph/bins/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ codec = { workspace = true }
error-stack = { workspace = true }
graph = { workspace = true, features = ["clap"] }
graph-api = { workspace = true }
graph-types = { workspace = true }
hash-tracing = { workspace = true, features = ["clap"] }
temporal-client = { workspace = true }
test-server = { workspace = true, optional = true }
Expand Down
1 change: 1 addition & 0 deletions apps/hash-graph/bins/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"@rust/error-stack": "0.5.0",
"@rust/graph": "0.0.0-private",
"@rust/graph-api": "0.0.0-private",
"@rust/graph-types": "0.0.0-private",
"@rust/hash-tracing": "0.0.0-private",
"@rust/temporal-client": "0.0.0-private",
"@rust/test-server": "0.0.0-private",
Expand Down
2 changes: 1 addition & 1 deletion apps/hash-graph/bins/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;

fn main() -> Result<(), GraphError> {
load_env(None);
validation::error::install_error_stack_hooks();
graph_types::knowledge::property::error::install_error_stack_hooks();

let Args {
subcommand,
Expand Down
45 changes: 0 additions & 45 deletions apps/hash-graph/libs/graph/src/store/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,51 +231,6 @@ where
.get(0))
}

#[expect(refining_impl_trait)]
async fn has_children(&self, data_type: &VersionedUrl) -> Result<bool, Report<QueryError>> {
let client = self.store.as_client().client();

Ok(client
.query_one(
"
SELECT EXISTS (
SELECT 1 FROM data_type_inherits_from
WHERE target_data_type_ontology_id = $1
);
",
&[&DataTypeId::from_url(data_type)],
)
.await
.change_context(QueryError)?
.get(0))
}

#[expect(refining_impl_trait)]
async fn has_non_abstract_parents(
&self,
data_type: &VersionedUrl,
) -> Result<bool, Report<QueryError>> {
let client = self.store.as_client().client();

Ok(client
.query_one(
"
SELECT EXISTS (
SELECT 1
FROM data_type_inherits_from
JOIN data_types
ON data_types.ontology_id = target_data_type_ontology_id
WHERE source_data_type_ontology_id = $1
AND data_types.schema->>'abstract' = 'false'
);
",
&[&DataTypeId::from_url(data_type)],
)
.await
.change_context(QueryError)?
.get(0))
}

#[expect(refining_impl_trait)]
async fn find_conversion(
&self,
Expand Down
8 changes: 0 additions & 8 deletions apps/hash-graph/libs/store/src/filter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,14 +532,6 @@ mod tests {
unimplemented!()
}

async fn has_children(&self, _: &VersionedUrl) -> Result<bool, Report<!>> {
unimplemented!()
}

async fn has_non_abstract_parents(&self, _: &VersionedUrl) -> Result<bool, Report<!>> {
unimplemented!()
}

async fn find_conversion(
&self,
_: &VersionedUrl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ impl AnyOfConstraints {
.attempt(schema.constraints.validate_value(value))
.is_some()
{
// We found a valid schema, so we can return early.
let _: Result<(), _> = status.finish();
return Ok(());
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use std::collections::HashSet;

use error_stack::Report;
use graph_types::knowledge::property::{PropertyWithMetadata, PropertyWithMetadataObject};
use serde_json::Value as JsonValue;
use type_system::{
schema::{ClosedEntityType, DataType, PropertyType},
url::VersionedUrl,
};

use crate::knowledge::property::{PropertyWithMetadata, PropertyWithMetadataObject};

pub fn install_error_stack_hooks() {
Report::install_debug_hook::<Actual>(|actual, context| match actual {
Actual::Json(json) => context.push_body(format!("actual: {json:#}")),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod error;
pub mod visitor;

pub use self::{
Expand Down
57 changes: 46 additions & 11 deletions libs/@local/hash-graph-types/rust/src/knowledge/property/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::{
knowledge::property::{
PropertyWithMetadata, PropertyWithMetadataArray, PropertyWithMetadataObject,
PropertyWithMetadataValue, ValueMetadata,
error::{Actual, Expected},
},
ontology::{
DataTypeProvider, DataTypeWithMetadata, OntologyTypeProvider, PropertyTypeProvider,
Expand Down Expand Up @@ -47,10 +48,12 @@ pub enum TraversalError {
},
#[error("a value was expected, but the property provided was of type `{actual}`")]
ExpectedValue { actual: JsonSchemaValueType },
#[error("The property provided is ambiguous")]
#[error("The property provided is ambiguous, more than one schema passed the validation.")]
AmbiguousProperty { actual: PropertyWithMetadata },
#[error("The data type ID was not specified and is ambiguous.")]
AmbiguousDataType,
#[error("Could not find a suitable data type for the property")]
DataTypeUnspecified,

#[error(
"the value provided does not match the data type in the metadata, expected `{expected}` \
Expand All @@ -64,6 +67,15 @@ pub enum TraversalError {
AbstractDataType { id: VersionedUrl },
#[error("the value provided does not match the constraints of the data type")]
ConstraintUnfulfilled,
#[error("the value provided does not match the data type")]
DataTypeUnfulfilled,
#[error(
"the value provided does not match the property type. Exactly one constraint has to be \
fulfilled."
)]
PropertyTypeUnfulfilled,
#[error("the entity provided does not match the entity type")]
EntityTypeUnfulfilled,
#[error("the property `{key}` was required, but not specified")]
MissingRequiredProperty { key: BaseUrl },
#[error(
Expand Down Expand Up @@ -260,23 +272,31 @@ where
V: EntityVisitor,
P: DataTypeProvider + PropertyTypeProvider + Sync,
{
let mut status = ReportSink::new();
match property {
PropertyWithMetadata::Value(value) => {
PropertyWithMetadata::Value(value) => status.attempt(
visitor
.visit_one_of_property(&schema.one_of, value, type_provider)
.await
}
PropertyWithMetadata::Array(array) => {
.change_context_lazy(|| TraversalError::PropertyTypeUnfulfilled),
),
PropertyWithMetadata::Array(array) => status.attempt(
visitor
.visit_one_of_array(&schema.one_of, array, type_provider)
.await
}
PropertyWithMetadata::Object(object) => {
.change_context_lazy(|| TraversalError::PropertyTypeUnfulfilled),
),
PropertyWithMetadata::Object(object) => status.attempt(
visitor
.visit_one_of_object(&schema.one_of, object, type_provider)
.await
}
}
.change_context_lazy(|| TraversalError::PropertyTypeUnfulfilled),
),
};
status
.finish()
.attach_lazy(|| Expected::PropertyType(schema.clone()))
.attach_lazy(|| Actual::Property(property.clone()))
}

/// Walks through an array property using the provided schema.
Expand Down Expand Up @@ -469,6 +489,9 @@ where
type_provider,
)
.await
.attach_lazy(|| Expected::DataType(data_type.borrow().schema.clone()))
.attach_lazy(|| Actual::Json(property.value.clone()))
.change_context(TraversalError::DataTypeUnfulfilled)
{
status.append(error);
} else {
Expand All @@ -490,7 +513,11 @@ where

match passed {
0 => status.finish(),
1 => Ok(()),
1 => {
// We ignore potential errors here, as we have exactly one successful result.
let _: Result<(), _> = status.finish();
Ok(())
}
_ => {
status.capture(TraversalError::AmbiguousProperty {
actual: PropertyWithMetadata::Value(property.clone()),
Expand Down Expand Up @@ -549,7 +576,11 @@ where

match passed {
0 => status.finish(),
1 => Ok(()),
1 => {
// We ignore potential errors here, as we have exactly one successful result.
let _: Result<(), _> = status.finish();
Ok(())
}
_ => {
status.capture(TraversalError::AmbiguousProperty {
actual: PropertyWithMetadata::Array(array.clone()),
Expand Down Expand Up @@ -609,7 +640,11 @@ where

match passed {
0 => status.finish(),
1 => Ok(()),
1 => {
// We ignore potential errors here, as we have exactly one successful result.
let _: Result<(), _> = status.finish();
Ok(())
}
_ => {
status.capture(TraversalError::AmbiguousProperty {
actual: PropertyWithMetadata::Object(object.clone()),
Expand Down
14 changes: 0 additions & 14 deletions libs/@local/hash-graph-types/rust/src/ontology/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,20 +184,6 @@ pub trait DataTypeProvider: OntologyTypeProvider<DataTypeWithMetadata> {
parent: &BaseUrl,
) -> impl Future<Output = Result<bool, Report<impl Context>>> + Send;

// TODO: Remove when the data type ID is forced to be passed
// see https://linear.app/hash/issue/H-2800/validate-that-a-data-type-id-is-always-specified
fn has_children(
&self,
data_type: &VersionedUrl,
) -> impl Future<Output = Result<bool, Report<impl Context>>> + Send;

// TODO: Remove when the data type ID is forced to be passed
// see https://linear.app/hash/issue/H-2800/validate-that-a-data-type-id-is-always-specified
fn has_non_abstract_parents(
&self,
data_type: &VersionedUrl,
) -> impl Future<Output = Result<bool, Report<impl Context>>> + Send;

fn find_conversion(
&self,
source_data_type_id: &VersionedUrl,
Expand Down
58 changes: 31 additions & 27 deletions libs/@local/hash-validation/src/entity_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,43 +380,49 @@ impl EntityVisitor for EntityPreprocessor {
// TODO: Remove when the data type ID is forced to be passed
// see https://linear.app/hash/issue/H-2800/validate-that-a-data-type-id-is-always-specified
if property.metadata.data_type_id.is_none() {
let mut infer_status = ReportSink::new();
let mut possible_data_types = HashSet::new();

for values in schema {
if let PropertyValues::DataTypeReference(data_type_ref) = values {
let has_children = type_provider
.has_children(&data_type_ref.url)
.await
.change_context_lazy(|| TraversalError::DataTypeRetrieval {
id: data_type_ref.clone(),
})?;
if has_children {
status.capture(TraversalError::AmbiguousDataType);
possible_data_types.clear();
break;
}

let has_non_abstract_parents = type_provider
.has_non_abstract_parents(&data_type_ref.url)
.await
.change_context_lazy(|| TraversalError::DataTypeRetrieval {
id: data_type_ref.clone(),
})?;

if has_non_abstract_parents {
status.capture(TraversalError::AmbiguousDataType);
possible_data_types.clear();
break;
let Some(data_type) = infer_status.attempt(
type_provider
.provide_type(&data_type_ref.url)
.await
.change_context_lazy(|| TraversalError::DataTypeRetrieval {
id: data_type_ref.clone(),
}),
) else {
continue;
};

if data_type.borrow().schema.r#abstract {
infer_status.capture(TraversalError::AbstractDataType {
id: data_type_ref.url.clone(),
});
continue;
}

possible_data_types.insert(data_type_ref.url.clone());
}
}

let inferred_successfully = status
.attempt(
infer_status
.finish()
.change_context(TraversalError::DataTypeUnspecified),
)
.is_some();

// Only if there is really a single valid data type ID, we set it. Note, that this is
// done before the actual validation step.
if possible_data_types.len() == 1 {
property.metadata.data_type_id = possible_data_types.into_iter().next();
if inferred_successfully {
if possible_data_types.len() == 1 {
property.metadata.data_type_id = possible_data_types.into_iter().next();
} else {
status.capture(TraversalError::AmbiguousDataType);
}
}
}

Expand Down Expand Up @@ -538,8 +544,6 @@ impl EntityVisitor for EntityPreprocessor {
status.append(error);
}
}
} else {
status.capture(TraversalError::AmbiguousDataType);
}

if let Err(error) = walk_one_of_property_value(self, schema, property, type_provider).await
Expand Down
Loading

0 comments on commit 49a3f9c

Please sign in to comment.