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

[Feature] Tools: Schema Merge #1632

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

burn2delete
Copy link

This adds a tools command for manipulating schema files.

It also adds a tools merge subcommand to merge multiple .graphql/.gql files together into a single file. (implemented with apollo-rs 🎉 )

It outputs the merged schema as rover command result.

Example:

rover tools merge --schemas /path/containing/schemas

Copy link
Contributor

@lleadbet lleadbet left a comment

Choose a reason for hiding this comment

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

one comment, but code looks solid overall. might be worth adding a more robust test for the actual functionality, however.

let ast = parser.parse();
let doc = ast.document();

for def in doc.definitions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question- does apollo-parser automatically aggregate multiple definitions together? Meaning, if I have:

type User @key(fields:"id") {
id: ID!
name: String!
} 

and

type User @key(fields:"id") {
id: ID!
username: String!
} 

Would I get a combo of both, an error, or one of them?

Copy link
Author

Choose a reason for hiding this comment

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

A quick test suggests you will get both of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing, thanks for testing!

impl Merge {
pub fn run(&self) -> RoverResult<RoverOutput> {
// find files by extension
let schemas = self.find_files_by_extensions(self.options.schemas.clone(), &["graphql", "gql"])?;
Copy link
Member

@dariuszkuc dariuszkuc Jun 12, 2023

Choose a reason for hiding this comment

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

I think some folks also use .graphqls extension for schemas and .graphql for operations. Technically folks could define their schemas using any extension they want so unsure if there is a need to capture all of them (that being said .graphqls is somewhat common).

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch. might be worth allowing users to use a custom extension as a flag in that case

@smyrick
Copy link
Member

smyrick commented Jun 14, 2023

Maybe a basic question but this is just doing "dumb" schema merging, right? This doesn't do full Federation composition?

Or maybe a better question, what is the intended user use case? For someone working one a single subgraph or client that has the type defs spread across multiple files and would be valid if they all just lived in on file?

OR

Trying to do a merging of multiple schema files into one valid subgraph file, while doing some type and directive composition?

@burn2delete
Copy link
Author

burn2delete commented Jun 26, 2023

@smyrick

Maybe a basic question but this is just doing "dumb" schema merging, right? This doesn't do full Federation composition?

Thats correct, this just merges schema files. It's intentionally dumb - ie low-level file manipulation. But it reads and writes the schema files using apollo-rs parser/encoder.

Or maybe a better question, what is the intended user use case? For someone working one a single subgraph or client that has the type defs spread across multiple files and would be valid if they all just lived in on file?

Yes, the inputs should already be valid schema files - just split up into multiple files.

Trying to do a merging of multiple schema files into one valid subgraph file, while doing some type and directive composition?

That could be something we add as an enhancement, maybe throw warnings about it but not prevent merging of the files.

Ok(result)
}

fn merge_schemas_into_one(&self, schemas: Vec<PathBuf>) -> RoverResult<String> {
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but something to consider in the future is that we'd want to validate that all these files getting merged are semantically valid (with apollo-compiler). the compiler now has multi-file support, so you can just add them all to a compiler instance and validate.

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.

5 participants