Add patch package to apply diff changes to schemas #2893

jdolle · 2025-07-08T05:48:25Z

Description

This change began as one to only the adds fields that are necessary for recreating the patched schema using the input schema and changes array, but it has expanded to also add the patch functionality. I found this was necessary in order to verify that the changes provided sufficient data.

This PR modifies the several change types, and modifies the onAdded handlers to also execute equivalent logic to onMutual. It also adds a new rule that can be added to filter out these additional changes, thus keeping changelogs to a minimum.

The expansion of "onAdded" has a few down sides and an important up side.

Cons

Increased noise when listing changes
Increased complexity because it blurs the line between changes and additions. Although arguably some of the changes did this already.
It requires adjusting some of the criticality of these changes when they are created from within the onAdded call. Because e.g. adding an interface to newly create object is completely safe, but adding an interface to an existing object is considered dangerous.

Pro

Does not require parsing additional change events or nested metadata within addition events.

The best alternative approach I could come up with is to add metadata to the "added" change events. This is doable, but requires essentially recreating all the parsing that is already done in onMutual, and then requires any logic using these events to also recreate logic for adding things to the schema, because for example a field could be added to a type via a FieldAdded change OR within a TypeAdded change. And the structures would not match.
This approach would make the meta data very large also, because it would have to capture interfaces, directives, fields, etc..
However, it would prevent us from polluting the existing change set with more noise.

An additional change is that multiple changes are no longer returned for the same "node". e.g. When adding a deprecated directive with a reason, the reason field is on the added change, and a "deprecatedReasonAdded" change is no longer emitted.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

See unit tests.

Checklist:

I have followed the
CONTRIBUTING doc and the
style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

jdolle · 2025-07-08T17:48:43Z

