charlypoly / graphql-to-json-schema

GraphQL Schema to JSON Schema
MIT License
202 stars 25 forks source link

Nullable fields, arguments or list elements types should be array containing "null"? #30

Closed newhouse closed 3 years ago

newhouse commented 3 years ago

Consider the following GraphQL SDL:

type Foo {
  bar: String
}

The Field bar on the Type Foo is of type String...which can be described in English as:

The type Foo has a field named bar that can be either a String or null/empty/undefined (perhaps this is slightly incorrect, but I think it's close enough)

(All of this is opposed to if the type of bar was String!, which would mean it should always have a String value)

Currently, the above SDL will result in a JSON Schema from this library that looks like the following:

{
  ...
  definitions: {
    Foo: {
      type: 'object',
      properties: {
        bar: {
          '$ref': '#/definitions/String,
          type: 'string'
        },
      },
      required: [],
    }
  }
}

But, I believe in JSON Schema that actually says something more like:

Foo has a property bar that will be a string

Perhaps this is fine the way it is, and the fact that required: [] is an empty array signals that bar may be null/empty/undefined?

But if not, perhaps the output should be more like this:

{
  ...
  definitions: {
    Foo: {
      type: 'object',
      properties: {
        bar: {
          '$ref': '#/definitions/String,
          // This is the change
          type: ['null', 'string']
        },
      },
      required: [],
    }
  }
}

But let's consider another SDL:

type Foo {
  bar: [String]
}

In English, this is something like:

Type Foo has a field bar that may be null/empty, or it may be an array of String elements, but some of those can be null/empty as well

The JSON Schema this library outputs in this case looks like so:

{
  ...
  definitions: {
    Foo: {
      type: 'object',
      properties: {
        bar: {
          type: 'array',
          items: { 
            '$ref': '#/definitions/String', 
            type: 'string'
          }
        }
      },
      required: []
    },
  }
}

Which I think says that the items in the bar array can NOT be null...which does not play nice with the GraphQL Schema type definition.

I think that JSON Schema standards would say it should look something like this:

{
  ...
  definitions: {
    Foo: {
      type: 'object',
      properties: {
        bar: {
          type: 'array',
          items: { 
            '$ref': '#/definitions/String', 
            // This is the change
            type: ['null', 'string']
          }
        }
      },
      required: []
    }
  }
}

Anyways...I think you see where I'm going with all this, and would like to hear your thoughts...

charlypoly commented 3 years ago

Looks good!

Let's good with the following approach:

const result = fromIntrospectionQuery(introspection, { nullableArrayValues: true });
charlypoly commented 3 years ago

Note: should we have a flag for all nullable types? wrapping all nullable types in a anyOf