From dda2fb8eca1c3479098033b87c140f5fea1105d0 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Tue, 26 Mar 2024 09:58:17 +0000 Subject: [PATCH] more explicit data flow in metadata resolver (#403) ## Description As noted in https://github.com/hasura/v3-engine/pull/398, the metadata resolver works by passing a lot of mutable references around, making the flow of data quite difficult to follow. This changes the passing of `types` to make it more explicit. Where possible, any mutation is pushed up to the top-level, or we return owned values. V3_GIT_ORIGIN_REV_ID: 61251929cbc9b9410fabba85b58738037dfcb586 --- .../engine/src/metadata/resolved/metadata.rs | 90 +++++++++++-------- 1 file changed, 54 insertions(+), 36 deletions(-) diff --git a/v3/crates/engine/src/metadata/resolved/metadata.rs b/v3/crates/engine/src/metadata/resolved/metadata.rs index 5c3507e96fe..85fa26e9bb5 100644 --- a/v3/crates/engine/src/metadata/resolved/metadata.rs +++ b/v3/crates/engine/src/metadata/resolved/metadata.rs @@ -113,43 +113,39 @@ pub fn resolve_metadata(metadata: open_dds::Metadata) -> Result // resolve data connectors let mut data_connectors = resolve_data_connectors(&metadata_accessor)?; - let mut types: HashMap, TypeRepresentation> = HashMap::new(); let mut existing_graphql_types: HashSet = HashSet::new(); // we collect all the types with global id fields, and models with global id source for that field. this is used // later for validation, such that a type with global id field must have atleast one model with global id source let mut global_id_enabled_types: HashMap, Vec>> = HashMap::new(); + let mut apollo_federation_entity_enabled_types: HashMap< Qualified, Option>, > = HashMap::new(); // resolve object types - // TODO: make this return more values rather than blindly mutating it's inputs - let data_connector_type_mappings = resolve_data_connector_type_mappings_and_objects( + let (data_connector_type_mappings, types) = resolve_data_connector_type_mappings_and_objects( &metadata_accessor, &data_connectors, - &mut types, &mut existing_graphql_types, &mut global_id_enabled_types, &mut apollo_federation_entity_enabled_types, )?; // resolve scalar types - // TODO: make this return values rather than blindly mutating it's inputs - resolve_scalar_types(&metadata_accessor, &mut types, &mut existing_graphql_types)?; + let scalar_types = resolve_scalar_types(&metadata_accessor, &mut existing_graphql_types)?; // resolve type permissions - // TODO: make this return values rather than blindly mutating it's inputs - resolve_type_permissions(&metadata_accessor, &mut types)?; + let types = resolve_type_permissions(&metadata_accessor, extend_types(types, scalar_types)?)?; // resolve object boolean expression types let boolean_expression_types = resolve_boolean_expression_types( &metadata_accessor, &data_connectors, &data_connector_type_mappings, - &mut types, + &types, &mut existing_graphql_types, )?; @@ -158,7 +154,7 @@ pub fn resolve_metadata(metadata: open_dds::Metadata) -> Result resolve_data_connector_scalar_representations( &metadata_accessor, &mut data_connectors, - &mut types, + &types, &mut existing_graphql_types, )?; @@ -171,7 +167,7 @@ pub fn resolve_metadata(metadata: open_dds::Metadata) -> Result &metadata_accessor, &data_connectors, &data_connector_type_mappings, - &mut types, + &types, &mut existing_graphql_types, &mut global_id_enabled_types, &mut apollo_federation_entity_enabled_types, @@ -205,11 +201,10 @@ pub fn resolve_metadata(metadata: open_dds::Metadata) -> Result )?; // resolve relationships - // TODO: make this return values rather than blindly mutating it's inputs - resolve_relationships( + let types = resolve_relationships( &metadata_accessor, &data_connectors, - &mut types, + types, &models, &commands, )?; @@ -304,15 +299,21 @@ fn resolve_data_connectors( fn resolve_data_connector_type_mappings_and_objects( metadata_accessor: &open_dds::accessor::MetadataAccessor, data_connectors: &HashMap, DataConnectorContext>, - types: &mut HashMap, TypeRepresentation>, existing_graphql_types: &mut HashSet, global_id_enabled_types: &mut HashMap, Vec>>, apollo_federation_entity_enabled_types: &mut HashMap< Qualified, Option>, >, -) -> Result { +) -> Result< + ( + DataConnectorTypeMappings, + HashMap, TypeRepresentation>, + ), + Error, +> { let mut data_connector_type_mappings = DataConnectorTypeMappings::new(); + let mut types = HashMap::new(); for open_dds::accessor::QualifiedObject { subgraph, @@ -370,17 +371,34 @@ fn resolve_data_connector_type_mappings_and_objects( } } - Ok(data_connector_type_mappings) + Ok((data_connector_type_mappings, types)) +} + +/// combine two sets of types, returning a `DuplicateTypeDefinition` error if we find duplicates +/// along the way +fn extend_types( + mut old_types: HashMap, TypeRepresentation>, + new_types: HashMap, TypeRepresentation>, +) -> Result, TypeRepresentation>, Error> { + for new_type_name in new_types.keys() { + if old_types.contains_key(new_type_name) { + return Err(Error::DuplicateTypeDefinition { + name: new_type_name.clone(), + }); + } + } + old_types.extend(new_types); + Ok(old_types) } /// resolve scalar types -/// this currently works by mutating `types` and `existing_graphql_types`, we should try +/// this currently works by mutating `existing_graphql_types`, we should try /// and change this to return new values here and make the caller combine them together fn resolve_scalar_types( metadata_accessor: &open_dds::accessor::MetadataAccessor, - types: &mut HashMap, TypeRepresentation>, existing_graphql_types: &mut HashSet, -) -> Result<(), Error> { +) -> Result, TypeRepresentation>, Error> { + let mut scalar_types = HashMap::new(); for open_dds::accessor::QualifiedObject { subgraph, object: scalar_type, @@ -392,9 +410,11 @@ fn resolve_scalar_types( .map(ast::TypeName) .map(Some), }?; + let qualified_scalar_type_name = Qualified::new(subgraph.to_string(), scalar_type.name.clone()); - if types + + if scalar_types .insert( qualified_scalar_type_name.clone(), TypeRepresentation::ScalarType(ScalarTypeRepresentation { @@ -410,16 +430,15 @@ fn resolve_scalar_types( } check_conflicting_graphql_types(existing_graphql_types, graphql_type_name.as_ref())?; } - Ok(()) + Ok(scalar_types) } /// resolve type permissions -/// this currently works by blindly mutation `types`, let's change it to more explicitly -/// return new values if possible +/// this works by mutating `types`, and returning an owned value fn resolve_type_permissions( metadata_accessor: &open_dds::accessor::MetadataAccessor, - types: &mut HashMap, TypeRepresentation>, -) -> Result<(), Error> { + mut types: HashMap, TypeRepresentation>, +) -> Result, TypeRepresentation>, Error> { // resolve type permissions for open_dds::accessor::QualifiedObject { subgraph, @@ -441,7 +460,7 @@ fn resolve_type_permissions( } } } - Ok(()) + Ok(types) } /// resolve object boolean expression types @@ -449,7 +468,7 @@ fn resolve_boolean_expression_types( metadata_accessor: &open_dds::accessor::MetadataAccessor, data_connectors: &HashMap, DataConnectorContext>, data_connector_type_mappings: &DataConnectorTypeMappings, - types: &mut HashMap, TypeRepresentation>, + types: &HashMap, TypeRepresentation>, existing_graphql_types: &mut HashSet, ) -> Result, ObjectBooleanExpressionType>, Error> { let mut boolean_expression_types = HashMap::new(); @@ -479,12 +498,12 @@ fn resolve_boolean_expression_types( } /// resolve data connector scalar representations -/// this currently works by mutating `types` and `existing_graphql_types`, let's change it to +/// this currently works by mutating `existing_graphql_types`, let's change it to /// return new values instead if possible fn resolve_data_connector_scalar_representations( metadata_accessor: &open_dds::accessor::MetadataAccessor, data_connectors: &mut HashMap, DataConnectorContext>, - types: &mut HashMap, TypeRepresentation>, + types: &HashMap, TypeRepresentation>, existing_graphql_types: &mut HashSet, ) -> Result<(), Error> { for open_dds::accessor::QualifiedObject { @@ -562,7 +581,7 @@ fn resolve_models( metadata_accessor: &open_dds::accessor::MetadataAccessor, data_connectors: &HashMap, DataConnectorContext>, data_connector_type_mappings: &DataConnectorTypeMappings, - types: &mut HashMap, TypeRepresentation>, + types: &HashMap, TypeRepresentation>, existing_graphql_types: &mut HashSet, global_id_enabled_types: &mut HashMap, Vec>>, apollo_federation_entity_enabled_types: &mut HashMap< @@ -641,15 +660,14 @@ fn resolve_models( } /// resolve relationships -/// this currently works by mutating `types`, let's change it to -/// return new values instead where possible +/// returns updated `types` value fn resolve_relationships( metadata_accessor: &open_dds::accessor::MetadataAccessor, data_connectors: &HashMap, DataConnectorContext>, - types: &mut HashMap, TypeRepresentation>, + mut types: HashMap, TypeRepresentation>, models: &IndexMap, Model>, commands: &IndexMap, command::Command>, -) -> Result<(), Error> { +) -> Result, TypeRepresentation>, Error> { for open_dds::accessor::QualifiedObject { subgraph, object: relationship, @@ -695,7 +713,7 @@ fn resolve_relationships( } } } - Ok(()) + Ok(types) } /// resolve command permissions