Skip to content

Conversation

@bspeth
Copy link
Contributor

@bspeth bspeth commented Sep 23, 2022

Description

SchemaDiff had a single method checkTypeWithNonNullAndList that always applied the rule "non-null to nullable is safe; nullable to non-null is breaking."
This is true for input types and arguments, but the opposite should be true for fields on output types -- "nullable to non-null is safe; non-null to nullable is breaking."
A consumer operating on a nullable field should already be checking for presence, and adding an additional guarantee of non-null is safe.
A consumer operating on a non-null field may not be checking for presence, and removing the guarantee of non-null may break when that field is null.

Implementation

Two methods are defined -- one for input and argument types, and one for output types.
The methods are rewritten in a style of handling three top-level cases for the old type (non-null, list, and neither), and within each case handling the cases for the new type.
Calls to the previous method are updated to their respective new versions.

graphql-js implementation for reference:
isChangeSafeForObjectOrInterfaceField
isChangeSafeForInputObjectFieldOrFieldArg

Testing

Unit tests for the new methods are updated and added:

  • change_in_null_ness_object_or_interface
  • change_in_null_ness_input_or_arg
  • change_in_list_ness_object_or_interface
  • change_in_list_ness_input_or_arg

schema_ABaseLine:

  • Added Query.allCharactersByTemperament : [[Character]] to test a complex wrapped type.
  • Made Questor.beingID non-null to be able to test "non-null to nullable" for input.

schema_changed_input_object_fields:

  • Added case Query.being( .. , newArg: String!) -> stricter (moved from schema_changed_object_fields to here)
  • Added case Questor.beingID "required to optional" -> safe
  • Added case Questor.nestedInput "optional to required" -> stricter

schema_changed_object_fields:

  • Moved case Query.being( .. , newArg: String!) to schema_changed_object_fields
  • Added case Query.allCharactersByTemperament : [[Character!]]! to test complex wrapped type "optional to required" -> safe
  • Added case Being.ID "optional to required" -> safe
  • Added case Istari.temperament "required to optional" -> stricter

Brendan Speth added 4 commits September 21, 2022 15:15
Moved case Query.being( .. , newArg: String!) from schema_changed_object_fields to schema_changed_input_object_fields.
Added case Istari.temperament "non-null to nullable" -> stricter.
@bspeth
Copy link
Contributor Author

bspeth commented Sep 29, 2022

Hi @bbakerman, is this something you can help review? I see you originally wrote SchemaDiff and have looked at a couple other changes to it this year.

@bspeth
Copy link
Contributor Author

bspeth commented Oct 7, 2022

Hey @bbakerman, is there anything I can/should do to help with respect to this change?

Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Thanks for this excellent and well tested change

@bbakerman bbakerman added this to the 20.0 milestone Oct 9, 2022
@bbakerman bbakerman merged commit 8c94777 into graphql-java:master Oct 9, 2022
@bspeth
Copy link
Contributor Author

bspeth commented Oct 17, 2022

@bbakerman is there have a rough timeline for 20.0?
If it's still a few weeks or months out, can this be a candidate to port to 19.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants