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

Override field types option for mergeSchemas #2051

Open
Tracked by #5201 ...
FluorescentHallucinogen opened this issue Sep 21, 2020 · 20 comments
Open
Tracked by #5201 ...

Override field types option for mergeSchemas #2051

FluorescentHallucinogen opened this issue Sep 21, 2020 · 20 comments
Labels
feature New addition or enhancement to existing solutions

Comments

@FluorescentHallucinogen

I'm working on a tool that shows a visual diff between two GraphQL schemas (e.g the previous and current versions of the same schema). Added fields are marked in green color, deleted in red and modified in yellow. It's built on top of graphql-voyager and uses @graphql-inspector/core under the hood.

In order to display the deleted fields in Voyager I merge both schemas using mergeSchemas from @graphql-tools/merge.

graphql-voyager-visual-diff

The problem is that if e.g. Post.content in schemaA (the previous version of the schema) has the String! (non-null) type, but Post.content in schemaB (the current version of the schema) has the String (nullable) type, the Post.content in the result of mergeSchemas of [schemaA, schemaB] (in that order) will have the String! type (from the previous schemaA), not the String type (from the current schemaB). See the image below.

Could you please add an option for mergeSchemas that changes the behavior of merging non-null and nullable fields to use (override) the nullability from the latest schema from the array?

@FluorescentHallucinogen
Copy link
Author

Also, if the field changes the type from e.g. Int to String, I get the following error: Error: Unable to merge GraphQL type "Post": Field "test" already defined with a different type. Declared as "Int", but you tried to override with "String". How to force override?

@ardatan
Copy link
Owner

ardatan commented Sep 26, 2020

You can try new onFieldTypeConflict option on the canary version of this PR;
#2065

In your case, you can overwrite the existing FieldDefinitionNode with the next one however you want.

@ardatan ardatan added the feature New addition or enhancement to existing solutions label Sep 26, 2020
@FluorescentHallucinogen
Copy link
Author

@ardatan

Is it incorrect, because type boolean is not assignable to type OnFieldTypeConflict?

you can overwrite the existing FieldDefinitionNode with the next one

But how? :-/ I've tried onFieldTypeConflict: (existingField, otherField) => { existingField = otherField; }, but it doesn't work.

Could you please provide an example of onFieldTypeConflict that overwrites the existingField by the the otherField?

It looks like it's a good idea to add this example into onFieldTypeConflict JSDoc.

@ardatan
Copy link
Owner

ardatan commented Sep 27, 2020

@FluorescentHallucinogen

onFieldTypeConflict: (existingField, otherField) => {
   existingField.type = otherField.type;
}

@FluorescentHallucinogen
Copy link
Author

@ardatan
I get a Cannot assign to 'type' because it is a read-only property. ts(2540) error.

I've tried the following:

onFieldTypeConflict: (existingField, otherField) => {
  Object.keys(existingField).forEach((key) => {
    delete existingField[key];
  });
  Object.assign(existingField, otherField);
}

to mutate existingField by reference, and it works, but I'm curious, is there a more clean and elegant way to do this?

I'm worried about the DX. What about making onFieldTypeConflict immutable (onFieldTypeConflict should return result of conflict resolving)? E.g.: onFieldTypeConflict: (existingField, otherField) => { return otherField; } to overwrite the existing FieldDefinitionNode and onFieldTypeConflict: (existingField, otherField) => { return existingField; } to keep the original one?

@FluorescentHallucinogen
Copy link
Author

I've also found that onFieldTypeConflict is not called while merging e.g. String! and String.

@FluorescentHallucinogen
Copy link
Author

@ardatan Just found https://www.apollographql.com/docs/apollo-server/api/graphql-tools/#ontypeconflict. The third info argument looks very useful.

Just curios, why onTypeConflict was dropped in the new graphql-tools?

@ardatan
Copy link
Owner

ardatan commented Sep 27, 2020

