breaking(compiler): unify find_operation #447
Conversation
Mimics spec's [`getOperation`](https://spec.graphql.org/October2021/#GetOperation()) functionality and returns the anonymous operation if `None` is specified for operation name.
| db: &dyn HirDatabase, | ||
| file_id: FileId, | ||
| name: String, | ||
| ) -> Option<Arc<OperationDefinition>> { | ||
| db.operations(file_id) | ||
| .iter() | ||
| .find(|def| def.name() == Some(&*name)) | ||
| .cloned() | ||
| } | ||
|
|
||
| pub(crate) fn find_anonymous_operation( | ||
| pub(crate) fn find_operation( | ||
| db: &dyn HirDatabase, | ||
| file_id: FileId, | ||
| name: Option<String>, | ||
| ) -> Option<Arc<OperationDefinition>> { | ||
| db.operations(file_id) | ||
| .iter() | ||
| .find(|def| def.name().is_none()) | ||
| .find(|def| def.name() == name.as_deref()) |
There was a problem hiding this comment.
When the name parameter is None is looks for an anonymous operation. This deviates from the spec’s algorithm, which instead does something like:
let definitions = db.operations(file_id);
if let Some(name) = name {
definitions.iter().find(|def| def.name() == Some(&*name))
} else if definitions.len() == 1 {
// The definition’s `.name()` doesn’t matter
definitions[0].clone()
} else {
None
}There was a problem hiding this comment.
oh, good catch, i misread that entirely!
There was a problem hiding this comment.
we don't (yet, or maybe even never?) return Result from these kinds of queries, so if None is provided and there are several operation definitions, this function will return None. Does that sound ok to everyone? @SimonSapin @goto-bus-stop?
apollo-rs/crates/apollo-compiler/src/database/hir_db.rs
Lines 1626 to 1630 in cd59064
There was a problem hiding this comment.
Yes, sounds good. Depending on whether the parameter is None (which the caller knows of course) in each case there’s only one error case, so it’s fine IMO to have a None return value represent it.
There was a problem hiding this comment.
@SimonSapin could you re-review then?
There was a problem hiding this comment.
Sorry, I think this still doesn’t quite match the spec. The path checking len() == 1 should only be taken when the name parameter is None. The path searching for a given def.name() should only be taken when the parameter is Some.
Or expressed as test cases:
- With
query A { b },find_operation(Some("C"))should return None because we ask for a specific name that isn’t there - With
query D { e } query { f },find_operation(None)should return None because not specifying a name is ambiguous when there are multiple definitions.
|
By the way, as far as I can tell that second document does not raise a validation error for Operation Name Uniqueness nor for Lone Anonymous Operation, but that anonymous operation is impossible to execute! |
|
well this has been a bad week!!
I don't think so. It's an |
Mimics spec's
getOperationfunctionality and returns the anonymous operation ifNoneis specified for operation name.