xuorig / graphql-schema_comparator Goto Github PK

Overview

If an input type has a field that is non-null and that field is changing to a nullable field, we should not treat that as a breaking change but the gem currently does. It's not a breaking since any clients would already be supplying the input field, with the change they have the opportunity to not do that anymore.

just a note to myself

Since latest ruby-graphql release comparator generates warnings:

Legacy `.to_graphql` objects are deprecated and will be removed in GraphQL-Ruby 2.0. Remove `.to_graphql` to use a class-based definition instead.

graphql-js has the concept of both breaking and dangerous changes. This might be a useful distinction to have here as well?

export const BreakingChangeType = {
  FIELD_CHANGED_KIND: 'FIELD_CHANGED_KIND',
  FIELD_REMOVED: 'FIELD_REMOVED',
  TYPE_CHANGED_KIND: 'TYPE_CHANGED_KIND',
  TYPE_REMOVED: 'TYPE_REMOVED',
  TYPE_REMOVED_FROM_UNION: 'TYPE_REMOVED_FROM_UNION',
  VALUE_REMOVED_FROM_ENUM: 'VALUE_REMOVED_FROM_ENUM',
  ARG_REMOVED: 'ARG_REMOVED',
  ARG_CHANGED_KIND: 'ARG_CHANGED_KIND',
  NON_NULL_ARG_ADDED: 'NON_NULL_ARG_ADDED',
  NON_NULL_INPUT_FIELD_ADDED: 'NON_NULL_INPUT_FIELD_ADDED',
  INTERFACE_REMOVED_FROM_OBJECT: 'INTERFACE_REMOVED_FROM_OBJECT',
};

export const DangerousChangeType = {
  ARG_DEFAULT_VALUE_CHANGE: 'ARG_DEFAULT_VALUE_CHANGE',
  VALUE_ADDED_TO_ENUM: 'VALUE_ADDED_TO_ENUM',
  INTERFACE_ADDED_TO_OBJECT: 'INTERFACE_ADDED_TO_OBJECT',
  TYPE_ADDED_TO_UNION: 'TYPE_ADDED_TO_UNION',
};

"the type changed" is almost always a breaking change, except when going from an object to an interface with exactly the same set of fields (and implemented interfaces). It would be nice if schema_comparator recognized this case and didn't flag it as breaking.

Hi 👋

For the schema comparison:

Old Schema:

type Query {}

New schema:

type Query {}

type App {
  name(
    foo: String
  ): String
}

The current behaviour only triggers one change:

Changes::TypeAdded

But for linting purposes, it's important to have the full set of changes:

Changes::TypeAdded
Changes::FieldAdded
Changes::FieldArgumentAdded

I'm proposing an option to return the nested schema changes for new types and fields.

This can be done by comparing the new types and fields to empty versions of them. Comparing the new type with an empty one returns Changes::FieldAdded and comparing this field with an empty one returns Changes::FieldArgumentAdded.

An approach like this does not require modifying the Change classes. It can be done in Schema::Diff or by having a similar class that does this "recursive" schema comparison.

When comparing a query where the expected type has been extended to exactly match the actual type, the comparator is currently reporting a change.

Can someone confirm if this is correct? I would have expected the two queries to show no changes.

Expected:

type Query {
}

extend type Query {
   test(id: String!): String!
}

Actual:

type Query {	
   test(id: String!): String!
}

Output:
`⏳ Checking for changes...
🎉 Done! Result:

Detected the following changes between schemas:

✅ Field test was added to object type Query
`

Some changes to nullability are being reported as breaking when they aren't.

Base schema:

schema {
  query: Query
}

type Query {
  number(plus: Int!): Int
}

Comparison are done using this gem and https://www.npmjs.com/package/@entria/graphql-findbreakingchanges which uses graphql-js under the hood (it's just a CLI wrapper).

Argument type made optional:

schema {
  query: Query
}

type Query {
  number(plus: Int): Int
}

gem:

⚠️ Type for argument plus on field Query.number changed from Int! to Int

graphql-js:

Congratulations! NO BREAKING CHANGES

Field type made non-null:

schema {
  query: Query
}

type Query {
  number(plus: Int): Int!
}

gem:

⚠️ Field Query.number changed type from Int to Int!

graphql-js:

Congratulations! NO BREAKING CHANGES

v1.1.1
graphql-ruby v2.0.8

@swalkinshaw

Possibly a duplicate/confirmation of #35? cc @RobertWSaunders @jturkel

In the following test case, the change from bar: Bar! to bar: Bar on an input type gets marked breaking incorrectly:

