Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize subgraph names when generating expanded enum values #5814

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Normalize subgraph names when generating expanded enum values #5814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c614a2
Normalize subgraph names when generating expanded enum values
dylan-apollo Aug 13, 2024
d03930c
Remove todone todo
dylan-apollo Aug 13, 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
148 changes: 112 additions & 36 deletions apollo-federation/src/merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,11 @@ impl Merger {
.map(|(_, subgraph)| subgraph)
.collect_vec();
subgraphs.sort_by(|s1, s2| s1.name.cmp(&s2.name));
let mut subgraphs_and_enum_values: Vec<(&ValidFederationSubgraph, Name)> = Vec::new();
let mut subgraphs_and_enum_values = Vec::new();
for subgraph in &subgraphs {
// TODO: Implement JS codebase's name transform (which always generates a valid GraphQL
// name and avoids collisions).
if let Ok(subgraph_name) = Name::new(&subgraph.name.to_uppercase()) {
subgraphs_and_enum_values.push((subgraph, subgraph_name));
} else {
self.errors.push(String::from(
"Subgraph name couldn't be transformed into valid GraphQL name",
));
match EnumValue::new(&subgraph.name) {
Ok(enum_value) => subgraphs_and_enum_values.push((subgraph, enum_value)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to worry about collisions resulting from the normalization? For example, if two names were the same except for characters that were all converted to "_"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only being done for our auto-transformed names, so hopefully that's really unlikely. It'd need to be two subgraphs which have nearly identical names and also have a @connect on the same path.

Long term, though, for sure we should also port this

Err(err) => self.errors.push(err),
}
}
if !self.errors.is_empty() {
Expand Down Expand Up @@ -186,35 +181,35 @@ impl Merger {
ExtendedType::Enum(value) => self.merge_enum_type(
&mut supergraph.types,
&relevant_directives,
subgraph_name.clone(),
subgraph_name,
type_name.clone(),
value,
),
ExtendedType::InputObject(value) => self.merge_input_object_type(
&mut supergraph.types,
&relevant_directives,
subgraph_name.clone(),
subgraph_name,
type_name.clone(),
value,
),
ExtendedType::Interface(value) => self.merge_interface_type(
&mut supergraph.types,
&relevant_directives,
subgraph_name.clone(),
subgraph_name,
type_name.clone(),
value,
),
ExtendedType::Object(value) => self.merge_object_type(
&mut supergraph.types,
&relevant_directives,
subgraph_name.clone(),
subgraph_name,
type_name.clone(),
value,
),
ExtendedType::Union(value) => self.merge_union_type(
&mut supergraph.types,
&relevant_directives,
subgraph_name.clone(),
subgraph_name,
type_name.clone(),
value,
),
Expand Down Expand Up @@ -299,7 +294,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<NamedType, ExtendedType>,
metadata: &DirectiveNames,
subgraph_name: Name,
subgraph_name: &EnumValue,
enum_name: NamedType,
enum_type: &Node<EnumType>,
) {
Expand Down Expand Up @@ -345,7 +340,7 @@ impl Merger {
arguments: vec![
(Node::new(Argument {
name: name!("graph"),
value: Node::new(Value::Enum(subgraph_name.clone())),
value: Node::new(Value::Enum(subgraph_name.to_name())),
})),
],
}));
Expand All @@ -359,7 +354,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<NamedType, ExtendedType>,
directive_names: &DirectiveNames,
subgraph_name: Name,
subgraph_name: &EnumValue,
input_object_name: NamedType,
input_object: &Node<InputObjectType>,
) {
Expand All @@ -369,7 +364,7 @@ impl Merger {

if let ExtendedType::InputObject(obj) = existing_type {
let join_type_directives =
join_type_applied_directive(subgraph_name, iter::empty(), false);
join_type_applied_directive(subgraph_name.clone(), iter::empty(), false);
let mutable_object = obj.make_mut();
mutable_object.directives.extend(join_type_directives);

Expand Down Expand Up @@ -409,7 +404,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<NamedType, ExtendedType>,
directive_names: &DirectiveNames,
subgraph_name: Name,
subgraph_name: &EnumValue,
interface_name: NamedType,
interface: &Node<InterfaceType>,
) {
Expand All @@ -420,7 +415,7 @@ impl Merger {
if let ExtendedType::Interface(intf) = existing_type {
let key_directives = interface.directives.get_all(&directive_names.key);
let join_type_directives =
join_type_applied_directive(subgraph_name, key_directives, false);
join_type_applied_directive(subgraph_name.clone(), key_directives, false);
let mutable_intf = intf.make_mut();
mutable_intf.directives.extend(join_type_directives);

Expand Down Expand Up @@ -466,7 +461,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<NamedType, ExtendedType>,
directive_names: &DirectiveNames,
subgraph_name: Name,
subgraph_name: &EnumValue,
object_name: NamedType,
object: &Node<ObjectType>,
) {
Expand Down Expand Up @@ -595,7 +590,7 @@ impl Merger {
.is_some();

let join_field_directive = join_field_applied_directive(
subgraph_name.clone(),
subgraph_name,
requires_directive_option,
provides_directive_option,
external_field,
Expand All @@ -614,7 +609,7 @@ impl Merger {
// TODO support interface object
let key_directives = object.directives.get_all(&directive_names.key);
let join_type_directives =
join_type_applied_directive(subgraph_name, key_directives, true);
join_type_applied_directive(subgraph_name.clone(), key_directives, true);
intf.make_mut().directives.extend(join_type_directives);
};
// TODO merge fields
Expand All @@ -624,7 +619,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<NamedType, ExtendedType>,
directive_names: &DirectiveNames,
subgraph_name: Name,
subgraph_name: &EnumValue,
union_name: NamedType,
union: &Node<UnionType>,
) {
Expand All @@ -651,7 +646,7 @@ impl Merger {
arguments: vec![
Node::new(Argument {
name: name!("graph"),
value: Node::new(Value::Enum(subgraph_name.clone())),
value: Node::new(Value::Enum(subgraph_name.to_name())),
}),
Node::new(Argument {
name: name!("member"),
Expand All @@ -667,7 +662,7 @@ impl Merger {
&mut self,
types: &mut IndexMap<Name, ExtendedType>,
directive_names: &DirectiveNames,
subgraph_name: Name,
subgraph_name: EnumValue,
scalar_name: NamedType,
ty: &Node<ScalarType>,
) {
Expand All @@ -677,7 +672,7 @@ impl Merger {

if let ExtendedType::Scalar(s) = existing_type {
let join_type_directives =
join_type_applied_directive(subgraph_name.clone(), iter::empty(), false);
join_type_applied_directive(subgraph_name, iter::empty(), false);
s.make_mut().directives.extend(join_type_directives);
self.add_inaccessible(
directive_names,
Expand Down Expand Up @@ -928,15 +923,15 @@ fn copy_union_type(union_name: Name, description: Option<Node<str>>) -> Extended
}

fn join_type_applied_directive<'a>(
subgraph_name: Name,
subgraph_name: EnumValue,
key_directives: impl Iterator<Item = &'a Component<Directive>> + Sized,
is_interface_object: bool,
) -> Vec<Component<Directive>> {
let mut join_type_directive = Directive {
name: name!("join__type"),
arguments: vec![Node::new(Argument {
name: name!("graph"),
value: Node::new(Value::Enum(subgraph_name)),
value: Node::new(Value::Enum(subgraph_name.into())),
})],
};
if is_interface_object {
Expand Down Expand Up @@ -979,15 +974,15 @@ fn join_type_applied_directive<'a>(
}

fn join_implements_applied_directive(
subgraph_name: Name,
subgraph_name: EnumValue,
intf_name: &Name,
) -> Component<Directive> {
Component::new(Directive {
name: name!("join__implements"),
arguments: vec![
Node::new(Argument {
name: name!("graph"),
value: Node::new(Value::Enum(subgraph_name)),
value: Node::new(Value::Enum(subgraph_name.into())),
}),
Node::new(Argument {
name: name!("interface"),
Expand Down Expand Up @@ -1141,7 +1136,7 @@ fn link_purpose_enum_type() -> (Name, EnumType) {
// TODO join spec
fn add_core_feature_join(
supergraph: &mut Schema,
subgraphs_and_enum_values: &Vec<(&ValidFederationSubgraph, Name)>,
subgraphs_and_enum_values: &Vec<(&ValidFederationSubgraph, EnumValue)>,
) {
// @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)
supergraph
Expand Down Expand Up @@ -1366,7 +1361,7 @@ fn join_field_directive_definition() -> DirectiveDefinition {
}

fn join_field_applied_directive(
subgraph_name: Name,
subgraph_name: &EnumValue,
requires: Option<&str>,
provides: Option<&str>,
external: bool,
Expand All @@ -1376,7 +1371,7 @@ fn join_field_applied_directive(
name: name!("join__field"),
arguments: vec![Node::new(Argument {
name: name!("graph"),
value: Node::new(Value::Enum(subgraph_name)),
value: Node::new(Value::Enum(subgraph_name.to_name())),
})],
};
if let Some(required_fields) = requires {
Expand Down Expand Up @@ -1555,7 +1550,7 @@ fn join_union_member_directive_definition() -> DirectiveDefinition {

/// enum Graph
fn join_graph_enum_type(
subgraphs_and_enum_values: &Vec<(&ValidFederationSubgraph, Name)>,
subgraphs_and_enum_values: &Vec<(&ValidFederationSubgraph, EnumValue)>,
) -> (Name, EnumType) {
let join_graph_enum_name = name!("join__Graph");
let mut join_graph_enum_type = EnumType {
Expand All @@ -1581,7 +1576,7 @@ fn join_graph_enum_type(
let graph = EnumValueDefinition {
description: None,
directives: DirectiveList(vec![Node::new(join_graph_applied_directive)]),
value: subgraph_name.clone(),
value: subgraph_name.to_name(),
};
join_graph_enum_type
.values
Expand All @@ -1590,6 +1585,87 @@ fn join_graph_enum_type(
(join_graph_enum_name, join_graph_enum_type)
}

/// Represents a valid enum value in GraphQL, used for building `join__Graph`.
///
/// TODO: Put this in `join_spec_definition.rs` when we convert to using that module.
#[derive(Clone, Debug)]
struct EnumValue(Name);

impl EnumValue {
fn new(raw: &str) -> Result<Self, String> {
let prefix = if raw.starts_with(char::is_numeric) {
Some('_')
} else {
None
};
let name = prefix
.into_iter()
.chain(raw.chars())
.map(|c| match c {
'a'..='z' => c.to_ascii_uppercase(),
'A'..='Z' | '0'..='9' => c,
_ => '_',
})
.collect::<String>();
Name::new(&name)
.map(Self)
.map_err(|_| format!("Failed to transform {raw} into a valid GraphQL name. Got {name}"))
}
fn to_name(&self) -> Name {
self.0.clone()
}

#[cfg(test)]
fn as_str(&self) -> &str {
self.0.as_str()
}
}

impl From<EnumValue> for Name {
fn from(ev: EnumValue) -> Self {
ev.0
}
}

#[cfg(test)]
mod test_enum_value {
#[test]
fn basic() {
let ev = super::EnumValue::new("subgraph").unwrap();
assert_eq!(ev.as_str(), "SUBGRAPH");
}

#[test]
fn with_underscores() {
let ev = super::EnumValue::new("a_subgraph").unwrap();
assert_eq!(ev.as_str(), "A_SUBGRAPH");
}

#[test]
fn with_hyphens() {
let ev = super::EnumValue::new("a-subgraph").unwrap();
assert_eq!(ev.as_str(), "A_SUBGRAPH");
}

#[test]
fn special_symbols() {
let ev = super::EnumValue::new("a$ubgraph").unwrap();
assert_eq!(ev.as_str(), "A_UBGRAPH");
}

#[test]
fn digit_first_char() {
let ev = super::EnumValue::new("1subgraph").unwrap();
assert_eq!(ev.as_str(), "_1SUBGRAPH");
}

#[test]
fn digit_last_char() {
let ev = super::EnumValue::new("subgraph_1").unwrap();
assert_eq!(ev.as_str(), "SUBGRAPH_1");
}
}

fn add_core_feature_inaccessible(supergraph: &mut Schema) {
// @link(url: "https://specs.apollo.dev/inaccessible/v0.2")
let spec = InaccessibleSpecDefinition::new(Version { major: 0, minor: 2 }, None);
Expand Down
3 changes: 2 additions & 1 deletion apollo-federation/src/sources/connect/expand/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ test_expands! {
steelthread,
types_used_twice,
carryover,
circular
circular,
normalize_names
}

test_ignores! {
Expand Down
Loading
Loading