typelevel / grackle

Grackle: Functional GraphQL for the Typelevel stack
http://typelevel.org/grackle/
Apache License 2.0
176 stars 25 forks source link

Schema validation failing when optional field is missing #638

Closed zakiahssn closed 2 months ago

zakiahssn commented 3 months ago

We have noticed an issue when upgrading to v19.0.0 (this also occurs in v20.0.0). We have the following type in our GraphQL Schema

type Contributors {
          directors: [PersonName!]
          actors: [PersonName!]
          narrators: [PersonName!]
          writers: [PersonName!]
        }

PersonName is described as follows

type PersonName {
          givenName: String!
          familyName: String!
        }

Contributors are referenced in the type Foo like so :

 type Foo {
          contributors: Contributors!
        }

Within the Foo type, Contributors is a required field i.e Contributors! however, within the the Contributors type all of it's fields are optional values and therefore according to GraphQL some fields may completely be omitted, so based on this understanding when asking for the following Contributors data via a query returning Foo, like so:

  contributors {
        directors{ givenName familyName }
        actors { givenName familyName }
        narrators { givenName familyName  }
        writers { givenName familyName }
      }

The expectation is that any of the following responses would be deemed valid due to the fields within Contributors being set to optional

"contributors": {}
"contributors": {"actors": []}
"contributors":{       
     "directors": []
     "actors": []
     "narrators": []
     "writers": []
    }
"contributors":{       
     "directors": ["givenName": "john",
              "familyName": "smith"]
     "actors": null
     "narrators": null
     "writers": ["givenName": "jane",
              "familyName": "smith"]
    }

The data corresponding to these types are stored in a column as a JSON blob in our database, see example below. We have noticed that after upgrading to grackle v19.0.0 , and running tests with this data the mapping now fails against the GraphQL Schema, due to narrators not being present:

          "actors": [],
          "writers": [
            {
              "givenName": "john",
              "familyName": "smith"
            }

          ],
          "directors": [
            {
              "givenName": "jane",
              "familyName": "smith"
            }
          ]
        }

It is our understanding of GraphQL, that the JSON from the DB should map correctly to the GraphQL response below:

contributors:{       
     directors: [{
             "givenName": "jane",
              "familyName": "smith"
            }],
     actors: [],
     narrators: null,
     writers: [{
              "givenName": "john",
              "familyName": "smith"
            }]
    }

However this doesn't work as expected, and grackle complains that narrators isn't present please see :

 java.lang.AssertionError: Obtained {"errors":[{"message":"No field 'narrators' for type Contributors"}],"data":null} data
Expected: a JSON object
     got: null
 ; 
Unexpected: errors

The messaging indicates that we need to specify a narrators field in our JSON object to either point to null or empty array for this to work. Backfilling the data like so causes the test to pass. However, we are not sure if this is desired behaviour as the fields within Contributors are Optional and therefore if the field doesn't exist in the JSON then the query should not fail, but the field should instead be omitted from the response i.e as described:

contributors:{       
     directors: [{
              "givenName": "jane",
              "familyName": "smith"
            }],
     actors: [],
     narrators: null,
     writers: [{
              "givenName": "john",
              "familyName": "smith"
            }]
    }

We didn't have this issue in v18.0.1, it started occuring in v19.0.0 even when upgrading to v20.0.0 the issue still persists. Please can this be looked into?

milessabin commented 3 months ago

You're right, this is a change in behaviour in 0.19.0 and it is intentional that you now have to provide an explicitly null or empty array value in those cases.

The previous behaviour where the missing field was omitted in the GraphQL response was violating the GraphQL spec, ie. it's not the case that "if the field doesn't exist in the JSON then the query should not fail, but the field should instead be omitted from the response" ... if a field is requested in the query, then it must be present in the response, as a null value or an empty array if that's permitted by the schema.

In principle it would be possible for the CirceMapping to synthesize these nulls or empty arrays, but the general consensus was that this had the potential to interfere with the mechanism for delegating to other mappings, and could also mask errors in the applications data representation, so on balance it was felt that providing null/empty fields should be done explicitly by the application.

zakiahssn commented 2 months ago

@milessabin Thanks for clarifying, will close this issue and look to make changes on our side to make this possible