require 'graphql/schema_comparator'

old_schema_idl = <<~SCHEMA
  schema {
    query: Query
  }
  input Foo {
    bar: Bar!
  }
  input Bar {
    x: String
  }
  type Query {
    a(foo: Foo!): String!
  }
SCHEMA

new_schema_idl = <<~SCHEMA
  schema {
    query: Query
  }
  input Foo {
    bar: Bar
  }
  input Bar {
    x: String
  }
  type Query {
    a(foo: Foo!): String!
  }
SCHEMA

result = GraphQL::SchemaComparator.compare(old_schema_idl, new_schema_idl)

puts result.changes.map(&:message)

puts result.breaking?

It looks like what's happening is that the parser is generating different classes for schema type Bar in the two schemas, so when the straight equality is run at

I'm not sure how this ever worked, or why we never ran into it at Shopify? Maybe the gem recently change how it parses string schemas to generate duplicate classes?

Background

Hi, we use the schema comparer library to output breaking change warnings as PR comments, as I'm sure many do. We do not use the rake task directly but rather take the output of GraphQL::SchemaComparator.compare and process it to our liking.

As we have some niche use case customisations, we have extensive testing of the output of our custom comparer.

Problem

In a routine upgrade from 1.0.0 to 1.1.1 (while remaining on GraphQL 1.12.10) while most tests still passed, we saw 2 failures:

The first is that removed arguments are missing their type:

Before: "Argument include_internal: Boolean! was removed from field Query.organisation"
After: "Argument include_internal: was removed from field Query.organisation"

The second is that when field description is changed on an interface which is implemented by another type, that type's name is no longer included in output, instead duplicating the interface's name:

Diff:04:08
@@ -1,7 +1,7 @@04:08
 ## Detected safe changes in your GraphQL schema for `/v1/graphql`04:08
 04:08
 Detected changes when compared against the staging schema @ 12/08/2021:04:08
-- ✅  Field `Organisation.visibleAccountsCount` description changed from `Number of accounts` to `Number of accounts on organisation`04:08
+- ✅  Field `OrganisationalUnit.visibleAccountsCount` description changed from `Number of accounts` to `Number of accounts on organisation`04:08
 - ✅  Field `OrganisationalUnit.visibleAccountsCount` description changed from `Number of accounts` to `Number of accounts on organisation`04:08

in this instance the relevant SDL is:

type Organisation implements OrganisationalUnit {}

interface OrganisationalUnit {
  """
  Number of accounts on organisation
  """
  visibleAccountsCount: Int
}

An unchanged directive is currently being marked as a breaking change. Example schema affected:

schema {
  query: QueryRoot
}

type QueryRoot {
  name(locale: String = "en"): String!
}

directive @colorFormat(
  colorFormat: ColorFormatEnum!
) on QUERY

enum ColorFormatEnum {
  HSL
  LCH
}

When using long descriptions, e.g.

value "width", "Resizes the image to given width (in pixels) and keeps proportions. Resizing the image bigger than the original is not possible.", value: :width

and dumping the schema, it looks like this

enum ImageOperatorFilter {
  """
  Resizes the image to given width (in pixels) and keeps proportions. Resizing
  the image bigger than the original is not possible.
  """
  width
}

because of the formatting in GraphQL::Language::BlockString, which adds a new line after a certain amount of characters.

Unfortunately, graphql-schema_comparator lists this new line as a change

Safe change:: Description for enum value `ImageOperatorFilter.width` changed from `Resizes the image to given width (in pixels) and keeps proportions. Resizing
the image bigger than the original is not possible.` to `Resizes the image to given width (in pixels) and keeps proportions.`

So now, you can't use graphql-schema_comparator any more because it always detects a change, even if there isn't one.

Would it be possible to ignore such new lines in descriptions? Or would it be better to not include newlines in the schema, and fix this on graphql-ruby gem side?

Hey @xuorig 👋 😄

Pulling the latest master, 79ad690, I'm getting some confusing failing tests:

https://github.com/xuorig/graphql-schema_comparator/blob/master/test/lib/graphql/schema_comparator_test.rb#L33-L46

-["Field `Query.a` changed type from `String!` to `Int`", "Field `b` was added to object type `Query`"]
+["Field `Query.a` changed type from `String!` to `Int`", "Field `b` was added to object type `Query`", "Type `Int` was added"]

