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

Add schema introspection support and execution-related APIs #758

Merged
merged 18 commits into from
Dec 11, 2023

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Nov 23, 2023

Changelog:

  • Add data structure in apollo_compiler::execution for a GraphQL response, its data, and errors.
    All (de)serializable with serde.
  • Add coerce_variable_values() in that same module.
  • Add apollo_compiler::execution::SchemaIntrospection
    providing full execution for the schema introspection parts of an operation
    and separating the rest to be executed separately.
    In order to support all kinds of introspection queries this actually includes
    a full execution engine where users provide objects with resolvable fields.
    At this time this engine is not exposed in the public API.
    If you’re interested in it let us know about your use case!
  • Add ExecutableDocument::insert_operation convenience method.

Replaces and closes #645:

  • The execution engine and introspection resolvers are copied from there. async was removed (since unused by introspection, the only user)
  • The public API was reworked to be part of apollo-compiler
  • The logic splitting an operation into introspection and non-introspection parts was rewritten two produce valid documents. This logic needs a lot more testing.

@SimonSapin
Copy link
Contributor Author

Converted to draft for:

  • Error representation. Currently RequestError contains a GraphQLError with line and column numbers. Maybe it should instead be something that can convert to GraphQLError.
  • Handling of schema introspection fields not at the top level is probably broken. I think we should make them invalid.

https://spec.graphql.org/October2021/#sec-Schema-Introspection

The schema introspection system is accessible from the meta-fields __schema and __type which are accessible from the type of the root of a query operation.

This could say from the root of a query, but it says from the type of the root of a query, which suggests this is valid:

schema { query: Fibonacci }
type Fibonacci { value: Int, next: Fibonacci }

query { value next { value next { __schema { description }}}}

I suspect this is not intended.

SimonSapin added a commit that referenced this pull request Nov 30, 2023
Extracted from #758

Co-authored-by: Renée <renee@kooi.me>
@SimonSapin
Copy link
Contributor Author

I pushed a rework of error handling. introspection_split.rs still needs work to reject non-top-level introspection, but the rest of the PR can be considered ready for review.

Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

We should probably merge this in without making too many more changes, and then incrementally improve after we get a chance to try it out in the router (like you said before, Simon).

Ideally we'd try integrating it in the router, see if our users have any feedback, implement tests from graphql-js implementations and make this API a 1.0 with the compiler. But yea, regardless, let's get this in.

crates/apollo-compiler/CHANGELOG.md Show resolved Hide resolved
use expect_test::expect;
use expect_test::expect_file;

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

sometime later we should port over the graphql-js introspection & execution tests

goto-bus-stop added a commit that referenced this pull request Dec 8, 2023
Using `GraphQLLocation::from_node` from #758

We could also have an optional `ToDiagnostic::location()` method and a
provided `ToDiagnostic::to_json(&SourceMap)` to let people do this
easily with their own errors. It's probably worth having now as we
will also have JSON error types in introspection
@SimonSapin
Copy link
Contributor Author

Alright, this is now fully ready for review! Please also take a look at introspection_split.rs and its tests.

@lrlna
Copy link
Member

lrlna commented Dec 11, 2023

I think you should merge, @SimonSapin ^__^

@SimonSapin SimonSapin marked this pull request as ready for review December 11, 2023 15:23
@SimonSapin SimonSapin enabled auto-merge (squash) December 11, 2023 15:25
@SimonSapin SimonSapin merged commit d97abbc into main Dec 11, 2023
10 of 11 checks passed
@SimonSapin SimonSapin deleted the introspection2 branch December 11, 2023 15:26
@lrlna lrlna mentioned this pull request Dec 11, 2023
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 this pull request may close these issues.

2 participants