Old mergeSchemas of graphql-tools v4/v5 is for schema stitching not schema merging so it's renamed to stitchSchemas in v6.
You can see; https://www.graphql-tools.com/docs/merge-schemas#difference-between-merging-and-stitching
So onTypeConflict is not dropped from mergeSchemas because new mergeSchemas comes from old graphql-toolkit. You can check migration notes to see the differences between v5 and v6.

You're right about DX so I changed the signature like you said. And now onFieldTypeConflict is called if there is a conflict about nullability.
onFieldTypeConflict: (existingField, otherField) => { return otherField; }

CI will release new canary version in a few minutes.

Let me know if it works.

@FluorescentHallucinogen
Copy link
Author

Let me know if it works.

onFieldTypeConflict: (existingField, otherField) => otherField works, but onFieldTypeConflict is still not called if there is a conflict about nullability.

@FluorescentHallucinogen
Copy link
Author

Just tested stitchSchemas from @graphql-tools/stitch and it surprisingly works exactly as I need even without onTypeConflict! :) It correctly overwrites e.g. String! to Int and String! to String.

@ardatan
Copy link
Owner

ardatan commented Sep 27, 2020

Tests have been added for your cases;
https://github.com/ardatan/graphql-tools/pull/2065/files#diff-91cd70a4ebe73358bd0b8c72c5b83b9e
It should work now. I am not sure overwriting String! with Int is a correct behavior because it breaks the previous schema.

@FluorescentHallucinogen
Copy link
Author

Just tested stitchSchemas from @graphql-tools/stitch and it surprisingly works exactly as I need

I was wrong. The stitchSchemas works strange.

left.graphql:

type Post {
  id: ID!
  title: String!
  content: String!
  published: Boolean!
}

type User {
  id: ID!
}

right.graphql

type Post {
  id: ID!
  title: String
  content: Int
  fieldA: Int
  fieldB: String
}

The

const schema = stitchSchemas({
  schemas: [leftSchema, rightSchema],
});

outputs:

type Post {
  id: ID!
  title: String
  content: Int
  fieldA: Int
  fieldB: String
}

type User {
  id: ID!
}

The

const schema = stitchSchemas({
  schemas: [leftSchema, rightSchema],
  onTypeConflict: (left, right) => right,
});

outputs:

type Post {
  id: ID!
  title: String
  content: Int
  fieldA: Int
  fieldB: String
}

type User {
  id: ID!
}

The

const schema = stitchSchemas({
  schemas: [leftSchema, rightSchema],
  onTypeConflict: (left, right) => left,
});

outputs:

type Post {
  id: ID!
  title: String!
  content: String!
  published: Boolean!
}

type User {
  id: ID!
}

I.e. stitchSchemas merges types (the output contains the type User), but doesn't merge fields in types (the output doesn't contain published field or fieldA and filedB fields), it just outputs the types from left or right "as is".

BTW, is it a bug or by design?

The mergeSchemas from the latest version 6.2.4-alpha-570022cf.0 works correctly:

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => right,
});

outputs:

type Post {
  id: ID!
  title: String
  content: Int
  published: Boolean!
  fieldA: Int
  fieldB: String
}

type User {
  id: ID!
}

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => left,
});

outputs:

type Post {
  id: ID!
  title: String!
  content: String!
  published: Boolean!
  fieldA: Int
  fieldB: String
}

type User {
  id: ID!
}

The only thing I've found, it doesn't merge arguments:

left.graphql:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
  ): Boolean!
}

right.graphql:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int
    argC: Int!
    argD: String
    argF: Boolean
  ): Boolean!
}

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => right,
});

outputs:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int
    argC: Int!
    argD: String
    argF: Boolean
  ): Boolean!
}

I've expected:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int
    argC: Int!
    argD: String
    argE: Int!
    argF: Boolean
  ): Boolean!
}

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => left,
});

outputs:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
  ): Boolean!
}

I've expected:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
    argF: Boolean
  ): Boolean!
}

