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.

Sign up for GitHub

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

Fix breaking change messages for root schema type changes/additions #55

Merged
merged 5 commits into from
Oct 18, 2023
Merged

Fix breaking change messages for root schema type changes/additions #55

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7650f6e
Add non-breaking change for schema root type changes and fix root typ…
willlockwood Oct 17, 2023
da6698c
Fix a few path names and null checks, remove duplicated query added type
willlockwood Oct 17, 2023
120b38e
Update null safe operators to account for when query/subscription typ…
willlockwood Oct 17, 2023
a5ee874
Update to use same added/changed/removed changes for every type of sc…
willlockwood Oct 18, 2023
681315e
Change to use OperationType instead of SchemaRootType language
willlockwood Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 74 additions & 10 deletions lib/graphql/schema_comparator/changes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,27 @@ def message
end

def path
# TODO
new_schema.query.graphql_name
end
end

class SchemaQueryTypeAdded < AbstractChange
attr_reader :old_schema, :new_schema, :criticality

def initialize(old_schema, new_schema)
@old_schema = old_schema
@new_schema = new_schema
@criticality = Changes::Criticality.non_breaking(
reason: "Adding a schema query root is considered non-breaking."
)
end

def message
"Schema query root `#{new_schema.query&.graphql_name}` was added."
end

def path
new_schema.query&.graphql_name
end
end

Expand Down Expand Up @@ -414,11 +434,31 @@ def initialize(old_schema, new_schema)
end

def message
"Schema mutation root has changed from `#{old_schema.mutation}` to `#{new_schema.mutation}`"
"Schema mutation root has changed from `#{old_schema.mutation.graphql_name}` to `#{new_schema.mutation&.graphql_name}`"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about the safe operator usage here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here, I think the null safe operator is necessary—this basically covers the case where someone removes a mutation root altogether (instead of changing/renaming it). I just verified that this was also a possible breakage on the schema query/subscription type changed classes, so I added it in there as well. I figured, it would be better to just make these changes null-safe, rather than add a set of SchemaQueryTypeRemoved/SchemaMutationTypeRemoved/SchemaSubscriptionTypeRemoved changes as well.

Let me know if that makes sense to you as well—thank you for taking a look!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another alternative is unifying these into "root operation" changes:

  • RootOperationTypeRemoved
  • RootOperationTypeAdded
  • RootOperationTypeChanged

Messaging could be "Schema root operation type mutation changed from {} to {}`".

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes a lot of sense. Just put those changes in—thanks for the review/suggestion!

end

def path
old_schema.mutation.graphql_name
end
end

class SchemaMutationTypeAdded < AbstractChange
attr_reader :old_schema, :new_schema, :criticality

def initialize(old_schema, new_schema)
@old_schema = old_schema
@new_schema = new_schema
@criticality = Changes::Criticality.non_breaking(
reason: "Adding a schema mutation root is considered non-breaking."
)
end

def message
"Schema mutation root `#{new_schema.mutation.graphql_name}` was added"
end

def path
# TODO
new_schema.mutation.graphql_name
end
end

Expand All @@ -432,11 +472,31 @@ def initialize(old_schema, new_schema)
end

def message
"Schema subscription type has changed from `#{old_schema.subscription}` to `#{new_schema.subscription}`"
"Schema subscription type has changed from `#{old_schema.subscription.graphql_name}` to `#{new_schema.subscription.graphql_name}`"
end

def path
old_schema.subscription.graphql_name
end
end

class SchemaSubscriptionTypeAdded < AbstractChange
attr_reader :old_schema, :new_schema, :criticality

def initialize(old_schema, new_schema)
@old_schema = old_schema
@new_schema = new_schema
@criticality = Changes::Criticality.non_breaking(
reason: "Adding a schema subscription root is considered non-breaking."
)
end

def message
"Schema subscription root `#{new_schema.subscription.graphql_name}` was added"
end

def path
# TODO
new_schema.subscription.graphql_name
end
end

Expand All @@ -457,11 +517,11 @@ def initialize(type, field, old_argument, new_argument)
end

def message
if old_argument.default_value.nil? || old_argument.default_value == :__no_default__
"Default value `#{new_argument.default_value}` was added to argument `#{new_argument.graphql_name}` on field `#{field.path}`"
else
if old_argument.default_value?
"Default value for argument `#{new_argument.graphql_name}` on field `#{field.path}` changed"\
" from `#{old_argument.default_value}` to `#{new_argument.default_value}`"
else
"Default value `#{new_argument.default_value}` was added to argument `#{new_argument.graphql_name}` on field `#{field.path}`"
Comment on lines -461 to +500
Copy link
Contributor Author

@willlockwood willlockwood Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, just a heads up about these default value changes—I updated these because the tests were failing on newer versions of graphql-ruby, that don't rely on :__no_default__ to specify no default argument value.

These changes only really are only necessary if you're running a later version of graphql-ruby—if you'd like me to make these changes separately and also set up a new gemfile specifically to run the test suite on graphql-ruby 2.1, let me know and I'd be happy to pull these changes out of this PR and do that as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice improvement 👍 default_value? has actually been around forever so we can safely use that: rmosolgo/graphql-ruby@23a467d

It's just that __no_default__ was only removed in 2.1 I guess.

end
end

Expand Down Expand Up @@ -507,8 +567,12 @@ def initialize(directive, old_argument, new_argument)
end

