Skip to content

Commit

Permalink
Resolve target booleanExpressionType for relationships earlier (#1370)
Browse files Browse the repository at this point in the history 
<!-- The PR description should answer 2 important questions: -->

### What

We already do this logic in two places, to save me doing it in a third,
resolving this earlier.

If a relationship field on a boolean expression is not specified, we
fall back to the filter expression on the target model.

Functional no-op.

V3_GIT_ORIGIN_REV_ID: 435f73426362bfb72096059d3b212208aa1b0b84
  • Loading branch information
@danieljharvey @hasura-bot
danieljharvey authored and hasura-bot committed Nov 25, 2024
1 parent b5ddc5b commit e3d3029
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 108 deletions.
88 changes: 38 additions & 50 deletions v3/crates/graphql/schema/src/boolean_expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,40 +346,48 @@ fn build_new_comparable_relationships_schema(
}
})?;

// if we have specified a boolean expression type to use for this relationship, look it up
// (if none is specified, we use whichever is specified on the model)
let target_boolean_expression_graphql_type =
match &comparable_relationship.boolean_expression_type {
Some(target_boolean_expression_type_name) => {
let target_boolean_expression = gds
let target_boolean_expression_type_name =
&comparable_relationship.boolean_expression_type;

let target_boolean_expression_graphql_type = {
let target_boolean_expression_graphql_type = match gds
.metadata
.boolean_expression_types
.objects
.get(target_boolean_expression_type_name)
{
Some(bool_exp) => {
Ok(bool_exp.graphql.as_ref().map(|graphql| &graphql.type_name))
}
None => {
// perhaps it's referring to an old-style ObjectBooleanExpressionType
match gds
.metadata
.boolean_expression_types
.objects
.object_boolean_expression_types
.get(target_boolean_expression_type_name)
.ok_or_else(|| Error::InternalBooleanExpressionNotFound {
{
Some(object_bool_exp) => Ok(object_bool_exp
.graphql
.as_ref()
.map(|graphql| &graphql.type_name)),
None => Err(Error::InternalBooleanExpressionNotFound {
type_name: target_boolean_expression_type_name.clone(),
})?;

// if we find a type, make sure it's added to the schema
if let Some(graphql) = &target_boolean_expression.graphql {
let _registered_type_name =
builder.register_type(TypeId::InputObjectBooleanExpressionType {
graphql_type_name: graphql.type_name.clone(),
gds_type_name: target_boolean_expression_type_name.clone(),
});
}),
}
// return type name
target_boolean_expression
.graphql
.as_ref()
.map(|graphql| graphql.type_name.clone())
}
None => {
// no specific type is provided by the relationship, so
// lookup filter expression graphql for target model
filter_graphql_type_for_model(target_model)
}
};
}?;

// if we find a type, make sure it's added to the schema
if let Some(type_name) = target_boolean_expression_graphql_type {
let _registered_type_name =
builder.register_type(TypeId::InputObjectBooleanExpressionType {
graphql_type_name: type_name.clone(),
gds_type_name: target_boolean_expression_type_name.clone(),
});
}
// return type name
target_boolean_expression_graphql_type
};

// lookup type underlying target model
let target_object_type_representation =
Expand All @@ -394,7 +402,7 @@ fn build_new_comparable_relationships_schema(
let (name, schema) = build_model_relationship_schema(
object_type_representation,
target_object_type_representation,
&target_boolean_expression_graphql_type,
target_boolean_expression_graphql_type,
target_model,
target_source,
relationship,
Expand All @@ -412,26 +420,6 @@ fn build_new_comparable_relationships_schema(
Ok(input_fields)
}

fn filter_graphql_type_for_model(
model: &metadata_resolve::ModelWithArgumentPresets,
) -> Option<ast::TypeName> {
match &model.filter_expression_type {
None => None,
Some(ModelExpressionType::BooleanExpressionType(target_boolean_expression_type)) => {
target_boolean_expression_type
.graphql
.as_ref()
.map(|graphql| graphql.type_name.clone())
}
Some(ModelExpressionType::ObjectBooleanExpressionType(target_model_filter_expression)) => {
target_model_filter_expression
.graphql
.as_ref()
.map(|graphql| graphql.type_name.clone())
}
}
}

// build comparable relationships input fields for the older `ObjectBooleanExpressionType`
// TODO(naveen): Add support for command relationships
fn build_comparable_relationships_schema(
Expand Down
9 changes: 9 additions & 0 deletions v3/crates/metadata-resolve/src/stages/boolean_expressions/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::types::subgraph::{Qualified, QualifiedTypeName};
use open_dds::{
data_connector::{DataConnectorName, DataConnectorObjectType},
models::ModelName,
relationships::RelationshipName,
types::{CustomTypeName, FieldName},
};

Expand Down Expand Up @@ -99,6 +100,14 @@ pub enum BooleanExpressionError {
#[error("Field level comparison operator configuration is not fully supported yet. Please add all fields in filterable_fields.")]
FieldLevelComparisonOperatorNeedsAllFields,

#[error(
"Target model {model_name:} not found, referenced in relationship {relationship_name:}"
)]
TargetModelNotFound {
relationship_name: RelationshipName,
model_name: Qualified<ModelName>,
},

#[error("{0}")]
GraphqlConfigError(#[from] graphql_config::GraphqlConfigError),

Expand Down
30 changes: 30 additions & 0 deletions v3/crates/metadata-resolve/src/stages/boolean_expressions/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,34 @@ pub fn resolve(
) -> Result<BooleanExpressionsOutput, BooleanExpressionError> {
let mut raw_boolean_expression_types = BTreeMap::new();
let mut issues = Vec::new();
let mut raw_models = BTreeMap::new();
let mut object_boolean_expression_type_names = BTreeSet::new();

// collect raw models for use in resolving relationships
for open_dds::accessor::QualifiedObject {
path: _,
subgraph,
object: model,
} in &metadata_accessor.models
{
raw_models.insert(
Qualified::new(subgraph.clone(), model.name().clone()),
model,
);
}

// collect raw object_boolean_expression_type names for validation
for open_dds::accessor::QualifiedObject {
path: _,
subgraph,
object: object_boolean_expression_type,
} in &metadata_accessor.object_boolean_expression_types
{
object_boolean_expression_type_names.insert(Qualified::new(
subgraph.clone(),
object_boolean_expression_type.name.clone(),
));
}

// first we collect all the boolean_expression_types
// so we have a full set to refer to when resolving them
Expand Down Expand Up @@ -80,6 +108,8 @@ pub fn resolve(
&boolean_expression_scalar_types,
&raw_boolean_expression_types,
relationships,
&raw_models,
&object_boolean_expression_type_names,
graphql_config,
&mut graphql_types,
flags,
Expand Down
114 changes: 82 additions & 32 deletions v3/crates/metadata-resolve/src/stages/boolean_expressions/object.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use open_dds::{
DataConnectorOperatorMapping,
},
data_connector::DataConnectorName,
models::ModelName,
types::{CustomTypeName, FieldName, TypeName},
};

Expand Down Expand Up @@ -50,6 +51,8 @@ pub(crate) fn resolve_object_boolean_expression_type(
>,
raw_boolean_expression_types: &RawBooleanExpressionTypes,
relationships: &relationships::Relationships,
raw_models: &BTreeMap<Qualified<ModelName>, &open_dds::models::Model>,
object_boolean_expression_type_names: &BTreeSet<Qualified<CustomTypeName>>,
graphql_config: &graphql_config::GraphqlConfig,
graphql_types: &mut BTreeSet<ast::TypeName>,
flags: &open_dds::flags::Flags,
Expand Down Expand Up @@ -100,6 +103,8 @@ pub(crate) fn resolve_object_boolean_expression_type(
relationships,
subgraph,
raw_boolean_expression_types,
raw_models,
object_boolean_expression_type_names,
)?;

// resolve graphql schema information
Expand Down Expand Up @@ -145,6 +150,8 @@ fn resolve_comparable_relationships(
relationships: &relationships::Relationships,
subgraph: &SubgraphName,
raw_boolean_expression_types: &RawBooleanExpressionTypes,
raw_models: &BTreeMap<Qualified<ModelName>, &open_dds::models::Model>,
object_boolean_expression_type_names: &BTreeSet<Qualified<CustomTypeName>>,
) -> Result<BTreeMap<FieldName, BooleanExpressionComparableRelationship>, BooleanExpressionError> {
let mut resolved_comparable_relationships = BTreeMap::new();

Expand All @@ -156,40 +163,83 @@ fn resolve_comparable_relationships(

match relationship {
relationships::Relationship::Relationship(relationship) => {
// if the relationship has provided an optional boolean_expression_type, let's
// check it makes sense
let target_boolean_expression_type = comparable_relationship
.boolean_expression_type
.as_ref()
.map(
|target_boolean_expression_type_name| -> Result<_, BooleanExpressionError> {
// create target boolean expression name
let target_boolean_expression_type = Qualified::new(
crate::helpers::relationship::get_target_subgraph(relationship)
.unwrap_or(subgraph.clone()),
target_boolean_expression_type_name.clone(),
);

// ...and ensure it exists
let _raw_boolean_expression_type =
helpers::lookup_raw_boolean_expression(
boolean_expression_type_name,
&target_boolean_expression_type,
raw_boolean_expression_types,
)?;
let target_subgraph =
crate::helpers::relationship::get_target_subgraph(relationship)
.unwrap_or(subgraph.clone());

Ok(target_boolean_expression_type)
let optional_target_boolean_expression_type_name = match &comparable_relationship
.boolean_expression_type
{
Some(target_boolean_expression_type_name) => {
Ok(Some(target_boolean_expression_type_name))
}
None => {
// if nothing is defined we fall back to the boolean expression for the target
match &relationship.target {
open_dds::relationships::RelationshipTarget::Model(model_target) => {
let target_model_name = Qualified::new(
target_subgraph.clone(),
model_target.name.clone(),
);

match raw_models.get(&target_model_name) {
Some(raw_model) => {
Ok(raw_model.filter_expression_type().as_ref())
}
None => Err(BooleanExpressionError::TargetModelNotFound {
relationship_name: comparable_relationship
.relationship_name
.clone(),
model_name: target_model_name,
}),
}
}
open_dds::relationships::RelationshipTarget::Command(_) => {
// command targets not currently supported in boolean expressions
// we just ignore these for now
Ok(None)
}
}
}
}?;

// if the target is a Model, include it
if let Some(target_boolean_expression_type_name) =
optional_target_boolean_expression_type_name
{
// create target boolean expression name
let target_boolean_expression_type = Qualified::new(
target_subgraph,
target_boolean_expression_type_name.clone(),
);

// ...and ensure it exists
match helpers::lookup_raw_boolean_expression(
boolean_expression_type_name,
&target_boolean_expression_type,
raw_boolean_expression_types,
) {
Ok(_) => Ok(()),
Err(e) => {
// it might be an old-style `ObjectBooleanExpressionType`
if object_boolean_expression_type_names
.contains(&target_boolean_expression_type)
{
Ok(())
} else {
Err(e)
}
}
}?;

resolved_comparable_relationships.insert(
FieldName::new(comparable_relationship.relationship_name.inner().clone()),
BooleanExpressionComparableRelationship {
relationship_name: comparable_relationship.relationship_name.clone(),
boolean_expression_type: target_boolean_expression_type,
},
)
.transpose()?;

resolved_comparable_relationships.insert(
FieldName::new(comparable_relationship.relationship_name.inner().clone()),
BooleanExpressionComparableRelationship {
relationship_name: comparable_relationship.relationship_name.clone(),
boolean_expression_type: target_boolean_expression_type,
},
);
);
}
}

// If the relationship is to an unknown subgraph, skip it because we're in
Expand Down
2 changes: 1 addition & 1 deletion v3/crates/metadata-resolve/src/stages/boolean_expressions/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ pub struct BooleanExpressionComparableRelationship {

/// The boolean expression type to use for comparison. This is optional for relationships to
/// models, and defaults to the filterExpressionType of the model
pub boolean_expression_type: Option<Qualified<CustomTypeName>>,
pub boolean_expression_type: Qualified<CustomTypeName>,
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
Expand Down
35 changes: 10 additions & 25 deletions v3/crates/metadata-resolve/src/stages/model_permissions/model_permission.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,6 @@ pub(crate) fn resolve_model_predicate_with_type(
fields,
type_name,
&relationship.relationship_name,
target_model,
boolean_expression_types,
flags,
),
Expand Down Expand Up @@ -762,7 +761,6 @@ fn lookup_relationship_in_boolean_expression(
fields: &boolean_expressions::ResolvedObjectBooleanExpressionTypeFields,
type_name: &Qualified<CustomTypeName>,
relationship_name: &open_dds::relationships::RelationshipName,
target_model: &models_graphql::ModelWithGraphql,
boolean_expression_types: &boolean_expressions::BooleanExpressionTypes,
flags: &open_dds::flags::Flags,
) -> Result<Option<boolean_expressions::ResolvedObjectBooleanExpressionTypeFields>, Error> {
Expand All @@ -779,29 +777,16 @@ fn lookup_relationship_in_boolean_expression(
})?;

// lookup the boolean expression type for this comparable
// relationship
// if it is defined, we fetch it from metadata
match &comparable_relationship.boolean_expression_type {
Some(target_bool_exp_name) => boolean_expression_types
.objects
.get(target_bool_exp_name)
.map(|bool_exp| bool_exp.get_fields(flags).cloned())
.ok_or_else(|| {
Error::from(TypePredicateError::BooleanExpressionNotFound {
boolean_expression_name: target_bool_exp_name.clone(),
})
}),
None => {
// if it is not defined we fall back to the one defined on the model
// we ignore `ObjectBooleanExpressionType` as we should not be using a mixture
match &target_model.filter_expression_type {
Some(models_graphql::ModelExpressionType::BooleanExpressionType(bool_exp)) => {
Ok(bool_exp.get_fields(flags).cloned())
}
_ => Ok(None),
}
}
}
// relationship and fetch it from metadata
boolean_expression_types
.objects
.get(&comparable_relationship.boolean_expression_type)
.map(|bool_exp| bool_exp.get_fields(flags).cloned())
.ok_or_else(|| {
Error::from(TypePredicateError::BooleanExpressionNotFound {
boolean_expression_name: comparable_relationship.boolean_expression_type.clone(),
})
})
}

// this is only used for the older `ObjectBooleanExpressionType` where we
Expand Down

0 comments on commit e3d3029

Please sign in to comment.