Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
574087c
Use SpecDefinition::directive_name_in_spec instead of a custom-built …
tninesling Nov 20, 2024
9545c53
If the cost spec is not present, check if cost and listsize names are…
tninesling Nov 20, 2024
4ccddea
Fix name lookups for cost directive transfer during subgraph extraction
tninesling Nov 20, 2024
16619c3
Undo some changes to apollo-federation public API
tninesling Nov 21, 2024
9224105
Remove unnecessary argument from position-based demand control direct…
tninesling Nov 21, 2024
3cdc462
Remove unnecessary self references from CostSpecDefinition to ensure …
tninesling Nov 21, 2024
a704085
Push details of directive name lookup into the cost spec so abstracti…
tninesling Nov 22, 2024
c4acdad
Fix linter errors
tninesling Nov 22, 2024
454c0fb
Merge branch 'dev' into tninesling/cost-with-updated-composition
tninesling Nov 22, 2024
2fa54dd
Stop cloning federation schema when checking directives
tninesling Nov 22, 2024
0e35b2f
Lint errors again
tninesling Nov 22, 2024
faa79ba
Hack to switch schema types.get() and type_field() to use ahash::Hash…
tninesling Nov 23, 2024
d9e0e99
Force disambiguation between default type_field and the ahash replace…
tninesling Dec 2, 2024
5764937
Revert "Force disambiguation between default type_field and the ahash…
tninesling Dec 2, 2024
a2843d9
Revert "Hack to switch schema types.get() and type_field() to use aha…
tninesling Dec 2, 2024
fac6098
Propagate federation errors up from cost spec
tninesling Dec 2, 2024
40e5382
Handle federation error propagation in demand control plugin
tninesling Dec 2, 2024
679f1e6
Add changeset
tninesling Dec 3, 2024
fdd035a
Merge branch 'dev' into tninesling/cost-with-updated-composition
tninesling Dec 3, 2024
b1d857c
Merge branch 'dev' into tninesling/cost-with-updated-composition
tninesling Dec 3, 2024
8127488
Merge branch 'dev' into tninesling/cost-with-updated-composition
tninesling Dec 4, 2024
aaf84a7
Use internal_error! for Federation errors
tninesling Dec 9, 2024
2de9e2e
Fix committed suggestion batch
tninesling Dec 9, 2024
4d39f57
Merge branch 'dev' into tninesling/cost-with-updated-composition
tninesling Dec 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .changesets/fix_tninesling_cost_name_handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
### Ensure cost directives are picked up when not explicitly imported ([PR #6328](https://github.com/apollographql/router/pull/6328))

With the recent composition changes, importing `@cost` results in a supergraph schema with the cost specification import at the top. The `@cost` directive itself is not explicitly imported, as it's expected to be available as the default export from the cost link. In contrast, uses of `@listSize` to translate to an explicit import in the supergraph.

Old SDL link

```
@link(
url: "https://specs.apollo.dev/cost/v0.1"
import: ["@cost", "@listSize"]
)
```

New SDL link

```
@link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@listSize"])
```

Instead of using the directive names from the import list in the link, the directive names now come from `SpecDefinition::directive_name_in_schema`, which is equivalent to the change we made on the composition side.

