-
-
Notifications
You must be signed in to change notification settings - Fork 205
Add patch package to apply diff changes to schemas #2893
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
base: master
Are you sure you want to change the base?
Changes from 13 commits
cf7fcfc
2adf87b
dfe87bf
c3dcc73
6fd13d2
0cdcc17
985a146
3f781fb
441c198
288ae36
f19e299
6cd0ba3
bab8b86
62c16ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@graphql-inspector/core': major | ||
--- | ||
|
||
"diff" includes all nested changes when a node is added. Some change types have had additional meta fields added. | ||
On deprecation add with a reason, a separate "fieldDeprecationReasonAdded" change is no longer included. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,13 +21,36 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Query.a.external'); | ||
const change = findFirstChangeByPath(changes, 'Query.a.@external'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding an indicator for directives is necessary to distinguish them from arguments. This makes the paths more meaningful and useful as lookups. |
||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_FIELD_DEFINITION_ADDED'); | ||
expect(change.message).toEqual("Directive 'external' was added to field 'Query.a'"); | ||
}); | ||
|
||
test('added directive on added field', async () => { | ||
const a = buildSchema(/* GraphQL */ ` | ||
type Query { | ||
_: String | ||
} | ||
`); | ||
const b = buildSchema(/* GraphQL */ ` | ||
directive @external on FIELD_DEFINITION | ||
|
||
type Query { | ||
_: String | ||
a: String @external | ||
} | ||
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Query.a.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_FIELD_DEFINITION_ADDED'); | ||
expect(change.message).toEqual("Directive 'external' was added to field 'Query.a'"); | ||
}); | ||
|
||
test('removed directive', async () => { | ||
const a = buildSchema(/* GraphQL */ ` | ||
directive @external on FIELD_DEFINITION | ||
|
@@ -44,7 +67,7 @@ describe('directive-usage', () => { | |
} | ||
`); | ||
|
||
const change = findFirstChangeByPath(await diff(a, b), 'Query.a.external'); | ||
const change = findFirstChangeByPath(await diff(a, b), 'Query.a.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_FIELD_DEFINITION_REMOVED'); | ||
|
@@ -68,7 +91,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Query.a.oneOf'); | ||
const change = findFirstChangeByPath(changes, 'Query.a.@oneOf'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Breaking); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_FIELD_DEFINITION_ADDED'); | ||
|
@@ -91,7 +114,7 @@ describe('directive-usage', () => { | |
} | ||
`); | ||
|
||
const change = findFirstChangeByPath(await diff(a, b), 'Query.a.oneOf'); | ||
const change = findFirstChangeByPath(await diff(a, b), 'Query.a.@oneOf'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_FIELD_DEFINITION_REMOVED'); | ||
|
@@ -128,7 +151,7 @@ describe('directive-usage', () => { | |
union Foo @external = A | B | ||
`); | ||
|
||
const change = findFirstChangeByPath(await diff(a, b), 'Foo.external'); | ||
const change = findFirstChangeByPath(await diff(a, b), 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_UNION_MEMBER_ADDED'); | ||
|
@@ -164,7 +187,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_UNION_MEMBER_REMOVED'); | ||
|
@@ -199,7 +222,7 @@ describe('directive-usage', () => { | |
union Foo @oneOf = A | B | ||
`); | ||
|
||
const change = findFirstChangeByPath(await diff(a, b), 'Foo.oneOf'); | ||
const change = findFirstChangeByPath(await diff(a, b), 'Foo.@oneOf'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Breaking); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_UNION_MEMBER_ADDED'); | ||
|
@@ -235,7 +258,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.oneOf'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@oneOf'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_UNION_MEMBER_REMOVED'); | ||
|
@@ -270,7 +293,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'enumA.external'); | ||
const change = findFirstChangeByPath(changes, 'enumA.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.criticality.reason).toBeDefined(); | ||
|
@@ -302,7 +325,7 @@ describe('directive-usage', () => { | |
} | ||
`); | ||
|
||
const change = findFirstChangeByPath(await diff(a, b), 'enumA.external'); | ||
const change = findFirstChangeByPath(await diff(a, b), 'enumA.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_ENUM_REMOVED'); | ||
|
@@ -338,7 +361,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'enumA.B.external'); | ||
const change = findFirstChangeByPath(changes, 'enumA.B.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -373,7 +396,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'enumA.A.external'); | ||
const change = findFirstChangeByPath(changes, 'enumA.A.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -400,7 +423,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -424,7 +447,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -451,7 +474,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -477,7 +500,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -500,7 +523,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -518,7 +541,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
|
@@ -543,7 +566,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_OBJECT_ADDED'); | ||
|
@@ -564,7 +587,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_OBJECT_REMOVED'); | ||
|
@@ -588,7 +611,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_INTERFACE_ADDED'); | ||
|
@@ -610,7 +633,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_INTERFACE_REMOVED'); | ||
|
@@ -634,7 +657,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.a.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.a.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_ARGUMENT_DEFINITION_ADDED'); | ||
|
@@ -658,7 +681,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.a.external'); | ||
const change = findFirstChangeByPath(changes, 'Foo.a.a.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_ARGUMENT_DEFINITION_REMOVED'); | ||
|
@@ -690,7 +713,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, '.@external'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This indicates the directive is applied to the schema object. This |
||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_SCHEMA_ADDED'); | ||
|
@@ -717,7 +740,7 @@ describe('directive-usage', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'Foo.external'); | ||
const change = findFirstChangeByPath(changes, '.@external'); | ||
|
||
expect(change.criticality.level).toEqual(CriticalityLevel.Dangerous); | ||
expect(change.type).toEqual('DIRECTIVE_USAGE_SCHEMA_REMOVED'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,13 +173,13 @@ describe('directive', () => { | |
}; | ||
|
||
// Nullable | ||
expect(change.a.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.a.type).toEqual('DIRECTIVE_ARGUMENT_ADDED'); | ||
expect(change.a.message).toEqual(`Argument 'name' was added to directive 'a'`); | ||
expect(change.a?.type).toEqual('DIRECTIVE_ARGUMENT_ADDED'); | ||
expect(change.a?.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In several places I reordered things because when tweaking behavior and testing, t was not providing me enough insights to understand what was wrong. Placing the "type" first let me know which change type was being returned. |
||
expect(change.a?.message).toEqual(`Argument 'name' was added to directive 'a'`); | ||
// Non-nullable | ||
expect(change.b.criticality.level).toEqual(CriticalityLevel.Breaking); | ||
expect(change.b.type).toEqual('DIRECTIVE_ARGUMENT_ADDED'); | ||
expect(change.b.message).toEqual(`Argument 'name' was added to directive 'b'`); | ||
expect(change.b?.type).toEqual('DIRECTIVE_ARGUMENT_ADDED'); | ||
expect(change.b?.criticality.level).toEqual(CriticalityLevel.Breaking); | ||
expect(change.b?.message).toEqual(`Argument 'name' was added to directive 'b'`); | ||
}); | ||
|
||
test('removed', async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,56 @@ | ||
import { buildSchema } from 'graphql'; | ||
import { CriticalityLevel, diff, DiffRule } from '../../src/index.js'; | ||
import { findFirstChangeByPath } from '../../utils/testing.js'; | ||
import { ChangeType, CriticalityLevel, diff, DiffRule } from '../../src/index.js'; | ||
import { findChangesByPath, findFirstChangeByPath } from '../../utils/testing.js'; | ||
|
||
describe('enum', () => { | ||
test('added', async () => { | ||
const a = buildSchema(/* GraphQL */ ` | ||
type Query { | ||
fieldA: String | ||
} | ||
`); | ||
|
||
const b = buildSchema(/* GraphQL */ ` | ||
type Query { | ||
fieldA: String | ||
} | ||
|
||
enum enumA { | ||
""" | ||
A is the first letter in the alphabet | ||
""" | ||
A | ||
B | ||
} | ||
`); | ||
|
||
const changes = await diff(a, b); | ||
expect(changes.length).toEqual(4); | ||
|
||
{ | ||
const change = findFirstChangeByPath(changes, 'enumA'); | ||
expect(change.meta).toMatchObject({ | ||
addedTypeKind: 'EnumTypeDefinition', | ||
addedTypeName: 'enumA', | ||
}); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.criticality.reason).not.toBeDefined(); | ||
expect(change.message).toEqual(`Type 'enumA' was added`); | ||
} | ||
|
||
{ | ||
const change = findFirstChangeByPath(changes, 'enumA.A'); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test shows that enum additions also contain all nested changes within that enum, and that those changes are flagged as non-breaking. |
||
expect(change.criticality.reason).not.toBeDefined(); | ||
expect(change.message).toEqual(`Enum value 'A' was added to enum 'enumA'`); | ||
expect(change.meta).toMatchObject({ | ||
addedEnumValueName: 'A', | ||
enumName: 'enumA', | ||
addedToNewType: true, | ||
}); | ||
} | ||
}); | ||
|
||
test('value added', async () => { | ||
const a = buildSchema(/* GraphQL */ ` | ||
type Query { | ||
|
@@ -130,7 +178,7 @@ describe('enum', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'enumA.A'); | ||
const change = findFirstChangeByPath(changes, 'enumA.A.@deprecated'); | ||
|
||
expect(changes.length).toEqual(1); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
|
@@ -163,11 +211,26 @@ describe('enum', () => { | |
`); | ||
|
||
const changes = await diff(a, b); | ||
const change = findFirstChangeByPath(changes, 'enumA.A'); | ||
|
||
expect(changes.length).toEqual(2); | ||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
expect(change.message).toEqual(`Enum value 'enumA.A' was deprecated with reason 'New Reason'`); | ||
expect(changes).toHaveLength(3); | ||
const directiveChanges = findChangesByPath(changes, 'enumA.A.@deprecated'); | ||
expect(directiveChanges).toHaveLength(2); | ||
|
||
for (const change of directiveChanges) { | ||
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking); | ||
if (change.type === ChangeType.EnumValueDeprecationReasonAdded) { | ||
expect(change.message).toEqual( | ||
`Enum value 'enumA.A' was deprecated with reason 'New Reason'`, | ||
); | ||
} else if (change.type === ChangeType.DirectiveUsageEnumValueAdded) { | ||
expect(change.message).toEqual(`Directive 'deprecated' was added to enum value 'enumA.A'`); | ||
} | ||
} | ||
|
||
{ | ||
const change = findFirstChangeByPath(changes, '[email protected]'); | ||
expect(change.type).toEqual(ChangeType.DirectiveUsageArgumentAdded); | ||
expect(change.message).toEqual(`Argument 'reason' was added to '@deprecated'`); | ||
} | ||
}); | ||
|
||
test('deprecation reason removed', async () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced that this is a minor patch because the changes to the output's format doesnt break anything. But the actual content of the changes has changed drastically which is why I thought we should be safe and declare a major change.