feat(compiler): Introduce ExecutableDocumentBuilder for operations with multiple sources#1017
feat(compiler): Introduce ExecutableDocumentBuilder for operations with multiple sources#1017trevor-scheer wants to merge 9 commits intoapollographql:mainfrom
ExecutableDocumentBuilder for operations with multiple sources#1017Conversation
Add ExecutableDocumentBuilder following the same pattern as SchemaBuilder,
enabling users to build executable documents from multiple source files.
Key features:
- Build executable documents from multiple GraphQL files
- Proper collision detection for operations and fragments across files
- Optional schema validation during building
- Source tracking for all definitions
- Backwards compatible with existing document_from_ast function
API additions:
- ExecutableDocumentBuilder struct with new(), add_ast_document(), and build() methods
- ExecutableDocument::builder() convenience method
- Parser::parse_into_executable_builder() for multi-file parsing
Example usage:
```rust
let mut builder = ExecutableDocument::builder(Some(&schema));
Parser::new().parse_into_executable_builder(
Some(&schema),
"query GetUser { user { id } }",
"query1.graphql",
&mut builder,
);
Parser::new().parse_into_executable_builder(
Some(&schema),
"query GetPost { post { title } }",
"query2.graphql",
&mut builder,
);
let document = builder.build().unwrap();
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Add 8 new tests covering: - Building documents from multiple files - Combining queries and fragments from separate files - Operation name collision detection across files - Fragment name collision detection across files - Building without a schema (validation-free mode) - Source information preservation - Anonymous/named operation mixing (error case) - Multiple fragments in single query All tests pass successfully, verifying the builder handles multi-file scenarios correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
e830b14 to
04b9622
Compare
tninesling
left a comment
There was a problem hiding this comment.
This is looking good, I just have a few suggestions.
| pub fn new(schema: Option<&Schema>) -> Self { | ||
| Self { | ||
| document: ExecutableDocument::new(), | ||
| schema: schema.cloned(), |
There was a problem hiding this comment.
It looks like the APIs where this is used take a reference to the schema, so we should be able to avoid cloning this. Avoiding the clone is particularly important when working with very large schemas. Can you update the builder to hold a Option<&Schema> instead of an owned one?
There was a problem hiding this comment.
Yep! Just noting this adds a lifetime to the builder but agree this is preferable.
f71a00e
| match definition { | ||
| ast::Definition::OperationDefinition(operation) => { | ||
| if let Some(name) = &operation.name { | ||
| // Named operation |
There was a problem hiding this comment.
nit: There are a lot of comments like "Named operation" or "overwritten" that aren't totally clear. In this case, I feels it's confusing to label this "Named operation" when we're specifically looking for an anonymous operation. It might be worth clearing up those comments.
There was a problem hiding this comment.
A few of these are stray adds, a few are preserved from the lifted and shifted document_from_ast fn. I've gone through and preserved what was there before and tidied up the other bits. Happy to revisit and clean up the rest if you like, this pass was to minimize the footprint.
6da8915
| let mut builder = ExecutableDocumentBuilder { | ||
| document: ExecutableDocument::new(), | ||
| schema: schema.cloned(), | ||
| errors: std::mem::replace(errors, DiagnosticList::new(Default::default())), |
There was a problem hiding this comment.
This pattern of replacing the input with default and then writing back to the mutable reference with the same data seems odd to me. It seems like it would make more sense to let the builder borrow the mutable reference and append to the diagnostic list as necessary.
There was a problem hiding this comment.
Yeah, this could be a take instead if that sits better (I think it's the same as what's written though).
I took your suggestion and made this a mutable reference though. The API for the builder is a bit less nice but I don't feel strongly either way. Let me know what you think! 8296091
crates/apollo-compiler/src/parser.rs
Outdated
| /// [`ExecutableDocumentBuilder::build`]. | ||
| pub fn parse_into_executable_builder( | ||
| &mut self, | ||
| _schema: Option<&Valid<Schema>>, |
There was a problem hiding this comment.
Was this parameter intended to be used? If not, we can remove this, since it's a net-new API.
There was a problem hiding this comment.
Yeah this ended up in the builder and unneeded 👍
95cb501
| let query2 = "query GetPost { post { id title } }"; | ||
|
|
||
| let mut builder = ExecutableDocument::builder(Some(&schema)); | ||
| Parser::new().parse_into_executable_builder( |
There was a problem hiding this comment.
It feels like we're missing an ExecutableDocumentBuilder::parse() function equivalent to the one on SchemaBuilder. The SchemaBuilder one just delegates to the equivalent Parser::parse_into_schema_builder(). I think that would be a more fluent interface so the end-user doesn't have to construct these ephemeral Parser instances themselves.
| } | ||
|
|
||
| #[test] | ||
| fn builder_detects_operation_name_collision() { |
There was a problem hiding this comment.
Could you add another test which asserts this properly collects diagnostics from multiple sources? I want to make sure that diagnostic rewriting logic doesn't accidentally wipe previous diagnostics.
|
Thanks for the speedy review @tninesling! And nice to see ya, so to speak 😄 |
|
@tninesling I spotted this issue comment from @lrlna this morning - we should see if they'd prefer this not land. |
tninesling
left a comment
There was a problem hiding this comment.
I think this is a nice ergonomic improvement to the API, and the changes look good with the recent adjustments. I don't think @lrlna has had a chance to take a detailed look, and it looks like they're out until next week. Feel free to add them as a reviewer if you're looking for their specific feedback, but from my perspective, this is good to go.
|
@tninesling Ultimately your call. I'm happy for this to land but also I'm completely unblocked on using this, just building against my branch for now. I'd err on hearing their input in case they had any particular hesitations or alternatives in mind! |
|
@tninesling i don't have the perms to add reviewers, would you mind adding them, or bring this to their attention? A happy middle ground option: introduce this API as experimental/feature-gated until @lrlna has the opportunity to provide their feedback. I'm still in no rush, but just don't want this to go by the wayside forever either. Thanks! |
Note
This PR was almost entirely written by Claude. I've reviewed this extensively myself and included the prompt and implementation plan for posterity.
Details
Prompt
@crates/apollo-compiler/src/executable/from_ast.rs @crates/apollo-compiler/src/schema/from_ast.rs
Create a plan and write to local
.claudefolder. ExecutableDocument and Schema are quite similar. I need a builder for ExecutableDocuments consisting of multiple sources. Schema already has this concept of schema builder, and you can parse schema AST into a schema builder. I want to be able to parse executable AST into an executable document builder.ExecutableDocumentBuilder Implementation Plan
Context
Currently,
ExecutableDocumentis created directly from a single AST document via thedocument_from_astfunction incrates/apollo-compiler/src/executable/from_ast.rs. However,Schemahas a more sophisticated builder pattern (SchemaBuilderincrates/apollo-compiler/src/schema/from_ast.rs) that allows building a schema from multiple source files incrementally.We need to implement an
ExecutableDocumentBuilderthat mirrors theSchemaBuilderpattern, allowing users to:Key Design Parallels with SchemaBuilder
SchemaBuilder Structure (lines 7-15 of schema/from_ast.rs)
Proposed ExecutableDocumentBuilder Structure
Implementation Steps
1. Create ExecutableDocumentBuilder Struct
File:
crates/apollo-compiler/src/executable/from_ast.rsAdd the builder struct at the beginning of the file:
ExecutableDocumentSchemareference for validation during buildingDiagnosticListadopt_orphan_extensionsandignore_builtin_redefinitions)2. Implement Builder Constructor and Configuration
File:
crates/apollo-compiler/src/executable/from_ast.rsAdd methods:
new(schema: Option<&Schema>) -> Self- Create a new builder with optional schemaSchemaBuilder::new()which clones built-in types (lines 69-71 of schema/from_ast.rs)3. Refactor
document_from_astto Use Builder PatternFile:
crates/apollo-compiler/src/executable/from_ast.rsCurrent function signature (line 9):
Refactor to:
ExecutableDocumentBuilder::add_ast_document(&mut self, document: &ast::Document, type_system_definitions_are_errors: bool)add_ast_documentmethodKey differences from current implementation:
ExecutableDocumentimmediately, accumulate intoself.documentself.document.operationsself.document.fragmentsself.document.sources4. Implement Core Builder Methods
File:
crates/apollo-compiler/src/executable/from_ast.rsAdd methods mirroring
SchemaBuilder:a)
add_ast_document(similar to lines 106-269 of schema/from_ast.rs)&ast::Documentand process all executable definitionsself.document.operations:operations.namedoperations.anonymousself.document.fragmentstype_system_definitions_are_errorsis truedocument.sourcesintoself.document.sourcesb)
build(similar to lines 277-280 of schema/from_ast.rs)build_inner()and convert errors to resultc)
build_inner(similar to lines 282-349 of schema/from_ast.rs)5. Update ExecutableDocument API
File:
crates/apollo-compiler/src/executable/mod.rsAdd a
builder()method (similar to lines 439-441 of schema/mod.rs):Consider deprecating or updating the existing
parsemethod to internally use the builder.6. Update Parser Integration
File:
crates/apollo-compiler/src/parser.rsAdd new method similar to
parse_into_schema_builder:Update
parse_executable_inner(lines 261-271) to optionally use the builder pattern.7. Export Builder in Public API
File:
crates/apollo-compiler/src/executable/mod.rsAdd to the public exports (around line 71):
8. Add Documentation
Add comprehensive documentation for:
ExecutableDocumentBuilderstructdocument_from_astto builder pattern9. Update Tests
File: Create
crates/apollo-compiler/src/executable/from_ast_tests.rsor update existing testsAdd tests for:
Key Differences from SchemaBuilder
Migration Path
For backwards compatibility:
document_from_astas a public APIExecutableDocumentBuilderBenefits
Example Usage
Files to Modify
crates/apollo-compiler/src/executable/from_ast.rs- Main implementationcrates/apollo-compiler/src/executable/mod.rs- Public API and exportscrates/apollo-compiler/src/parser.rs- Parser integrationEstimated Complexity
In frontend projects, operation documents are commonly created from multiple sources. Fragment definitions are often treated as global and stitched in by clients.
Example:
Fragment def
Usage
In order to build tooling that accommodates this common use case, we should be able to build
ExecutableDocuments from multiple sources. Fortunately, we already accommodate a parallel use case for schema via theSchemaBuilder, so this implementation leans heavily on that pattern and prior art.This change largely extracts the functionality out of
document_from_astand intoExecutableDocumentBuilder.