Add a utility to detect description changes #1127

eapache · 2017-12-07T17:41:22Z

We use this internally for pinging docs team for PR reviews.

Discussed briefly at #981 cc @leebyron.

Flow is complaining about some of this code, but Flow is just wrong; the equivalence check between newType and oldType provides the necessary safety guarantees. Not sure what the preferred solution is here.

leebyron

This looks great. I've added some review notes to help you get Flow to pass and with suggestions to simplify the code and improve the usefulness of it's results.

leebyron · 2017-12-07T19:06:21Z

src/utilities/findDescriptionChanges.js

+      }
+    }
+
+    if (oldType && !(newType instanceof oldType.constructor)) {


Flow cannot see this as a cross-type refinement, plus it's a bit challenging to understand. I would instead make instanceof invariants for the oldType within each case below

I did it this way because spelling out the instanceofs actually becomes really messy in its own right to handle the cases where oldType is null/undefined, and where oldType is present but a different kind from newType. I'll play with it a bit to see if I can make it nicer, but any suggestions would be very welcome.

In this case, below line 50, you could just mirror the if statement above in an invariant while including the falsey check:

invariant( !oldType || oldType instanceof GraphQLObjectType || oldType instanceof GraphQLInterfaceType || oldType instanceof GraphQLInputObjectType, 'Expected oldType to also have fields' );

leebyron · 2017-12-07T19:10:50Z

src/utilities/findDescriptionChanges.js

+      newType instanceof GraphQLInterfaceType ||
+      newType instanceof GraphQLInputObjectType
+    ) {
+      const oldTypeFields: ?GraphQLFieldMap<*, *> = oldType


Should this differentiate between the old type not existing vs the old type existing but the field not existing?

It could be overwhelming to see a ton of "Description added on new field" for every field on a type that was newly added, that might not be helpful.

Instead perhaps "New type added with description" and "New field added with description" should be different from "Description added on field"

It could be overwhelming to see a ton of "Description added on new field" for every field on a type that was newly added, that might not be helpful.

The goal was to provide our docs team with a comprehensive set of all the new strings that needed reviewing; I'll type these differently when I add stronger types per your earlier comment, and then the caller can filter as desired.

leebyron · 2017-12-07T19:11:21Z

src/utilities/findDescriptionChanges.js

+        if (newField.description) {
+          if (!oldField) {
+            descriptionChanges.push(
+              `Description added on new field ${newType.name}.${newField.name}`,


This wording is a little confusing. How do you add to something that is new? Add implies additive to the existing state

leebyron · 2017-12-07T19:12:26Z

src/utilities/findDescriptionChanges.js

+export function findDescriptionChanges(
+  oldSchema: GraphQLSchema,
+  newSchema: GraphQLSchema,
+): Array<string> {


I would suggest mirroring the return value of the breaking change detection which offers what kind of detection was encountered. It looks like there are a handful of types here to differentiate: new types, new fields on existing types, added descriptions where there were none, descriptions changed, descriptions removed.

Also, if these are complex objects, you could contain references to the old/new schema elements themselves, that would allow you to use the output of this function to build a review tool. Currently you just have english text which requires manually looking up the described types to see what changes actually occurred, which would be limited in usefulness.

leebyron · 2017-12-07T19:16:04Z

src/utilities/findDescriptionChanges.js

+            );
+          }
+        }
+      });


Much of the logic above looks repetitive, with the exception of the strings referencing each kind of type. Could you factor out the common logic?

I'll take a look. Javascript isn't my forte so for a first pass I definitely stuck with a simpler more explicit style.

odykyi · 2017-12-27T13:15:22Z

src/utilities/findDescriptionChanges.js

