graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.63k stars 572 forks source link

Questions based on initial experience with v5 polymorphism #2077

Closed FelixZY closed 6 months ago

FelixZY commented 6 months ago

Summary

I started experimenting a bit with the new polymorphism in v5 tonight and found myself wondering about a few different things.

Additional context

PostgreSQL: docker - postgis/postgis:16-3.4-alpine Node: 20.13.1 Postgraphile: 5.0.0-beta.26

Setup:

graphile.config.mjs ```js import { PostGraphileAmberPreset } from "postgraphile/presets/amber"; import { PostGraphileRelayPreset } from "postgraphile/presets/relay"; import { makePgService } from "postgraphile/adaptors/pg"; import { cleanEnv, str, host, port } from "envalid"; const env = cleanEnv(process.env, { GQL_HOST: host(), GQL_PORT: port(), DB_APP_API_USER: str(), DB_APP_API_PASSWORD: str(), DB_NAME: str(), DB_HOST: host(), DB_PORT: port(), NODE_ENV: str({ choices: ["development", "test", "production", "staging"] }), GRAPHILE_ENV: str({ choices: ["development", "test", "production", "staging"], }), }); const connectionString = `postgres://${encodeURIComponent(env.DB_APP_API_USER)}:${encodeURIComponent(env.DB_APP_API_PASSWORD)}@${env.DB_HOST}:${env.DB_PORT}/${env.DB_NAME}`; let superuserConnectionString = undefined; if (env.NODE_ENV === "development") { const superEnv = cleanEnv(process.env, { DBMS_OWNER_USER: str(), DBMS_OWNER_PASSWORD: str(), }); superuserConnectionString = `postgres://${encodeURIComponent(superEnv.DBMS_OWNER_USER)}:${encodeURIComponent(superEnv.DBMS_OWNER_PASSWORD)}@${env.DB_HOST}:${env.DB_PORT}/${env.DB_NAME}`; } /** @type {GraphileConfig.Preset} */ const preset = { extends: [PostGraphileAmberPreset, PostGraphileRelayPreset], pgServices: [ makePgService({ connectionString, superuserConnectionString, pubsub: true, schemas: ["dansdata", "portal"], }), ], gather: { pgStrictFunctions: true, installWatchFixtures: true, }, schema: { dontSwallowErrors: true, jsonScalarAsString: false, pgForbidSetofFunctionsToReturnNull: true, }, grafserv: { host: env.GQL_HOST, port: env.GQL_PORT, graphiql: true, graphqlOverGET: false, graphqlPath: "/graphql", graphiqlPath: "/", watch: env.NODE_ENV === "development", }, grafast: { explain: env.NODE_ENV === "development", }, }; export default preset; ```
seed.sql ```sql CREATE SCHEMA if NOT EXISTS "dansdata"; CREATE TYPE dansdata.profile_type AS ENUM ('individual'); CREATE TABLE dansdata.profiles ( id UUID DEFAULT gen_random_uuid () PRIMARY KEY, type profile_type NOT NULL, name TEXT NOT NULL ); comment ON TABLE dansdata.profiles IS $$ Represents an entity with profile data. @interface mode:relational type:type @type individual references:individuals @name Profile $$; CREATE TABLE dansdata.individuals ( id UUID PRIMARY KEY REFERENCES profiles (id) ON DELETE CASCADE ); comment ON TABLE dansdata.individuals IS $$ Represents an individual person. @name Individual $$; ``` I also tried the `@interface mode:union`/`@implements Profile` version of the above (without the `profiles.type` column).

1. Include example queries in documentation

The documentation is quite clear in how one should use the smart comments to configure polymorphism. Good work!

I am however, missing examples of what effect this has on the syntax of graphql queries. For example, I'm currently forced to write

query {
  allIndividuals {
    nodes {
      id
      profileByRowId {
        name
      }
    }
  }
}

whereas I would expect to write

query {
  allIndividuals {
    nodes {
      id
      name
    }
  }
}

I cannot figure out if I'm doing something wrong or if this is the intended behavior as the documentation does not describe the intended GraphQL syntax.


2. PostGraphileRelayPreset does not eliminate Row in ByRowId

PostGraphileRelayPreset is suggested as a way to hide the ugly rowId from your schema. This does work. However, connections are still generated using the ByRowId syntax. Even worse, it seems generated ByRowId connections do not actually expect the rowId after enabling PostGraphileRelayPreset! Instead, the value returned from the id is expected. This becomes confusing very quickly.

Current:

mutation {
  createIndividual(
    input: {individual: {profileByRowId: "WyJQcm9maWxlIiwiZjVmOWNiNWEtN2Y1Mi00MzgyLWE3ZDgtMzdkOWZhY2Q5NjJkIl0="}}
  ) {
    individual {
      id
    }
  }
}

Expected

mutation {
  createIndividual(
    input: {individual: {profileById: "WyJQcm9maWxlIiwiZjVmOWNiNWEtN2Y1Mi00MzgyLWE3ZDgtMzdkOWZhY2Q5NjJkIl0="}}
  ) {
    individual {
      id
    }
  }
}

3. I would like to drop the ByRowId when accessing parent properties

Not sure how to do this. I'm guessing I'd have to write some kind of custom plugin? It would be nice if it was the default though.

Current:

query {
  allIndividuals {
    nodes {
      id
      profileByRowId {
        name
      }
    }
  }
}

Expected

query {
  allIndividuals {
    nodes {
      id
      profile {
        name
      }
    }
  }
}

Even better

query {
  allIndividuals {
    nodes {
      id
      name
    }
  }
}

4. I can't find a way to create a Profile when creating an Individual

In my system, a Profile is always one of a few given types. It is unexpected that a Profile should be created without an associated e.g. Individual. Preferably, I would like to remove createProfile from my mutations altogether and expose only e.g. createIndividual. However, this means createIndividual needs to be able to create the base Profile, which it seems is not possible?

Current

mutation {
  createIndividual(input: {individual: {
    # This is the only property available in `individual`.
    profileByRowId {
      # There are no properties available here for creating a profile
    }
  }}) {
    id
  }
}

Expected

mutation {
  createIndividual(input: {individual: {
    profile: {
      name: "Felix"
    }
  }}) {
    id
  }
}

5. Custom startup text in graphiql

I'm planning to use Postgraphile to expose a free API for third parties. I would therefore like to change the default text in graphiql/Ruru to link to related resources. Is this possible?

Default text ```gql # Welcome to Ruru, our distribution of GraphiQL and related tooling to # inspect your GraphQL API. # # GraphiQL is an in-browser tool for writing, validating, and # testing GraphQL queries. # # Type queries into this side of the screen, and you will see intelligent # typeaheads aware of the current GraphQL type schema and live syntax and # validation errors highlighted within the text. # # GraphQL queries typically start with a "{" character. Lines that starts # with a # are ignored. # # An example GraphQL query might look like: # # { # field(arg: "value") { # subField # } # } # # Keyboard shortcuts: # # Prettify Query: Shift-Ctrl-P (or press the prettify button above) # # Merge Query: Shift-Ctrl-M (or press the merge button above) # # Run Query: Ctrl-Enter (or press the play button above) # # Auto Complete: Ctrl-Space (or just start typing) # ```
FelixZY commented 6 months ago
  1. can be addressed using the @graphile/simplify-inflection plugin. See also https://postgraphile.org/postgraphile/next/inflection#advice
benjie commented 6 months ago

Point 1

Hi @FelixZY; thanks for the great reproduction! I trimmed it down a bit further so I wouldn't need to provide a bunch of envvars, and I had to tweak your seed.sql file as it missed the schema in a couple places, but it was quite easy to reproduce your issue.

