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.

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

Use apollo-compiler to resolve type of selections in query parsing #2710

Closed
2 tasks done
Tracked by #2483
SimonSapin opened this issue Mar 3, 2023 · 0 comments · Fixed by #4038
Closed
2 tasks done
Tracked by #2483

Use apollo-compiler to resolve type of selections in query parsing #2710

SimonSapin opened this issue Mar 3, 2023 · 0 comments · Fixed by #4038
Assignees

Comments

@SimonSapin
Copy link
Contributor

SimonSapin commented Mar 3, 2023

Part of #2483

hir::Field::ty could replace this logic:

let field_type = match field.name() {
TYPENAME => FieldType::String,
"__schema" => FieldType::Introspection("__Schema".to_string()),
"__type" => FieldType::Introspection("__Type".to_string()),
field_name => {
let name = current_type
.inner_type_name()
.ok_or_else(|| SpecError::InvalidType(current_type.to_string()))?;
//looking into object types
schema
.object_types
.get(name)
.and_then(|ty| ty.field(field_name))
// otherwise, it might be an interface
.or_else(|| {
schema
.interfaces
.get(name)
.and_then(|ty| ty.field(field_name))
})
.ok_or_else(|| {
SpecError::InvalidField(
field_name.to_owned(),
current_type.to_string(),
)
})?
.clone()
}
};

I have a local branch doing so, but some tests still fail when either introspection or type extensions are involved. This needs changes in apollo-compiler.

Tasks

Preview Give feedback
  1. 4 of 4
    apollo-compiler
    SimonSapin
  2. apollo-compiler triage
    lrlna
@SimonSapin SimonSapin self-assigned this Mar 3, 2023
SimonSapin added a commit to apollographql/apollo-rs that referenced this issue Mar 24, 2023
SimonSapin added a commit to apollographql/apollo-rs that referenced this issue Mar 24, 2023
lrlna pushed a commit to apollographql/apollo-rs that referenced this issue Mar 24, 2023
…xtensions (#492)

This type resolution in apollographql/router#2710
was the motivation to start work on
#468
SimonSapin added a commit that referenced this issue Oct 20, 2023
Fixes #2710

# apollo-compiler background

The public API of apollo-compiler 1.0 is almost completely different
from 0.x. Notably it doesn’t have compiler objects anymore. Instead,
users mainly manipulate:

* `apollo_compiler::ast::Document` is the result of parsing one input
file/string. May contain parse errors. Finding anything requires a
linear scan of a `Vec` of top-level definitions.
* `apollo_compiler::Schema` has stuff indexed by name, with GraphQL
extensions "applied". Conversion from AST may record "build errors"
(e.g. for a name collision). Stuff with errors (e.g. that second
definition with the same name) may be missing in the main data
structure.
* `apollo_compiler::ExecutableDocument` similarly has operations and
fragments indexed by name, has build errors, and may be missing stuff
related to errors. Creating one requires `&Schema`, where it’ll find a
field definition to associate with every field selection, and resolve
the type of every selection set.

`Schema` and `ExecutableDocument` have `validate` methods to run full
GraphQL spec validation, but it needs to be called explicitly. A future
beta will likely change the API to make it less easy to forget dealing
with build errors or validation errors.

# This PR

Instead of creating and passing around a `Arc<Mutex<ApolloCompiler>>`
for every GraphQL request, the Router now creates a
`Arc<ParsedDocument>` which contains *both* an `ast::Document` and an
`ExecutableDocument`. In a later PR we’ll want to run Rust validation
early and have the rest of a request lifecycle only deal with
`ExecutableDocument`, but as long as we rely on TypeScript validation
some things still need to work at the AST level.

~~This PR starts as a draft because of remaining failing tests:~~

* `cargo test --lib`
- [x]
plugins::telemetry::metrics::apollo::test::apollo_metrics_validation_failure
  - [x] router::tests::schema_update_test
  - [x] spec::query::tests::reformat_response_data_best_effort
* `cargo test --test integration_tests`
  - [x] api_schema_hides_field
  - [x] defer_path_with_disabled_config
  - [x] validation_errors_from_rust

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [x] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [x] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant