search

supabase / pg_graphql

GraphQL support for PostgreSQL
https://supabase.github.io/pg_graphql
Apache License 2.0
2.9k stars 103 forks source link

Better domain type support #399

Open andrew-w-ross opened 1 year ago

andrew-w-ross commented 1 year ago

Describe the bug

Types for scalar domain types return type Opaque and for array domain types they just fail.

To Reproduce

Given the following schema:

create domain pos_int as int check (value > 0);
create domain non_empty_array as int[] check (array_length(value, 1) > 0);
create domain pos_array as pos_int[];

create table domain_test(
    id serial primary key,
    int_field int,
    array_field int[],
    pos_int pos_int,
    non_empty_array non_empty_array,
    pos_array pos_array
);

insert into domain_test(int_field, array_field, pos_int, non_empty_array, pos_array)
values (1, '{1,2,3}', 1, '{1,2,3}', '{1,2,3}');

Querying for the types on domain_test returns the following:

select graphql.resolve($$
query {
  __type(name: "domain_test") {
    fields {
      name
      type {
        name
        kind
        ofType {
          name
          kind
          ofType {
            name
            kind
          }
        }
      }
    }
  }
}
$$);
{
  "data": {
    "__type": {
      "fields": [
        {
          "name": "nodeId",
          "type": {
            "kind": "NON_NULL",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "ID",
              "ofType": null
            }
          }
        },
        {
          "name": "id",
          "type": {
            "kind": "NON_NULL",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        },
        {
          "name": "int_field",
          "type": {
            "kind": "SCALAR",
            "name": "Int",
            "ofType": null
          }
        },
        {
          "name": "array_field",
          "type": {
            "kind": "LIST",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        },
        {
          "name": "pos_int",
          "type": {
            "kind": "SCALAR",
            "name": "Opaque",
            "ofType": null
          }
        }
      ]
    }
  }
}

Querying the data will work for the fields on the Opaque types:

select graphql.resolve($$
query {
  domain_testCollection{
    edges {
        node {
            id
            int_field
            array_field
            pos_int
        }
    }
  }
}
$$);
{
  "data": {
    "domain_testCollection": {
      "edges": [
        {
          "node": {
            "id": 1,
            "pos_int": 1,
            "int_field": 1,
            "array_field": [
              1,
              2,
              3
            ]
          }
        }
      ]
    }
  }
}

And fails for fields with an array domain_type:

select graphql.resolve($$
query {
  domain_testCollection{
    edges {
        node {
            id
            non_empty_array
        }
    }
  }
}
$$);
{
  "data": null,
  "errors": [
    {
      "message": "Unknown field 'non_empty_array' on type 'domain_test'"
    }
  ]
}

You can attempt the above with pos_array and get a similar result.

Expected behavior

For fields with domain types to resolve to there domain type or at least the domain base type.

E.g. 

select graphql.resolve($$
query {
  __type(name: "domain_test") {
    fields {
      name
      type {
        name
        kind
        ofType {
          name
          kind
          ofType {
            name
            kind
          }
        }
      }
    }
  }
}
$$);

Would return:

{
  "data": {
    "__type": {
      "fields": [
        {
          "name": "nodeId",
          "type": {
            "kind": "NON_NULL",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "ID",
              "ofType": null
            }
          }
        },
        {
          "name": "id",
          "type": {
            "kind": "NON_NULL",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        },
        {
          "name": "int_field",
          "type": {
            "kind": "SCALAR",
            "name": "Int",
            "ofType": null
          }
        },
        {
          "name": "array_field",
          "type": {
            "kind": "LIST",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        },
        {
          "name": "pos_int",
          "type": {
            "kind": "SCALAR",
            "name": "Int",
            "ofType": null
          }
        },
        {
          "name": "non_empty_array",
          "type": {
            "kind": "LIST",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        },
        {
          "name": "pos_array",
          "type": {
            "kind": "LIST",
            "name": null,
            "ofType": {
              "kind": "SCALAR",
              "name": "Int",
              "ofType": null
            }
          }
        }
      ]
    }
  }
}

The behavior of the field itself on querying would be the same as it's base type.

Screenshots N/A

Versions:

Additional context I think I have a possible fix for this and I'l like to try opening a pr to fix this.

A simple solution would be to resolve the base type of the domain object itself. To my understanding domain types always have to eventually resolve to a built in sql scalar or array type so this seems like a valid solution. While this would technically be a breaking change Opaque isn't particularly useful so it could also hide behind a schema directive flag but that would be opt out. This is also a possible fix for #370.

Let me know if you agree and I'll submit a pr.

The complete fix would be to generate new types for domain types. That is breaking change and some client might not care to know about anything but the base type. It'll have to be behind a schema directive flag that's opt in. I won't be attempting this approach right away just spitballing a final solution.

olirice commented 1 year ago

Resolving to the base type of the domain makes sense to me

While this would technically be a breaking change

Moving from Opaque to a descriptive type is non-breaking. Opaque types use default postgres json serialization (vs always a string etc) and support the fewest operations (like filtering) for that reason

What breakage were you expecting?

olirice commented 1 year ago

Although I guess a domain based on a bigint (which we serialize as a string vs the default of an int) or json (doesn't support equality but Opaque does) could introduce a data format and API change respectively

andrew-w-ross commented 1 year ago

Good point I didn't even think about bigint.

I was mainly thinking about fields that weren't surfaced before, domain array types, will now all of a sudden show up. On a lesser extent it's a contract change for users doing introspection on the graphql types and have made workaround for Opaque.

I might be overly safe that's why I was going to put it behind a opt out schema config and then that could be removed on the next major release. It's your call just want to get requirements down before working on a pr.

olirice commented 1 year ago

yeah you're right. I scanned some of the public facing schemas on the platform and there would be impact

behind a opt out schema config

a schema level comment directive to enable domain types sounds great

If you create a PR, please also add it to the 2.0 breaking changes checklist

thanks!

andrew-w-ross commented 1 year ago

Will do, just to confirm I would like to do this in two pr's.

The first will be a very simple resolve to the base types for domain types. In the example I posted it would be a simply report and resolve to there base types.

E.g. pos_int would just be int and pos_array would be int[].

Putting the resolve base type behind a opt out of flag just in case someone is reliant on the old behaviour. This pr would be relatively quick and doesn't need a lot of documentation. Personally this partially unblocks my work and would also be a possible fix for #370.

The second pr would take a while longer and probably needs some further discussion and that would be to surface the domain types.

e.g. pos_int would resolve to pos_int ( or should it be PosInt?) on introspection and add it as a scalar type

This probably be opt in just in case clients only care about the base types. I'll raise a new issue for this once the resolve base type pr is in to hash out the particulars.

Does that work for you?

olirice commented 1 year ago

To roll it out safely I think

and in version 2.0 we remove the schema directive and make that default behavior

The second pr would take a while longer and probably needs some further discussion and that would be to surface the domain types.

e.g. pos_int would resolve to pos_int ( or should it be PosInt?) on introspection and add it as a scalar type

there would be no equivalent way to declare the array of positive integer example so I'm not sold on using the domain's name directly, but I'd love to hear any ideas you have

andrew-w-ross commented 1 year ago

there would be no equivalent way to declare the array of positive integer example so I'm not sold on using the domain's name directly, but I'd love to hear any ideas you have

I haven't given this too much thought yet but it could be embedded in a directive.

Something like directive @DomainType(typename: String!, elementType: String) on FIELD_DEFINITION.

And then it could be defined on the schema like so:

directive @DomainType(type: String!, elementType: String) on FIELD_DEFINITION

type domain_test {
  id: ID!

  pos_int: Int @DomainType('pos_int`)

  pos_int: [Int!] @DomainType('pos_array`,`pos_type`)
}

I haven't even tried this out so I don't know if it's valid or how the introspection would work. Might be a better solution for pr B as the type information is available for those that care and the types look the same for those that don't.

imor commented 1 year ago

I tried the same example above so see what other tools do. PostGraphile emits the following type:

type DomainTest {
  id: Int!
  intField: Int
  arrayField: [Int]
  posInt: PosInt
  nonEmptyArray: [Int]
  posArray: [PosInt]
}

Which looks like quite a reasonable solution to me.

there would be no equivalent way to declare the array of positive integer example so I'm not sold on using the domain's name directly, but I'd love to hear any ideas you have

I don't fully understand what is the concern here? @olirice could you please show an example?

Hasura produces the following type (which I don't like at all):

type domain_test {
  array_field: [Int!]
  id: Int!
  int_field: Int
  non_empty_array: _int4
  pos_array: _pos_int
  pos_int: Int
}
dvv commented 11 months ago

I attempted to step down to base domain types here https://github.com/supabase/pg_graphql/compare/master...dvv:pg_graphql:master It can help variables validation (String != Opaque) while we're thinking over the better domain type support.