It seems odd that Int, which is a built-in GraphQL scalar type would be considered a "new type" in any context. Inversely, the toy schema that no longer refers to String is not reported as dropping that type (I also wouldn't expect it to.)

In rmosolgo/graphql-ruby#2178 (graphql-ruby 1.10.0) was changed the .from_definition method which used in:

So we can't compare even the same schemas:

> new_schema = BinaazSchema.to_definition
> GraphQL::SchemaComparator.compare(new_schema, new_schema)

Detected the following changes between schemas:

🛑  Schema mutation root has changed from `#<Class:0x00007f51e8424c08>` to `#<Class:0x00005616c7bfaf98>`
red
🛑  Schema query root has changed from `` to ``
red

Hi @xuorig !!

First thanks you for this project!

Now I try compare a more complex schemas and returns this error:

graphql-schema compare gw20171103.graphql gw20171108.graphql 
🤖  Checking for changes...
/home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/enum.rb:23:in `block in diff': no implicit conversion of GraphQL::SchemaComparator::Changes::EnumValueDescriptionChanged into Array (TypeError)
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/enum.rb:44:in `block in each_common_value'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/enum.rb:40:in `each'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/enum.rb:40:in `each_common_value'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/enum.rb:19:in `diff'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:45:in `changes_in_type'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:25:in `block in diff'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:103:in `block in each_common_type'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:99:in `each'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:99:in `each_common_type'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator/diff/schema.rb:24:in `diff'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/lib/graphql/schema_comparator.rb:25:in `compare'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/bin/graphql-schema:15:in `compare'
	from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/command.rb:27:in `run'
	from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in `invoke_command'
	from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor.rb:387:in `dispatch'
	from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/base.rb:466:in `start'
	from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.1/bin/graphql-schema:53:in `<top (required)>'
	from /usr/local/bin/graphql-schema:23:in `load'
	from /usr/local/bin/graphql-schema:23:in `<main>'

Is possible it is a an error caused by enum type?

gw20171103.graphql.txt

gw20171108.graphql.txt

Thanks a lot!!

I think this is partly because of

I believe default_value is nil both for the case when it's not set and when it's set to nil, so we might need to add || old_arg.default_value? != new_arg.default_value?.

Then there's also

Which renders poorly when default_value = nil (since nil becomes the empty string).

(Apologies for only submitting a bug report and not test cases or a PR fixing this. I had some weird issues getting the tests to pass deterministically locally...)

I have a directive with a single, required argument:

# localize.rb
class Localize < GraphQL::Schema::Directive
  locations(GraphQL::Schema::Directive::FIELD)
  argument :locale, LocaleEnum, required: true

  def self.resolve(_obj, args, _ctx)
    I18n.with_locale(args.fetch(:locale)) { yield }
  end
end

# locale_enum.rb
class LocaleEnum < GraphQL::Schema::Enum
  description "Available locales"

  I18n.available_locales.each do |locale|
    value locale
  end
end

When I dump the schema and start a comparison, it complains that the type has changed:

Type for argument `locale` on directive `localize` changed from `LocaleEnum!` to `LocaleEnum!`

I debugged a little bit into the source code and found this part

From: /usr/local/bundle/gems/graphql-schema_comparator-1.0.0/lib/graphql/schema_comparator/diff/directive_argument.rb @ line 24 GraphQL::SchemaComparator::Diff::DirectiveArgument#diff:

    11: def diff
    12:   changes = []
    13:
    14:   if old_arg.description != new_arg.description
    15:     changes << Changes::DirectiveArgumentDescriptionChanged.new(directive, old_arg, new_arg)
    16:   end
    17:
    18:   if old_arg.default_value != new_arg.default_value
    19:     changes << Changes::DirectiveArgumentDefaultChanged.new(directive, old_arg, new_arg)
    20:   end
    21:
    22:   if old_arg.type != new_arg.type
    23:     binding.pry
 => 24:     changes << Changes::DirectiveArgumentTypeChanged.new(directive, old_arg, new_arg)
    25:   end
    26:
    27:   # TODO directives on directive arguments
    28:
    29:   changes
    30: end

It seems like the comparison doesn't work properly:

[23] pry(#<GraphQL::SchemaComparator::Diff::DirectiveArgument>)> old_arg
=> #<GraphQL::Schema::Argument:0x000055deff8468c0
 @as=nil,
 @ast_node=
  #<GraphQL::Language::Nodes::InputValueDefinition:0x000055deff792938
   @col=21,
   @default_value=nil,
   @definition_line=9,
   @description=nil,
   @directives=[],
   @filename=nil,
   @line=9,
   @name="locale",
   @type=#<GraphQL::Language::Nodes::NonNullType:0x000055deff792ac8 @filename=nil, @of_type=#<GraphQL::Language::Nodes::TypeName:0x000055deff792b90 @filename=nil, @name="LocaleEnum">>>,
 @default_value=:__no_default__,
 @description=nil,
 @from_resolver=false,
 @keyword=:locale,
 @loads=nil,
 @method_access=false,
 @name="locale",
 @null=true,
 @owner=#<Class:0x000055deff846be0>,
 @prepare=nil,
 @type=#<GraphQL::Schema::NonNull @of_type=#<Class:0x000055deff8e7c70>>,
 @type_expr=#<LateBoundType @name=LocaleEnum>!>
[24] pry(#<GraphQL::SchemaComparator::Diff::DirectiveArgument>)> new_arg
=> #<GraphQL::Schema::Argument:0x000055deffc01cf8
 @as=nil,
 @ast_node=
  #<GraphQL::Language::Nodes::InputValueDefinition:0x000055deffb42ce0
   @col=21,
   @default_value=nil,
   @definition_line=9,
   @description=nil,
   @directives=[],
   @filename=nil,
   @line=9,
   @name="locale",
   @type=#<GraphQL::Language::Nodes::NonNullType:0x000055deffb42f10 @filename=nil, @of_type=#<GraphQL::Language::Nodes::TypeName:0x000055deffb42fd8 @filename=nil, @name="LocaleEnum">>>,
 @default_value=:__no_default__,
 @description=nil,
 @from_resolver=false,
 @keyword=:locale,
 @loads=nil,
 @method_access=false,
 @name="locale",
 @null=true,
 @owner=#<Class:0x000055deffc01fc8>,
 @prepare=nil,
 @type=#<GraphQL::Schema::NonNull @of_type=#<Class:0x000055deffc684f8>>,
 @type_expr=#<LateBoundType @name=LocaleEnum>!>

But when you look into the types, they are the same:

[27] pry(#<GraphQL::SchemaComparator::Diff::DirectiveArgument>)> old_arg.type.to_graphql
=> LocaleEnum!
[28] pry(#<GraphQL::SchemaComparator::Diff::DirectiveArgument>)> new_arg.type.to_graphql
=> LocaleEnum!
[29] pry(#<GraphQL::SchemaComparator::Diff::DirectiveArgument>)> old_arg.type.to_graphql == new_arg.type.to_graphql
=> true

So it seems to me that this is a bug. Any chance that this gets fixed? Would a comparison using to_graphql be of help?

Overview

Right now if an argument is changed from not required to required with a default value it is considered a breaking change. Since the default value guards the case where someone doesn't specify the argument this shouldn't be considered a breaking change.

Hello @ xuorig,

I try to use CLI but I am very new to RoR and I am very lost in what I can be doing wrong

My Log.
/home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.0/bin/graphql-schema:34:in block in print_changes': undefined method breaking' for #<GraphQL::SchemaComparator::Changes::FieldRemoved:0x005638d348d4d0> (NoMethodError) Did you mean? breaking? from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.0/bin/graphql-schema:33:in each'
from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.0/bin/graphql-schema:33:in print_changes' from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.0/bin/graphql-schema:23:in compare'
from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run' from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command'
from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch' from /home/dcalle/.gem/ruby/2.4.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start'
from /home/dcalle/.gem/ruby/2.4.0/gems/graphql-schema_comparator-0.3.0/bin/graphql-schema:53:in <top (required)>' from /home/dcalle/bin/graphql-schema:23:in load'
from /home/dcalle/bin/graphql-schema:23:in <main>'

Can you guide me?

Thanks a lot!!!

Right now a change is any object that responds to #breaking. I'm not totally sure if I like this 🤔

Should think if a base class would make sense, or maybe use composition of change(change_type) ?

At the moment, all EnumValueAdded changes are marked as dangerous because it may break clients that do not handle a certain enum value.

This is true if you're using the enum as a field type, but if you're only using it as an argument type, adding a new enum value should be safe.

Some types of change are breaking in certain cases, and sometimes not. Using the info we have, can we determine if the change is breaking?

This looks super useful! There's many node.js devs using GraphQL (see e.g. postgraphile), and I'd think it would be useful for many of them as well. Have you considered adding a node.js wrapper?

With the GraphQL Spec having moved the deprecation of input values (field args, directive args, input fields) to Stage 3 (graphql/graphql-spec#805), we should consider deprecation_reason when comparing arguments. This is already supported for enums and fields. GraphQL-Ruby supports specifying a reason on arguments (docs).