@ardatan
Copy link
Owner

ardatan commented Sep 28, 2020

I pushed a fix for arguments(I thought you need to override field completely) so you can try after CI releases the new canary version.

@FluorescentHallucinogen
Copy link
Author

Also, it seems I've found a bug in stitchSchemas: onTypeConflict works for any types, except Query, Mutation and Subscription.

left.graphql:

type Test {
  fieldA: Int
}

right.graphql:

type Test {
  fieldA: String
}

The

const schema = stitchSchemas({
  schemas: [leftSchema, rightSchema],
  onTypeConflict: (left, right) => right,
});

outputs:

type Test {
  fieldA: String
}

The

const schema = stitchSchemas({
  schemas: [leftSchema, rightSchema],
  onTypeConflict: (left, right) => left,
});

outputs:

type Test {
  fieldA: Int
}

But for Query, Mutation and Subscription the behavior for some reason is different:

left.graphql:

type Query {
  fieldA: Int
}

right.graphql:

type Query {
  fieldA: String
}

the both onTypeConflict: (left, right) => right and onTypeConflict: (left, right) => left

outputs:

Error: Unable to merge GraphQL type "Query": Field "fieldA" already defined with a different type. Declared as "Int", but you tried to override with "String"

@ardatan
Copy link
Owner

ardatan commented Sep 28, 2020

Ok but this is a different issue :) Schema stitching works in a different approach and aims a different thing. It might not be a good idea to overwrite Query like that because onTypeConflict overwrites the type definition completely. If mergeSchemas works for you in that way, I'd recommend to go with it.

@FluorescentHallucinogen
Copy link
Author

Just tested merging arguments in 6.2.4-alpha-34c1194a.0 and it works not as I expected:

left.graphql:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
  ): Boolean!
}

right.graphql:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int
    argC: Int!
    argD: String
    argF: Boolean
  ): Boolean!
}

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => right,
});

outputs:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
    argF: Boolean
  ): Boolean!
}

I've expected:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int
    argC: Int!
    argD: String
    argE: Int!
    argF: Boolean
  ): Boolean!
}

The

const schema = mergeSchemas({
  schemas: [leftSchema, rightSchema],
  onFieldTypeConflict: (left, right) => left,
});

outputs:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
    argF: Boolean
  ): Boolean!
}

I've expected:

type Mutation {
  doSomething(
    argA: Int!
    argB: Int!
    argC: Int
    argD: Int!
    argE: Int!
    argF: Boolean
  ): Boolean!
}

I.e. for onFieldTypeConflict: (left, right) => left (the 2nd test case in this comment) it works correct. But for onFieldTypeConflict: (left, right) => right it works incorrect (the 1st test case in this comment).

@simplecommerce
Copy link
Contributor

@FluorescentHallucinogen Hi, I know this has been a long time, but have you found a solution to your issue here? I was curious to know what alternative solution you used. I have a similar need as you described here and I am trying to find a solution.

@shellscape
Copy link

@ardatan onFieldTypeConflict was added in 8.4.0 (

- [#5075](https://github.com/ardatan/graphql-tools/pull/5075) [`04e3ecb9`](https://github.com/ardatan/graphql-tools/commit/04e3ecb9c377834984ab0a272add29a1709e305d) Thanks [@simplecommerce](https://github.com/simplecommerce)! - Added onFieldTypeConflict option to handle manual conflicts for mergeTypeDef and reverseArguments option to select left side arguments if specified.
) but mergeSchemas was not updated to support this. That was a rather large miss, since mergeSchemas provides no mechanism to resolve conflicts.

@ardatan
Copy link
Owner

ardatan commented Jul 10, 2023

PRs are welcome! @shellscape

@shellscape
Copy link

shellscape commented Jul 10, 2023

@ardatan I'll see what I can do tonight. Please be aware that I have very limited time (as I'm sure you do as well) so please do try to help keep friction low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

No branches or pull requests

4 participants