Implement schema merging · Pull Request #1443 · graphprotocol/graph-node

ghost · 2020-01-10T16:20:20Z

Jannis · 2020-01-15T14:07:11Z

graph/src/data/schema.rs

-    let document = graphql_parser::parse_schema(ROOT_SCHEMA).expect("Failed to parse root schema");
+    let document = graphql_parser::parse_schema(ROOT_SCHEMA).unwrap();


Why drop the error message here (and in the following tests)?

Jannis · 2020-01-15T14:10:45Z

graph/src/data/subgraph/mod.rs

 pub enum SubgraphManifestValidationWarning {
    #[fail(display = "schema validation produced warnings: {:?}", _0)]
-    SchemaValidationWarning(SchemaImportError),
+    SchemaImportError(SchemaImportError),


We could just call this variant SchemaImport because it's already in the ...Warning enum.

Jannis · 2020-01-15T14:13:52Z

graphql/src/execution/execution.rs

+        // TODO: Does anything guarantee that the introspection and subgraph query fields
+        // do not overlap?


That would have to be checked during validation. What it needs to ensure is that no imported types (or type aliases) collide with local types / interfaces / enums. If that is the case then there will be no conflicts in the merged schema / query fields and introspection.

Jannis · 2020-01-15T14:20:49Z

graphql/src/schema/merge.rs

+    // If the schema is not available, then add a placeholder type to the root schema.
+    //
+    // If the schema is available, copy the type over.
+    // Check each field in the copied type and for non scalar fields, produce an (ImportedType, SchemaRefernce)


Typo: SchemaRefernce -> SchemaReference

Jannis · 2020-01-15T14:30:55Z

graphql/src/schema/merge.rs

+                    .find(|definition| {
+                        if let Definition::TypeDefinition(TypeDefinition::Object(obj)) = definition
+                        {
+                            obj.name.eq(&original_name)
+                        } else {
+                            false
+                        }
+                    })
+                    .map(|definition| match definition {
+                        Definition::TypeDefinition(TypeDefinition::Object(obj)) => obj,
+                        _ => unreachable!(),
+                    })


Hint hint: find_map will allow you to collapse these two combinators into one and only match the type definition kind once: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.find_map

Jannis · 2020-01-15T15:04:01Z

graphql/src/schema/merge.rs

+        let merged = merged_schema(&root_schema, schemas);
+
+        // Verify the output schema is correctl
+        match merged


You could make your life a whole lot easier if you just dump the merged GraphQL schema and compare it to an expected string. That'll also make it easier to understand the test. See https://docs.rs/graphql-parser/0.2.3/graphql_parser/#example-parse-and-format-schema for more info. It looks like given a GraphQL schema AST, dumping the schema is as simple as doing format!("{}", schema).

Jannis · 2020-01-15T15:04:18Z

graphql/src/schema/merge.rs

+        match merged.document.get_object_type_definitions().iter().next() {
+            None => panic!("Failed to import placeholder for type `A`"),
+            Some(type_a) => {
+                // Has an id field
+                match type_a.fields.iter().find(|field| field.name.eq("id")) {
+                    Some(_) => (),
+                    _ => panic!("Placeholder for imported type does not have the correct fields"),
+                };
+
+                // Has a placeholder directive
+                match type_a
+                    .directives
+                    .iter()
+                    .find(|directive| directive.name.eq("placeholder"))
+                {
+                    Some(_) => (),
+                    _ => {
+                        panic!("Imported type `A` does not have a `@placeholder` directive");
+                    }
+                }
+            }
+        };


Same here (compare schema strings)

Same in all other tests in this file. 😉

Jannis · 2020-01-15T18:25:08Z

store/postgres/src/store.rs

+            // Cache entry is stale and an unstable import has changed or is now available
+            // TODO: Is it okay to ignore unstable imports which are no longer available?
+            // TODO: Make this threshold an envvar?
+            let requires_refresh = entry.last_refresh.elapsed().as_secs() >= 120
+                && entry
+                    .unstable_imports
+                    .iter()
+                    .any(|(schema_reference, subgraph_id_opt)| {
+                        self.resolve_schema_reference(schema_reference)
+                            .map(|schema| match subgraph_id_opt {
+                                Some(subgraph_id) => schema.id.eq(subgraph_id),
+                                None => true,
+                            })
+                            .unwrap_or(false)
+                    });


Don't we also have to check the previously available schemas that were imported? Some of them might be gone now.

Jannis · 2020-01-15T18:26:43Z

store/postgres/src/store.rs

+        if let Some(entry) = self.schema_cache.lock().unwrap().get(&subgraph_id) {
+            // Cache entry is stale and an unstable import has changed or is now available
+            // TODO: Is it okay to ignore unstable imports which are no longer available?
+            // TODO: Make this threshold an envvar?


Yes, an env var sounds good.

Jannis · 2020-01-15T18:28:08Z

store/postgres/src/store.rs

+            .filter_map(|(schema_reference, schema)| match schema_reference {
+                SchemaReference::ByName(_) => {
+                    Some((schema_reference.clone(), Some(schema.id.clone())))
+                }
+                _ => None,
+            })


Aren't references by ID also unstable, as they may or may not be available all the time?

Jorge Olivero added 30 commits January 14, 2020 11:22

graph: Add UnvalidatedSubgraphManifest

2b6ab1e

core, graph: Update subgraph manifest resolution

719bba7

graph: Add SchemaReference enum and remove Schema method

cfa183a

schema: Add imported_schemas method to Schema

ebae6f0

core, graph: SchemaReferences are being resolved

f70804a

core, graph: Validation of imported subgraphs

01f55e1

graph: Validate Schema fields

408cfd6

core, graph: Address warnings

1eace39

core, graph: Move resolve_schema_references to method on Schema

a88e04f

core: Update combinator order subgraph deploy

82e734e

graph: Update schema type name const

5acf36e

graph: Simplify combinators

8c43beb

graph: Update comments

7ceffc2

graph: Remove comment and add placeholder method

add1bd7

graph: Remove extra method on UnvalidateSubgraphManifest

f3cf585

graph: Detect cycles in graph when resolving imports

5d7dfa3

schema: Better validation for import directives

8b53793

graph: Validate imported types exists in schema

6d0fe09

graph: Update SchemaReference type

8eb4abe

graph: Validated subgraph name and id in imports directives

8f75eac

graph: Migrate the tests to schema

be3491a

graph: Update assertions on test

3b3bcda

graph: Update fail string

ba3a328

graph: Edit fail string

1453a89

graph: Remove superfluous comments

59ba60b

graph: Update comments

7d1025e

graph: Add validation tests for imported schemas

3eb1731

graph: Update tests for validating imported types

ea5f178

graph: Rename method on Schema

2219852

graph: Shorten validation with if let

509f225

Jorge Olivero added 11 commits January 14, 2020 11:22

graph: Address naming and msg feedback

8fcfabb

graph: Remove TODO and make tuple values private

e4f75da

graph: Address comments about errors and hashing

8d02fb3

graph: Address schema.rs comment from review

9fb0afd

graph: Overload variable names

72552c3

graph: Address validation comments

24a2483

graph: Better use of combinators

67f5c56

graph: Leaner pattern matching

8713b29

graph: Better pattern matching

214d44b

graph: Address feedback on collecting errors

a308077

graph: Tests passing

17f5d84

ghost force-pushed the subgraph-composition branch from 50fe5fb to 17f5d84 Compare January 14, 2020 17:26

Jorge Olivero added 13 commits January 14, 2020 11:27

graphql: Initial implementation of merged_schema

8976ca0

graphql: Add placeholder type in case where schema is not found

c612581

graphql: Import nonscalar fields of imported types, add tests cases

e62eafd

graphql: Remove unused import

cd83cd8

graph, graphql: Add tests for merged_schema

fb4a302

graphql: Define @originalname directive, update comments

5950601

graph, graphql, mock, store: Integrate merged_schema with Store

d6f08a0

graphql: Remove extra import

47efb3a

graph, mock, store: Use SchemaImportError

88e0f4c

store: Implement cache invalidation

d24ab6a

store: Update comment

94a9577

graphql, server: Remove redundant code

4f0436b

mock: Ensure Store is correctly mocked

2b12994

ghost force-pushed the merged-schema branch from 5c92afd to 2b12994 Compare January 14, 2020 17:45

Jannis suggested changes Jan 15, 2020

View reviewed changes

ghost force-pushed the subgraph-composition branch from 4e04f73 to f389c99 Compare February 10, 2020 19:03

ghost closed this Feb 13, 2020

ghost mentioned this pull request Feb 13, 2020

Merged schema #1492

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement schema merging#1443

Implement schema merging#1443
ghost wants to merge 64 commits intosubgraph-compositionfrom
merged-schema

ghost commented Jan 10, 2020 •

edited by ghost

Loading

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Jannis Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		let document = graphql_parser::parse_schema(ROOT_SCHEMA).expect("Failed to parse root schema");
		let document = graphql_parser::parse_schema(ROOT_SCHEMA).unwrap();

		// TODO: Does anything guarantee that the introspection and subgraph query fields
		// do not overlap?

Conversation

ghost commented Jan 10, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost commented Jan 10, 2020 •

edited by ghost

Loading