It was a simple one, smart comments must be the first things in a comment, but you had them as the last things. (See below for fixed SQL.) This was visible in GraphiQL as the smart tags were coming through into the documentation when you hovered over fields, whereas they should have been removed. Once I resolved this, I then faced the issue that you called your type profile_type and we already generate a type ProfileType to represent the possible types of the interface Profile and that caused issues. Ideally we'd use the same enum, but we're not smart enough for that yet (please feel free to file a separate issue if you're so inclined). So I had to write a quick inflection plugin to rename the builtin type to ProfilePolyType; I suggest you replace that with something more suitable (or disable PgPolymorphismOnlyArgumentPlugin):

 import { PostGraphileAmberPreset } from "postgraphile/presets/amber";
 import { PostGraphileRelayPreset } from "postgraphile/presets/relay";
 import { makePgService } from "postgraphile/adaptors/pg";

 const connectionString = `postgres:///felixzy`;

 /** @type {GraphileConfig.Preset} */
 const preset = {
   extends: [PostGraphileAmberPreset, PostGraphileRelayPreset],
   pgServices: [
     makePgService({
       connectionString,
       superuserConnectionString: connectionString,
       pubsub: true,
       schemas: ["dansdata"],
     }),
   ],
   gather: {
     pgStrictFunctions: true,
     installWatchFixtures: true,
   },
   schema: {
     dontSwallowErrors: true,
     jsonScalarAsString: false,
     pgForbidSetofFunctionsToReturnNull: true,
   },
   grafserv: {
     graphiql: true,
     graphqlOverGET: false,
     watch: true,
   },
   grafast: {
     explain: true,
   },
+  plugins: [
+    {
+      name: "RenamePolymorphismOnlyTypePlugin",
+      inflection: {
+        replace: {
+          pgPolymorphismEnumType(prev, options, pgCodec) {
+            return this.upperCamelCase(`${this._codecName(pgCodec)}-poly-type`);
+          },
+        },
+      },
+    },
+  ],
 };

 export default preset;

Here's the SQL I used:

DROP SCHEMA if EXISTS "dansdata" CASCADE;
CREATE SCHEMA "dansdata";

CREATE TYPE dansdata.profile_type AS ENUM ('individual');

CREATE TABLE dansdata.profiles (
  id UUID DEFAULT gen_random_uuid () PRIMARY KEY,
  type dansdata.profile_type NOT NULL,
  name TEXT NOT NULL
);

comment ON TABLE dansdata.profiles IS $$
  @interface mode:relational type:type
  @type individual references:individuals
  @name Profile

  Represents an entity with profile data.
$$;

CREATE TABLE dansdata.individuals (
  id UUID PRIMARY KEY REFERENCES dansdata.profiles (id) ON DELETE CASCADE
);

comment ON TABLE dansdata.individuals IS $$
  @name Individual

  Represents an individual person.
$$;

Once you get this working, submitting some examples to the documentation would be appreciated!

Point 2

You should consider using @graphile/simplify-inflection; but you're right that's a super weird naming issue. Please file a separate issue about it :heart: Filed as https://github.com/graphile/crystal/issues/2082 (In general, each issue should be filed in a separate issue; it makes them easier to track and address.)

Point 3

Hopefully @graphile/simplify-inflection will address that.

Point 4

I didn't think polymorphic types supported any CRUD mutations; if they do then this is a bug, please file it as a separate issue filed as https://github.com/graphile/crystal/issues/2083.

Point 5

I think so; please use a plugin like:

https://github.com/graphile/crystal/blob/91e87ab6516490a4cc7b7fc6400efb7623fbd331/postgraphile/postgraphile/graphile.config.ts#L85-L102

TypeScript should give you some guidance. If you get it working, I'd love it if you could contribute an example back to the docs.


Closing this since issue 1 is dealt with, please file any separate issues as separate issues :heart:

FelixZY commented 6 months ago

Thanks a lot @benjie ! Sorry about the "multi-bug" - I wasn't sure any one of them were significant enough to post about. I'll be happy to provide some PRs once I'm done with my current queue :)

FelixZY commented 6 months ago

4

I didn't think polymorphic types supported any CRUD mutations; if they do then this is a bug

I don't know if they do - based on your feedback on 1., I probably did not configure polymorphism properly.

benjie commented 6 months ago

Ah 🤦‍♂️ I should have thought of that! Closing #2083; please re-open if it does turn out to be an issue.

FelixZY commented 5 months ago

For future reference: the PgPolymorphismOnlyArgumentPlugin has description "Adds the 'only' argument to polymorphic relations to limit to only the given types". See also https://github.com/graphile/crystal/blob/73fd6f0422d174d43b689ed76a34123122825c8e/graphile-build/graphile-build-pg/src/plugins/PgPolymorphismOnlyArgumentPlugin.ts#L37