def message
"Default value for argument `#{new_argument.graphql_name}` on directive `#{directive.graphql_name}` changed"\
" from `#{old_argument.default_value}` to `#{new_argument.default_value}`"
if old_argument.default_value?
"Default value for argument `#{new_argument.graphql_name}` on directive `#{directive.graphql_name}` changed"\
" from `#{old_argument.default_value}` to `#{new_argument.default_value}`"
else
"Default value `#{new_argument.default_value}` was added to argument `#{new_argument.graphql_name}` on directive `#{directive.graphql_name}`"
end
end

def path
Expand Down
18 changes: 15 additions & 3 deletions lib/graphql/schema_comparator/diff/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,27 @@ def changes_in_schema
changes = []

if old_schema.query&.graphql_name != new_schema.query&.graphql_name
changes << Changes::SchemaQueryTypeChanged.new(old_schema, new_schema)
if old_schema.query
changes << Changes::SchemaQueryTypeChanged.new(old_schema, new_schema)
else
changes << Changes::SchemaQueryTypeAdded.new(old_schema, new_schema)
end
end

if old_schema.mutation&.graphql_name != new_schema.mutation&.graphql_name
changes << Changes::SchemaMutationTypeChanged.new(old_schema, new_schema)
if old_schema.mutation
changes << Changes::SchemaMutationTypeChanged.new(old_schema, new_schema)
else
changes << Changes::SchemaMutationTypeAdded.new(old_schema, new_schema)
end
end

if old_schema.subscription&.graphql_name != new_schema.subscription&.graphql_name
changes << Changes::SchemaSubscriptionTypeChanged.new(old_schema, new_schema)
if old_schema.subscription
changes << Changes::SchemaSubscriptionTypeChanged.new(old_schema, new_schema)
else
changes << Changes::SchemaSubscriptionTypeAdded.new(old_schema, new_schema)
end
end

changes
Expand Down
60 changes: 59 additions & 1 deletion test/lib/graphql/schema_comparator/diff/schema_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ def setup
@old_schema = <<~SCHEMA
schema {
query: Query
mutation: OldMutation
}
input AInput {
# a
Expand Down Expand Up @@ -67,13 +68,17 @@ def setup
type WillBeRemoved {
a: String
}
type OldMutation {
a: String!
}

directive @willBeRemoved on FIELD
SCHEMA

@new_schema =<<~SCHEMA
schema {
query: Query
mutation: Mutation
}
input AInput {
# changed
Expand Down Expand Up @@ -139,6 +144,10 @@ def setup
# Included when true.
someArg: String!
) on FIELD

type Mutation {
a: String!
}
SCHEMA

@differ = GraphQL::SchemaComparator::Diff::Schema.new(
Expand Down Expand Up @@ -168,7 +177,7 @@ def test_changes_kitchensink
"Deprecation reason on field `CType.a` has changed from `whynot` to `cuz`",
"Argument `arg: Int` added to field `CType.a`",
"Default value `10` was added to argument `arg` on field `CType.d`",
"Default value for argument `anotherArg` on directive `yolo` changed from `__no_default__` to `Test`",
"Default value `Test` was added to argument `anotherArg` on directive `yolo`",
"Union member `BType` was removed from Union type `MyUnion`",
"Union member `DType` was added to Union type `MyUnion`",
"Field `anotherInterfaceField` was removed from object type `AnotherInterface`",
Expand All @@ -192,6 +201,9 @@ def test_changes_kitchensink
"Argument `willBeRemoved` was removed from directive `yolo`",
"Description for argument `someArg` on directive `yolo` changed from `Included when true.` to `someArg does stuff`",
"Type for argument `someArg` on directive `yolo` changed from `Boolean!` to `String!`",
"Type `Mutation` was added",
"Type `OldMutation` was removed",
"Schema mutation root has changed from `OldMutation` to `Mutation`",
].sort, @differ.diff.map(&:message).sort

assert_equal [
Expand All @@ -214,8 +226,11 @@ def test_changes_kitchensink
"CType.a.arg",
"CType.d.arg",
"CType.interfaceField",
"Mutation",
"MyUnion",
"MyUnion",
"OldMutation",
"OldMutation",
"AnotherInterface.anotherInterfaceField",
"AnotherInterface.b",
"WithInterfaces",
Expand All @@ -241,6 +256,48 @@ def test_changes_kitchensink
].sort, @differ.diff.map(&:path).sort
end

def test_schema_changes
old_schema = <<~SCHEMA
schema {
query: Query
}

type Query {
a: String!
}
SCHEMA

new_schema = <<~SCHEMA
schema {
query: Query
mutation: Mutation
subscription: Subscription
}

type Query {
a: String!
}

type Mutation {
a: String!
}

type Subscription {
a: String!
}
SCHEMA

expected_changes = [
{ path: "Mutation", message: "Schema mutation root `Mutation` was added", level: 1 },
{ path: "Mutation", message: "Type `Mutation` was added", level: 1 },
{ path: "Subscription", message: "Schema subscription root `Subscription` was added", level: 1 },
{ path: "Subscription", message: "Type `Subscription` was added", level: 1 },
]

actual_changes = schema_diff(old_schema, new_schema)
assert_equal(normalize_schema_diff(expected_changes), normalize_schema_diff(actual_changes))
end

def test_enum_value_changes
old_schema = <<~SCHEMA
schema {
Expand Down Expand Up @@ -354,6 +411,7 @@ def schema_diff(old_schema, new_schema)
end

def normalize_schema_diff(changes)
puts changes
changes.sort_by { |changes| [changes[:path], changes[:message], changes[:level]] }
end
end