+        const oldField = oldTypeFields ? oldTypeFields[fieldName] : null;
+        const newField = newTypeFields[fieldName];
+
+        if (newField.description) {


if (newField && newField.description) {

eapache · 2018-01-02T16:19:33Z

Sorry for the delay on this; finally got flow passing and I think I addressed all the code review comments. Please let me know if there's anything else.

leebyron · 2018-01-29T20:57:04Z

Thanks for the update and sorry for the review delay - it appears the test coverage is still below where we usually try to keep it. Here's a link from the coveralls report: https://coveralls.io/builds/14868261/source?filename=src%2Futilities%2FfindDescriptionChanges.js

Could you add more tests to ensure all aspects of the new functionality are well tested?

We use this internally for pinging docs team for PR reviews.

Strongly type the return objects, use invariants in a way to satisfy flow, and DRY up one common function.

eapache · 2018-04-02T16:27:06Z

Sorry for the delay again, I think we're both just busy :)

I've added some tests, so Coveralls now says overall coverage has increased. There's only one line (86) that's not covered at all and I'm not sure why - is newField.args expected to be truthy even when the field has no arguments? Should that check be adjusted, or removed? Otherwise (with the exception of "branch missed" warnings I don't know what to do with since it's not clear which branch was missed) everything should be covered.

IvanGoncharov · 2018-04-02T17:47:59Z

src/utilities/findDescriptionChanges.js

+          oldType instanceof GraphQLObjectType ||
+          oldType instanceof GraphQLInterfaceType ||
+          oldType instanceof GraphQLInputObjectType,
+        'Expected oldType to also have fields',


It shouldn't be an error.
For example:

scalar Date

type Date { day: Int month: Int year: Int }

This was added to satisfy Flow (#1127 (comment)).

This (and the enum case) are breaking schema changes, so an error seemed appropriate? Would you prefer they were silently ignored? If so, how do I do that in a way that Flow will recognize?

So if I do breaking change in my schema I don't need to notify my documentation team about other non-breaking changes.
BTW Breaking changes is normal in many cases for example if you have your API in public beta.

Yes, you're right. I was thinking too much about our specific case (where breaking changes already ping a substantial set of people for review and the docs changes are less relevant) but in the general case this should absolutely work.

I'm still not sure how to satisfy Flow without an invariant, but that's purely me not being much of a javascript developer.

IvanGoncharov · 2018-04-02T17:49:56Z

src/utilities/findDescriptionChanges.js

+      invariant(
+        !oldType || oldType instanceof GraphQLEnumType,
+        'Expected oldType to also have values',
+      );


Same here:

scalar Month

enum Month { Jan Feb # ... }

IvanGoncharov · 2018-04-02T17:53:21Z

src/utilities/findDescriptionChanges.js

+  FIELD: 'FIELD',
+  TYPE: 'TYPE',
+  ARGUMENT: 'ARGUMENT',
+  ENUM_VALUE: 'ENUM_VALUE',


Your missing directives + you need to check arguments of directives in findDescriptionChanges

IvanGoncharov · 2018-04-02T17:57:07Z

src/utilities/findDescriptionChanges.js

+};
+
+export const DescriptionChangeType = {
+  OBJECT_ADDED: 'OBJECT_ADDED',


As I understand it trigger when any new type is added not only GraphQLObjectType. So I think it should have a different name.

IvanGoncharov · 2018-04-02T17:58:11Z

src/utilities/findDescriptionChanges.js

@@ -0,0 +1,163 @@
+/* eslint-disable no-restricted-syntax */
+// @flow


Please use the same header license + @flow strict as other files

IvanGoncharov · 2018-04-02T18:01:19Z

src/utilities/findDescriptionChanges.js

@@ -0,0 +1,163 @@
+/* eslint-disable no-restricted-syntax */


Please remove and fix all eslint errors if any.

IvanGoncharov · 2018-04-02T18:25:27Z

@eapache Sorry for not reviewing it earlier.

My 2¢: Different developers and teams have different needs but we can't add find*Changes function for every case. So I think we need more generic solutions for finding the difference between two schemas.

From #981:

aka anything that might need to be reviewed by a docs team

In that case changes in deprecationReason is also relevant and should be reviewed so we need to add findDeprecationReasonChanges.

Moreover, some documentation teams would be interested in being notified of new types without description.

I also see a use case for detecting any non-documentation (not in description or deprecationReason) change.

So I think instead of adding new find*Changes function for every use case we should provide single diffSchemas function. That returns something like this:

type SchemaDifference = {|
  added: {|
    queryType?: GraphQLObjectType,
    // ...
    types: Array<GraphQLTypes>,
    directives: Array<GraphQLDirectives>,
  |},
  changed: {|
    queryType?: ValueDifference<GraphQLType>,
    // ...
    types: Array<TypeDifference>,
    directives: Array<DirectivesDifference>,
  |},
  // ...
|};

type ValueDifference<T> = { old: T, new: T };

type TypeDifference = ScalarTypeDifference | ObjectTypeDifference /* ... */

type ObjectTypeDifference = {|
  name: string,
  added: {|
    description: string,
    fields: Array<GraphQLField>,
    // ...
  |},
  changed: {|
    description: ValueDifference<string>,
    fields: Array<FieldDifference>,
    // ...
  |},
  // ...
|};

It will allow to implement your particular check as:

const diff = diffSchema(oldSchema, newSchema);

for (const newType of diff.added.types) {
  if (newType.description) {
    console.log('Description of a new type ...');
  }
  newType.getFields().forEach(reportNewField);
}

for (const typeDiff of diff.changed.types) {
  if (typeDiff.added.description) {
    console.log('Added type description ...');
  }
  typeDiff.added.fields.forEach(reportNewField);
  if (typeDiff.changed.description) {
    console.log('Changed type description ...');
  }
  for (const fieldDiff of typeDiff.changed.field) {
    if (fieldDiff.added.description) {
      console.log('Added field description ...');
      // ...
    }
  }
}

function reportNewField(field) {
  if (field.description) {
    // ...
  }
}

I think it would be more generic approach and allow other developers to easily implement their project-specific needs.

@leebyron What do you think?

eapache · 2018-04-02T18:43:42Z

Different developers and teams have different needs but we can't add find*Changes function for every case. So I think we need more generic solutions for finding the difference between two schemas.

You're not wrong, however a full diffSchema is well outside of my available time here (and probably outside of my skillset to implement correctly in js). If you'd prefer to move in that direction instead I'll wait until somebody else tackles it and we can keep just using our copy of this method internally for the time being.

The only thing I can think might still belong outside of diffSchema is detecting breaking changes, since to a certain extent that check is not just descriptive but also prescriptive of what changes count as breaking. Though if that is already well-specified elsewhere we could just have a breaking: true/false flag on the various different types returned by diffSchema?

Moreover, some documentation teams would be interested in being notified of new types without description.

Also a good point. We just have CI fail in this case, but I imagine that isn't universal.

facebook-github-bot · 2018-07-02T00:49:32Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-07-02T01:08:24Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

IvanGoncharov · 2021-08-10T10:05:09Z

Closing since it's outdated but extracted diffSchema idea into #3224

facebook-github-bot added the CLA Signed label Dec 7, 2017

leebyron requested changes Dec 7, 2017

View reviewed changes

OlegIlyenko mentioned this pull request Dec 7, 2017

Verify: some minor changes to the ref. impl. sangria-graphql/sangria#311

Closed

13 tasks

odykyi reviewed Dec 27, 2017

View reviewed changes

eapache force-pushed the find-description-changes branch from bb202a5 to 4576e13 Compare January 2, 2018 16:11

eapache added 3 commits April 2, 2018 12:08

Add a utility to detect description changes

eb15335

We use this internally for pinging docs team for PR reviews.

Refactor for better typing

3948be9

Strongly type the return objects, use invariants in a way to satisfy flow, and DRY up one common function.

Add more tests

9dd09cd

eapache force-pushed the find-description-changes branch from 4576e13 to 9dd09cd Compare April 2, 2018 16:18

IvanGoncharov reviewed Apr 2, 2018

View reviewed changes

IvanGoncharov force-pushed the master branch from e7509a6 to 16f5ca0 Compare June 26, 2019 17:05

IvanGoncharov mentioned this pull request Sep 17, 2019

Replace 'find{Breaking,Dangerous}Changes' with generic 'findSchemaChanges' #2181

Open

Base automatically changed from master to main January 27, 2021 11:10

IvanGoncharov mentioned this pull request Aug 10, 2021

Add 'diffSchema' utility function #3224

Open

IvanGoncharov closed this Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a utility to detect description changes #1127

Add a utility to detect description changes #1127

eapache commented Dec 7, 2017

leebyron left a comment

leebyron Dec 7, 2017

eapache Dec 7, 2017

leebyron Dec 7, 2017

leebyron Dec 7, 2017

eapache Dec 7, 2017

leebyron Dec 7, 2017

leebyron Dec 7, 2017

leebyron Dec 7, 2017

leebyron Dec 7, 2017

eapache Dec 7, 2017

odykyi Dec 27, 2017

eapache commented Jan 2, 2018

leebyron commented Jan 29, 2018

eapache commented Apr 2, 2018

IvanGoncharov Apr 2, 2018

eapache Apr 2, 2018

IvanGoncharov Apr 2, 2018 •

edited

Loading

eapache Apr 2, 2018

IvanGoncharov Apr 2, 2018

IvanGoncharov Apr 2, 2018

IvanGoncharov Apr 2, 2018

IvanGoncharov Apr 2, 2018

IvanGoncharov Apr 2, 2018

IvanGoncharov commented Apr 2, 2018 •

edited

Loading

eapache commented Apr 2, 2018

facebook-github-bot commented Jul 2, 2018

facebook-github-bot commented Jul 2, 2018

IvanGoncharov commented Aug 10, 2021

		@@ -0,0 +1,163 @@
		/* eslint-disable no-restricted-syntax */
		// @flow

Add a utility to detect description changes #1127

Add a utility to detect description changes #1127

Conversation

eapache commented Dec 7, 2017

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapache commented Jan 2, 2018

leebyron commented Jan 29, 2018

eapache commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Apr 2, 2018 • edited Loading

eapache commented Apr 2, 2018

facebook-github-bot commented Jul 2, 2018

facebook-github-bot commented Jul 2, 2018

IvanGoncharov commented Aug 10, 2021

IvanGoncharov Apr 2, 2018 •

edited

Loading

IvanGoncharov commented Apr 2, 2018 •

edited

Loading