By [@tninesling](https://github.com/tninesling) in https://github.com/apollographql/router/pull/6328
264 changes: 215 additions & 49 deletions apollo-federation/src/link/cost_spec_definition.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
use std::collections::HashSet;

use apollo_compiler::ast::Argument;
use apollo_compiler::ast::Directive;
use apollo_compiler::collections::IndexMap;
use apollo_compiler::ast::DirectiveList;
use apollo_compiler::ast::FieldDefinition;
use apollo_compiler::ast::InputValueDefinition;
use apollo_compiler::name;
use apollo_compiler::schema::Component;
use apollo_compiler::schema::EnumType;
use apollo_compiler::schema::ObjectType;
use apollo_compiler::schema::ScalarType;
use apollo_compiler::schema::ExtendedType;
use apollo_compiler::Name;
use apollo_compiler::Node;
use lazy_static::lazy_static;

use crate::error::FederationError;
use crate::internal_error;
use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph;
use crate::link::spec::Identity;
use crate::link::spec::Url;
use crate::link::spec::Version;
Expand All @@ -21,42 +25,42 @@ use crate::schema::position::ObjectTypeDefinitionPosition;
use crate::schema::position::ScalarTypeDefinitionPosition;
use crate::schema::FederationSchema;

pub(crate) const COST_DIRECTIVE_NAME_IN_SPEC: Name = name!("cost");
pub(crate) const COST_DIRECTIVE_NAME_DEFAULT: Name = name!("federation__cost");

pub(crate) const LIST_SIZE_DIRECTIVE_NAME_IN_SPEC: Name = name!("listSize");
pub(crate) const LIST_SIZE_DIRECTIVE_NAME_DEFAULT: Name = name!("federation__listSize");
const COST_DIRECTIVE_NAME: Name = name!("cost");
const COST_DIRECTIVE_WEIGHT_ARGUMENT_NAME: Name = name!("weight");
const LIST_SIZE_DIRECTIVE_NAME: Name = name!("listSize");
const LIST_SIZE_DIRECTIVE_ASSUMED_SIZE_ARGUMENT_NAME: Name = name!("assumedSize");
const LIST_SIZE_DIRECTIVE_SLICING_ARGUMENTS_ARGUMENT_NAME: Name = name!("slicingArguments");
const LIST_SIZE_DIRECTIVE_SIZED_FIELDS_ARGUMENT_NAME: Name = name!("sizedFields");
const LIST_SIZE_DIRECTIVE_REQUIRE_ONE_SLICING_ARGUMENT_ARGUMENT_NAME: Name =
name!("requireOneSlicingArgument");

#[derive(Clone)]
pub(crate) struct CostSpecDefinition {
pub struct CostSpecDefinition {
url: Url,
minimum_federation_version: Option<Version>,
}

macro_rules! propagate_demand_control_directives {
($func_name:ident, $directives_ty:ty, $wrap_ty:expr) => {
pub(crate) fn $func_name(
&self,
subgraph_schema: &FederationSchema,
supergraph_schema: &FederationSchema,
source: &$directives_ty,
subgraph_schema: &FederationSchema,
dest: &mut $directives_ty,
original_directive_names: &IndexMap<Name, Name>,
) -> Result<(), FederationError> {
let cost_directive_name = original_directive_names.get(&COST_DIRECTIVE_NAME_IN_SPEC);
let cost_directive = cost_directive_name.and_then(|name| source.get(name.as_str()));
let cost_directive = Self::cost_directive_name(supergraph_schema)?
.and_then(|name| source.get(name.as_str()));
if let Some(cost_directive) = cost_directive {
dest.push($wrap_ty(self.cost_directive(
dest.push($wrap_ty(Self::cost_directive(
subgraph_schema,
cost_directive.arguments.clone(),
)?));
}

let list_size_directive_name =
original_directive_names.get(&LIST_SIZE_DIRECTIVE_NAME_IN_SPEC);
let list_size_directive =
list_size_directive_name.and_then(|name| source.get(name.as_str()));
let list_size_directive = Self::list_size_directive_name(supergraph_schema)?
.and_then(|name| source.get(name.as_str()));
if let Some(list_size_directive) = list_size_directive {
dest.push($wrap_ty(self.list_size_directive(
dest.push($wrap_ty(Self::list_size_directive(
subgraph_schema,
list_size_directive.arguments.clone(),
)?));
Expand All @@ -68,34 +72,31 @@ macro_rules! propagate_demand_control_directives {
}

macro_rules! propagate_demand_control_directives_to_position {
($func_name:ident, $source_ty:ty, $dest_ty:ty) => {
($func_name:ident, $source_ty:ty, $pos_ty:ty) => {
pub(crate) fn $func_name(
&self,
supergraph_schema: &FederationSchema,
subgraph_schema: &mut FederationSchema,
source: &Node<$source_ty>,
dest: &$dest_ty,
original_directive_names: &IndexMap<Name, Name>,
pos: &$pos_ty,
) -> Result<(), FederationError> {
let cost_directive_name = original_directive_names.get(&COST_DIRECTIVE_NAME_IN_SPEC);
let cost_directive =
cost_directive_name.and_then(|name| source.directives.get(name.as_str()));
let source = pos.get(supergraph_schema.schema())?;
let cost_directive = Self::cost_directive_name(supergraph_schema)?
.and_then(|name| source.directives.get(name.as_str()));
if let Some(cost_directive) = cost_directive {
dest.insert_directive(
pos.insert_directive(
subgraph_schema,
Component::from(
self.cost_directive(subgraph_schema, cost_directive.arguments.clone())?,
),
Component::from(Self::cost_directive(
subgraph_schema,
cost_directive.arguments.clone(),
)?),
)?;
}

let list_size_directive_name =
original_directive_names.get(&LIST_SIZE_DIRECTIVE_NAME_IN_SPEC);
let list_size_directive =
list_size_directive_name.and_then(|name| source.directives.get(name.as_str()));
let list_size_directive = Self::list_size_directive_name(supergraph_schema)?
.and_then(|name| source.directives.get(name.as_str()));
if let Some(list_size_directive) = list_size_directive {
dest.insert_directive(
pos.insert_directive(
subgraph_schema,
Component::from(self.list_size_directive(
Component::from(Self::list_size_directive(
subgraph_schema,
list_size_directive.arguments.clone(),
)?),
Expand All @@ -119,35 +120,32 @@ impl CostSpecDefinition {
}

pub(crate) fn cost_directive(
&self,
schema: &FederationSchema,
arguments: Vec<Node<Argument>>,
) -> Result<Directive, FederationError> {
let name = self
.directive_name_in_schema(schema, &COST_DIRECTIVE_NAME_IN_SPEC)?
.unwrap_or(COST_DIRECTIVE_NAME_DEFAULT);
let name = Self::cost_directive_name(schema)?.ok_or_else(|| {
internal_error!("The \"@cost\" directive is undefined in the target schema")
})?;

Ok(Directive { name, arguments })
}

pub(crate) fn list_size_directive(
&self,
schema: &FederationSchema,
arguments: Vec<Node<Argument>>,
) -> Result<Directive, FederationError> {
let name = self
.directive_name_in_schema(schema, &LIST_SIZE_DIRECTIVE_NAME_IN_SPEC)?
.unwrap_or(LIST_SIZE_DIRECTIVE_NAME_DEFAULT);
let name = Self::list_size_directive_name(schema)?.ok_or_else(|| {
internal_error!("The \"@listSize\" directive is undefined in the target schema")
})?;

Ok(Directive { name, arguments })
}

propagate_demand_control_directives!(
propagate_demand_control_directives,
apollo_compiler::ast::DirectiveList,
DirectiveList,
Node::new
);

propagate_demand_control_directives_to_position!(
propagate_demand_control_directives_for_enum,
EnumType,
Expand All @@ -163,6 +161,81 @@ impl CostSpecDefinition {
ScalarType,
ScalarTypeDefinitionPosition
);

fn for_federation_schema(schema: &FederationSchema) -> Option<&'static Self> {
let link = schema
.metadata()?
.for_identity(&Identity::cost_identity())?;
COST_VERSIONS.find(&link.url.version)
}

/// Returns the name of the `@cost` directive in the given schema, accounting for import aliases or specification name
/// prefixes such as `@federation__cost`. This checks the linked cost specification, if there is one, and falls back
/// to the federation spec.
fn cost_directive_name(schema: &FederationSchema) -> Result<Option<Name>, FederationError> {
if let Some(spec) = Self::for_federation_schema(schema) {
spec.directive_name_in_schema(schema, &COST_DIRECTIVE_NAME)
} else if let Ok(fed_spec) = get_federation_spec_definition_from_subgraph(schema) {
fed_spec.directive_name_in_schema(schema, &COST_DIRECTIVE_NAME)
} else {
Ok(None)
}
}

/// Returns the name of the `@listSize` directive in the given schema, accounting for import aliases or specification name
/// prefixes such as `@federation__listSize`. This checks the linked cost specification, if there is one, and falls back
/// to the federation spec.
fn list_size_directive_name(
schema: &FederationSchema,
) -> Result<Option<Name>, FederationError> {
if let Some(spec) = Self::for_federation_schema(schema) {
spec.directive_name_in_schema(schema, &LIST_SIZE_DIRECTIVE_NAME)
} else if let Ok(fed_spec) = get_federation_spec_definition_from_subgraph(schema) {
fed_spec.directive_name_in_schema(schema, &LIST_SIZE_DIRECTIVE_NAME)
} else {
Ok(None)
}
}

pub fn cost_directive_from_argument(
schema: &FederationSchema,
argument: &InputValueDefinition,
ty: &ExtendedType,
) -> Result<Option<CostDirective>, FederationError> {
let directive_name = Self::cost_directive_name(schema)?;
if let Some(name) = directive_name.as_ref() {
Ok(CostDirective::from_directives(name, &argument.directives)
.or(CostDirective::from_schema_directives(name, ty.directives())))
} else {
Ok(None)
}
}

pub fn cost_directive_from_field(
schema: &FederationSchema,
field: &FieldDefinition,
ty: &ExtendedType,
) -> Result<Option<CostDirective>, FederationError> {
let directive_name = Self::cost_directive_name(schema)?;
if let Some(name) = directive_name.as_ref() {
Ok(CostDirective::from_directives(name, &field.directives)
.or(CostDirective::from_schema_directives(name, ty.directives())))
} else {
Ok(None)
}
}

pub fn list_size_directive_from_field_definition(
schema: &FederationSchema,
field: &FieldDefinition,
) -> Result<Option<ListSizeDirective>, FederationError> {
let directive_name = Self::list_size_directive_name(schema)?;
if let Some(name) = directive_name.as_ref() {
Ok(ListSizeDirective::from_field_definition(name, field))
} else {
Ok(None)
}
}
}

impl SpecDefinition for CostSpecDefinition {
Expand All @@ -185,3 +258,96 @@ lazy_static! {
definitions
};
}

pub struct CostDirective {
weight: i32,
}

impl CostDirective {
pub fn weight(&self) -> f64 {
self.weight as f64
}

fn from_directives(directive_name: &Name, directives: &DirectiveList) -> Option<Self> {
directives
.get(directive_name)?
.specified_argument_by_name(&COST_DIRECTIVE_WEIGHT_ARGUMENT_NAME)?
.to_i32()
.map(|weight| Self { weight })
}

fn from_schema_directives(
directive_name: &Name,
directives: &apollo_compiler::schema::DirectiveList,
) -> Option<Self> {
directives
.get(directive_name)?
.specified_argument_by_name(&COST_DIRECTIVE_WEIGHT_ARGUMENT_NAME)?
.to_i32()
.map(|weight| Self { weight })
}
}

pub struct ListSizeDirective {
pub assumed_size: Option<i32>,
pub slicing_argument_names: Option<HashSet<String>>,
pub sized_fields: Option<HashSet<String>>,
pub require_one_slicing_argument: bool,
}

impl ListSizeDirective {
pub fn from_field_definition(
directive_name: &Name,
definition: &FieldDefinition,
) -> Option<Self> {
let directive = definition.directives.get(directive_name)?;
let assumed_size = Self::assumed_size(directive);
let slicing_argument_names = Self::slicing_argument_names(directive);
let sized_fields = Self::sized_fields(directive);
let require_one_slicing_argument =
Self::require_one_slicing_argument(directive).unwrap_or(true);

Some(Self {
assumed_size,
slicing_argument_names,
sized_fields,
require_one_slicing_argument,
})
}

fn assumed_size(directive: &Directive) -> Option<i32> {
directive
.specified_argument_by_name(&LIST_SIZE_DIRECTIVE_ASSUMED_SIZE_ARGUMENT_NAME)?
.to_i32()
}

fn slicing_argument_names(directive: &Directive) -> Option<HashSet<String>> {
let names = directive
.specified_argument_by_name(&LIST_SIZE_DIRECTIVE_SLICING_ARGUMENTS_ARGUMENT_NAME)?
.as_list()?
.iter()
.flat_map(|arg| arg.as_str())
.map(String::from)
.collect();
Some(names)
}

fn sized_fields(directive: &Directive) -> Option<HashSet<String>> {
let fields = directive
.specified_argument_by_name(&LIST_SIZE_DIRECTIVE_SIZED_FIELDS_ARGUMENT_NAME)?
.as_list()?
.iter()
.flat_map(|arg| arg.as_str())
.map(String::from)
.collect();
Some(fields)
}

fn require_one_slicing_argument(directive: &Directive) -> Option<bool> {
directive
.specified_argument_by_name(
&LIST_SIZE_DIRECTIVE_REQUIRE_ONE_SLICING_ARGUMENT_ARGUMENT_NAME,
)?
.to_bool()
}
}
Loading