packages/core/src/diff/changes/directive.ts

  newDirective: GraphQLDirective,
 ): Change<typeof ChangeType.DirectiveDescriptionChanged> {
  return directiveDescriptionChangedFromMeta({
    type: ChangeType.DirectiveDescriptionChanged,
    meta: {
-      directiveName: oldDirective.name,
-      oldDirectiveDescription: oldDirective.description ?? null,
+      directiveName: newDirective.name,


In these cases, the type is being used to extract the name only. It doesn't matter if we pass the new or old directive, since the name is the same. However, since calling these functions onAdd -- the old type can be null, which is why this was changed to pass the new type (newDirective)

jdolle · 2025-07-08T17:57:11Z

packages/core/__tests__/diff/enum.test.ts

+
+    {
+      const change = findFirstChangeByPath(changes, 'enumA.A');
+      expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking);


This test shows that enum additions also contain all nested changes within that enum, and that those changes are flagged as non-breaking.

jdolle · 2025-07-09T04:46:09Z

packages/core/src/diff/changes/field.ts

@@ -218,6 +221,7 @@ export function fieldDeprecationRemovedFromMeta(args: FieldDeprecationRemovedCha
  return {
    type: ChangeType.FieldDeprecationRemoved,
    criticality: {
+      // @todo: Add a reason for why is this dangerous... Why is it??


^ very curious why... It doesn't change behavior/how clients interact. "Dangerous" seems extreme.

packages/core/src/diff/changes/type.ts

jdolle · 2025-07-09T04:49:57Z

packages/core/src/diff/field.ts

-          parentType: type,
-          field: newField,
-        }),
+        directiveUsageAdded(


Should this filter out the deprecated directive since that's captured by another change?

jdolle · 2025-07-09T05:12:54Z

.changeset/seven-jars-yell.md

@@ -0,0 +1,6 @@
+---
+'@graphql-inspector/core': major


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.

jdolle · 2025-07-09T21:31:51Z

packages/core/src/diff/rules/ignore-nested-additions.ts

+  return lastDividerIndex === -1 ? path : path.substring(0, lastDividerIndex);
+};
+
+export const ignoreNestedAdditions: Rule = ({ changes }) => {


This allows us to reduce noise when running the diff. If the parent was added during this change set, then the change will be excluded from the list.
See tests for what this looks like in practice.

jdolle · 2025-07-24T00:19:45Z

packages/core/__tests__/diff/directive.test.ts

-      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);


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.

jdolle · 2025-07-26T18:50:32Z

packages/core/__tests__/diff/directive-usage.test.ts

@@ -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');


Adding an indicator for directives is necessary to distinguish them from arguments. This makes the paths more meaningful and useful as lookups.

jdolle · 2025-07-26T18:51:41Z

packages/core/__tests__/diff/directive-usage.test.ts

@@ -690,7 +713,7 @@ describe('directive-usage', () => {
      `);

      const changes = await diff(a, b);
-      const change = findFirstChangeByPath(changes, 'Foo.external');
+      const change = findFirstChangeByPath(changes, '.@external');


This indicates the directive is applied to the schema object. This . is necessary to distinguish directive usages from directive definitions at the root level.

jdolle · 2025-07-28T15:44:21Z

packages/core/src/diff/enum.ts

+      isVoid(oldValue?.deprecationReason) ||
+      oldValue?.deprecationReason === DEPRECATION_REASON_DEFAULT
+    ) {
+      addChange(enumValueDeprecationReasonAdded(newEnum, oldValue, newValue));


Thoughts on removing this in favor of only using the directive change for @deprecated instead of a unique change type?

jdolle requested a review from n1ru4l July 8, 2025 17:41

jdolle self-assigned this Jul 8, 2025

jdolle commented Jul 8, 2025

View reviewed changes

major: diff includes all nested changes when a node is added

cf7fcfc

jdolle force-pushed the type-added-meta branch from 359eea3 to cf7fcfc Compare July 9, 2025 04:40

jdolle commented Jul 9, 2025

View reviewed changes

packages/core/src/diff/changes/type.ts Outdated Show resolved Hide resolved

jdolle commented Jul 9, 2025

View reviewed changes

jdolle requested a review from dotansimha July 9, 2025 04:50

jdolle changed the title ~~Enhance TypeAdded meta data~~ Diff returns all nested changes for additions Jul 9, 2025

Fix directive argument changes to match others

2adf87b

jdolle commented Jul 9, 2025

View reviewed changes

jdolle added 5 commits July 9, 2025 12:59

Add rule to ignore nested additions

dfe87bf

Add a field test

c3dcc73

Fix parent path; add more tests

6fd13d2

TypeChanged changes

0cdcc17

prettier

985a146

jdolle commented Jul 9, 2025

View reviewed changes

jdolle marked this pull request as ready for review July 9, 2025 21:33

Add more meta to changes

3f781fb

jdolle requested a review from ardatan July 17, 2025 04:25

jdolle added 2 commits July 23, 2025 15:48

Add directive usage; add tests

441c198

Improve path handling

288ae36

jdolle changed the title ~~Diff returns all nested changes for additions~~ Add patch package to apply diff changes to schemas Jul 24, 2025

jdolle commented Jul 24, 2025

View reviewed changes

WIP: Print test schemas with directives.

f19e299

jdolle added the ⚙️ work in progress Someone is working on it label Jul 24, 2025

Support all directive usage cases

6cd0ba3

jdolle removed the ⚙️ work in progress Someone is working on it label Jul 26, 2025

jdolle added the kind/enhancement New feature or request label Jul 26, 2025

jdolle commented Jul 26, 2025

View reviewed changes

dotansimha requested review from kamilkisiela and removed request for ardatan July 27, 2025 14:19

Remove unnecessary import

bab8b86

jdolle commented Jul 28, 2025

View reviewed changes

Same

62c16ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add patch package to apply diff changes to schemas #2893

Add patch package to apply diff changes to schemas #2893

Uh oh!

jdolle commented Jul 8, 2025 •

edited

Loading

Uh oh!

jdolle Jul 8, 2025

Uh oh!

jdolle Jul 8, 2025

Uh oh!

jdolle Jul 9, 2025

Uh oh!

Uh oh!

jdolle Jul 9, 2025

Uh oh!

jdolle Jul 9, 2025

Uh oh!

jdolle Jul 9, 2025

Uh oh!

jdolle Jul 24, 2025 •

edited

Loading

Uh oh!

jdolle Jul 26, 2025

Uh oh!

jdolle Jul 26, 2025

Uh oh!

jdolle Jul 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Add patch package to apply diff changes to schemas #2893

Are you sure you want to change the base?

Add patch package to apply diff changes to schemas #2893

Uh oh!

Conversation

jdolle commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Cons

Pro

Type of change

How Has This Been Tested?

Checklist:

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!

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!

jdolle Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jdolle Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdolle commented Jul 8, 2025 •

edited

Loading

jdolle Jul 24, 2025 •

edited

Loading

jdolle Jul 28, 2025 •

edited

Loading