From aae750ae92c9cae07d3d440e5c9316f589490ff5 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Fri, 3 May 2024 14:19:33 +0100 Subject: [PATCH] Remove Option from ModelWithPermissions (#529) ## Description Much as per https://github.com/hasura/v3-engine/pull/524 for `CommandWithPermission`, this removes the `Option` from `ModelWithPermissions`. Again, select permissions are opt-in, so `Some(HashMap::new())` is the same as `None`, so we can remove this layer of indirection, which I believe was only there because we were building up the `Model` type incrementally. Functional no-op. V3_GIT_ORIGIN_REV_ID: fcb6479f32b001380ae24db6439e96339a868692 --- .../engine/src/schema/model_arguments.rs | 29 ++-- v3/crates/engine/src/schema/permissions.rs | 159 ++++++++---------- .../src/stages/model_permissions/mod.rs | 8 +- .../src/stages/model_permissions/types.rs | 2 +- .../metadata-resolve/src/stages/roles/mod.rs | 6 +- 5 files changed, 87 insertions(+), 117 deletions(-) diff --git a/v3/crates/engine/src/schema/model_arguments.rs b/v3/crates/engine/src/schema/model_arguments.rs index 2b8d441214c..c973c032b48 100644 --- a/v3/crates/engine/src/schema/model_arguments.rs +++ b/v3/crates/engine/src/schema/model_arguments.rs @@ -84,27 +84,20 @@ pub fn build_model_argument_fields( gql_schema::DeprecationStatus::NotDeprecated, ); - // if we have select_permissions, then work out which arguments to include in the schema - match &model.select_permissions { - // if no permissions apply, allow the argument through - None => Ok((field_name, builder.allow_all_namespaced(input_field, None))), - Some(permissions_by_namespace) => { - let mut namespaced_annotations = HashMap::new(); + let mut namespaced_annotations = HashMap::new(); - for (namespace, permission) in permissions_by_namespace { - // if there is a preset for this argument, remove it from the schema - // so the user cannot provide one - if permission.argument_presets.get(argument_name).is_none() { - namespaced_annotations.insert(namespace.clone(), None); - } - } - - Ok(( - field_name, - builder.conditional_namespaced(input_field, namespaced_annotations), - )) + for (namespace, permission) in &model.select_permissions { + // if there is a preset for this argument, remove it from the schema + // so the user cannot provide one + if permission.argument_presets.get(argument_name).is_none() { + namespaced_annotations.insert(namespace.clone(), None); } } + + Ok(( + field_name, + builder.conditional_namespaced(input_field, namespaced_annotations), + )) }) .collect() } diff --git a/v3/crates/engine/src/schema/permissions.rs b/v3/crates/engine/src/schema/permissions.rs index 628f0f46461..57e42bda15b 100644 --- a/v3/crates/engine/src/schema/permissions.rs +++ b/v3/crates/engine/src/schema/permissions.rs @@ -20,43 +20,38 @@ pub(crate) fn get_select_permissions_namespace_annotations( ) -> Result>, schema::Error> { let mut permissions: HashMap> = model .select_permissions - .as_ref() - .map(|permissions| { - permissions - .iter() - .map(|(role, select_permission)| { - ( - role.clone(), - Some(types::NamespaceAnnotation::Model { - filter: select_permission.filter.clone(), - argument_presets: types::ArgumentPresets { - argument_presets: select_permission - .argument_presets - .iter() - .map(|(arg_name, preset)| { - ( - ArgumentNameAndPath { - ndc_argument_name: model - .model - .source - .as_ref() - .and_then(|model_source| { - model_source.argument_mappings.get(arg_name) - }) - .cloned(), - field_path: vec![], - }, - preset.clone(), - ) - }) - .collect(), - }, - }), - ) - }) - .collect() + .iter() + .map(|(role, select_permission)| { + ( + role.clone(), + Some(types::NamespaceAnnotation::Model { + filter: select_permission.filter.clone(), + argument_presets: types::ArgumentPresets { + argument_presets: select_permission + .argument_presets + .iter() + .map(|(arg_name, preset)| { + ( + ArgumentNameAndPath { + ndc_argument_name: model + .model + .source + .as_ref() + .and_then(|model_source| { + model_source.argument_mappings.get(arg_name) + }) + .cloned(), + field_path: vec![], + }, + preset.clone(), + ) + }) + .collect(), + }, + }), + ) }) - .unwrap_or_default(); + .collect(); // if any of model argument's input type has field presets defined, add // them to model argument preset annotations as well. if there is no @@ -453,33 +448,25 @@ pub(crate) fn get_node_field_namespace_permissions( ) -> HashMap { let mut permissions = HashMap::new(); - match &model.select_permissions { - // Model doesn't have any select permissions, so no `FilterPermission` can be obtained - None => {} - Some(select_permissions) => { - for (role, type_output_permission) in - &object_type_representation.type_output_permissions - { - let is_global_id_field_accessible = object_type_representation - .object_type - .global_id_fields - .iter() - .all(|field_name| type_output_permission.allowed_fields.contains(field_name)); + for (role, type_output_permission) in &object_type_representation.type_output_permissions { + let is_global_id_field_accessible = object_type_representation + .object_type + .global_id_fields + .iter() + .all(|field_name| type_output_permission.allowed_fields.contains(field_name)); - if is_global_id_field_accessible { - let select_permission = select_permissions.get(role).map(|s| s.filter.clone()); + if is_global_id_field_accessible { + let select_permission = model.select_permissions.get(role).map(|s| s.filter.clone()); - match select_permission { - // Select permission doesn't exist for the role, so no `FilterPermission` can - // be obtained. - None => {} - Some(select_permission) => { - permissions.insert(role.clone(), select_permission); - } - } - }; + match select_permission { + // Select permission doesn't exist for the role, so no `FilterPermission` can + // be obtained. + None => {} + Some(select_permission) => { + permissions.insert(role.clone(), select_permission); + } } - } + }; } permissions @@ -492,39 +479,31 @@ pub(crate) fn get_entities_field_namespace_permissions( ) -> HashMap { let mut permissions = HashMap::new(); - match &model.select_permissions { - // Model doesn't have any select permissions, so no `FilterPermission` can be obtained - None => {} - Some(select_permissions) => { - for (role, type_output_permission) in - &object_type_representation.type_output_permissions - { - if let Some(apollo_federation_config) = &object_type_representation - .object_type - .apollo_federation_config - { - let is_all_keys_field_accessible = - apollo_federation_config.keys.iter().all(|key_fields| { - key_fields.fields.iter().all(|field_name| { - type_output_permission.allowed_fields.contains(field_name) - }) - }); + for (role, type_output_permission) in &object_type_representation.type_output_permissions { + if let Some(apollo_federation_config) = &object_type_representation + .object_type + .apollo_federation_config + { + let is_all_keys_field_accessible = + apollo_federation_config.keys.iter().all(|key_fields| { + key_fields.fields.iter().all(|field_name| { + type_output_permission.allowed_fields.contains(field_name) + }) + }); - if is_all_keys_field_accessible { - let select_permission = - select_permissions.get(role).map(|s| s.filter.clone()); + if is_all_keys_field_accessible { + let select_permission = + model.select_permissions.get(role).map(|s| s.filter.clone()); - match select_permission { - // Select permission doesn't exist for the role, so no `FilterPermission` can - // be obtained. - None => {} - Some(select_permission) => { - permissions.insert(role.clone(), select_permission); - } - } - }; + match select_permission { + // Select permission doesn't exist for the role, so no `FilterPermission` can + // be obtained. + None => {} + Some(select_permission) => { + permissions.insert(role.clone(), select_permission); + } } - } + }; } } diff --git a/v3/crates/metadata-resolve/src/stages/model_permissions/mod.rs b/v3/crates/metadata-resolve/src/stages/model_permissions/mod.rs index ed6e20e96e2..7e252f1316e 100644 --- a/v3/crates/metadata-resolve/src/stages/model_permissions/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/model_permissions/mod.rs @@ -46,7 +46,7 @@ pub fn resolve( model_name.clone(), ModelWithPermissions { model: model.clone(), - select_permissions: None, + select_permissions: HashMap::new(), }, ) }) @@ -67,8 +67,8 @@ pub fn resolve( model_name: model_name.clone(), })?; - if model.select_permissions.is_none() { - let select_permissions = Some(resolve_model_select_permissions( + if model.select_permissions.is_empty() { + let select_permissions = resolve_model_select_permissions( &model.model, subgraph, permissions, @@ -76,7 +76,7 @@ pub fn resolve( object_types, models, // This is required to get the model for the relationship target boolean_expression_types, - )?); + )?; model.select_permissions = select_permissions; } else { diff --git a/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs b/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs index 2ab78136f12..cd5499f2444 100644 --- a/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs +++ b/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs @@ -20,7 +20,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] pub struct ModelWithPermissions { pub model: models::Model, - pub select_permissions: Option>, + pub select_permissions: HashMap, } #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] diff --git a/v3/crates/metadata-resolve/src/stages/roles/mod.rs b/v3/crates/metadata-resolve/src/stages/roles/mod.rs index 64227ce0df4..7b575632a68 100644 --- a/v3/crates/metadata-resolve/src/stages/roles/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/roles/mod.rs @@ -25,10 +25,8 @@ pub fn resolve( } } for model in models.values() { - if let Some(select_permissions) = &model.select_permissions { - for role in select_permissions.keys() { - roles.push(role.clone()); - } + for role in model.select_permissions.keys() { + roles.push(role.clone()); } } for command in commands.values() {