graphile / crystal-pre-merge Goto Github PK

View Code? Open in Web Editor NEW

39.0 39.0 10.0 43.07 MB

Repository moved

Home Page: https://github.com/graphile/crystal

crystal-pre-merge's People

Contributors

Stargazers

Watchers

Forkers

francescovenica dmfay elob vince-smith csnweb logamithran nestarz b9r5 delcaos drewwestphal

crystal-pre-merge's Issues

Unlogged tables should have a default `-select -insert -update -delete` behavior

graphile/crystal#1135

Table inheritance

At least make sure it's handled in a reasonable fashion.

Consider integrating it into polymorphism.

Possibly of interest to @TimoStolz

grafserv should only take one argument (the second one)?

Tests for makePgSmartTagsPlugin and derivatives

I ported this over but haven't even checked it runs, let alone works, so it definitely needs testing!

Order of columns: use json_populate_recordset rather than records directly?

We should ensure that PostGraphile depends as little as possible on the explicit column order. The reason this is significant is when dealing with a record object; e.g. a table create table person (id int, name text) would have the record representation (1,Alice)), but if the columns were in a different order, it would instead be (Alice,1). Note that Postgres does not indicate the column names when doing this.

Currently PostGraphile uses these records in rare locations, but doing so could cause problems for people who use a migration system where the migrations can run in different orders - this can lead to out-of-order columns, which means that an export from one server may not match the schema on another. (Graphile Migrate does not have this problem, but any migration framework that does not assert that the migrations must run in one particular could.)

One way to solve this would be to use json_populate_recordset instead of using records directly. It's a bit more work for client and server, but is safer. Maybe we can enable this only in the case that the schema is exported?

Have steps implement `export` / `hydrate` methods or similar

Since planning is expensive it would be nice if planning could happen in a background thread, and then rehydrated on the main thread. Further, this would allow caching plans to e.g. redis so that servers don't need to re-plan existing operations the first time they see them.

Doesn't even need to be the whole step - just what we need at runtime. Could even be that we export the plan as JS function source code.

Some queries crash with Cannot read properties of undefined (reading '0')

First, congrats on the soft launch of Postgraphile v5! I'm extremely excited about it, the performance is impressive and the DX looks awesome!

Not sure if issues are welcome yet, but just in case, I wanted to report my finding. Absolutely no pressure to fix it, I just want to help here. This might also be a known bug.

Overview

I found a case of the following error:

/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:410
                            const val = store[planId][index];
                                                     ^

TypeError: Cannot read properties of undefined (reading '0')
    at processListChildren (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:410:54)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:438:25)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:445:29)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:445:29)
    at produceOutput (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:356:17)
    at executeBucket (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:74:16)
    at produceOutput (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:379:27)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

To reproduce

Clone the pagila repo and start the db in docker

git clone https://github.com/devrimgunduz/pagila.git
cd pagila
docker run -p 127.0.0.1:5432:5432/tcp --name postgres -e POSTGRES_PASSWORD=secret -d postgres
sleep 10
echo "CREATE DATABASE pagila;\n\\q" | docker exec -i postgres psql -U postgres
cat ./pagila-schema.sql | docker exec -i postgres psql -U postgres -d pagila
cat ./pagila-data.sql | docker exec -i postgres psql -U postgres -d pagila

Setup postgraphile

In a terminal, on this repo, run the recommended

yarn
yarn watch

Start the `fastify-helix-envelop` example

Change the config.ts to have:

const DATABASE_CONNECTION_STRING =
  "postgresql://postgres:[email protected]:5432/pagila";
const DATABASE_SCHEMAS: string[] = ["public", "app_public"];

Run the fastify-helix-envelop example

$ node dist/examples/fastify-helix-envelop.js
Don't understand how to build type for bpchar
Don't understand how to build type for bytea
Don't understand how to build type for tsvector


Running a GraphQL API server.
For Graphile Inspect (GraphQL IDE), see     http://localhost:4000/
Issue GraphQL queries via HTTP POST to  http://localhost:4000/graphql

Run a successful query

Running this query from graphiql works and gives correct results:

{
  allFilms(first: 100) {
    totalCount
    edges {
      cursor
      node {
        title
        description
        inventoriesByFilmId {
          edges {
            node {
              storeId
              storeByStoreId {
                storeId
              }
            }
          }
        }
      }
    }
  }
  allStores {
    edges {
      node {
        storeId
        staffByStoreId {
          edges {
            node {
              firstName
              lastName
            }
          }
        }
      }
    }
  }
}

Run a failing query

Running this query from graphiql crashes (Note that it's very similar to the previous query):

{
  allFilms(first: 100) {
    totalCount
    edges {
      cursor
      node {
        title
        description
        inventoriesByFilmId {
          edges {
            node {
              storeId
              storeByStoreId {
                storeId
                # THIS PART IS NEW =====================
                staffByStoreId {
                  edges {
                    node {
                      firstName
                      lastName
                    }
                  }
                }
                # END OF NEW PART  ===================
              }
            }
          }
        }
      }
    }
  }
  allStores {
    edges {
      node {
        storeId
        staffByStoreId {
          edges {
            node {
              firstName
              lastName
            }
          }
        }
      }
    }
  }
}

Here is the stack trace:

$ node dist/examples/fastify-helix-envelop.js
Don't understand how to build type for bpchar
Don't understand how to build type for bytea
Don't understand how to build type for tsvector


Running a GraphQL API server.
For Graphile Inspect (GraphQL IDE), see     http://localhost:4000/
Issue GraphQL queries via HTTP POST to  http://localhost:4000/graphql

/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:410
                            const val = store[planId][index];
                                                     ^

TypeError: Cannot read properties of undefined (reading '0')
    at processListChildren (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:410:54)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:438:25)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:445:29)
    at processObject (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:445:29)
    at produceOutput (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:356:17)
    at executeBucket (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:74:16)
    at produceOutput (/Users/vincent/Code/postgraphile-private/packages/dataplanner/dist/execution-v2.js:379:27)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Node.js v17.8.0

See if it's possible to have TypeScript autocomplete disablePlugins names

Relations should be a separate thing

Currently relations belong to sources; however they really ought to belong to codecs (it doesn't matter where you get a row from: table, function, whatever; if it has an organization_id column then you should be able to get the organization associated with it).

However, relations can't exist until all the sources exist, so we have PgSourceBuilder as like a "this will be a PgSource, but it isn't right now" placeholder so that we can do circular relations. It's all weird and yucky.

Then at runtime, half the time we're talking about codecs, rather than sources, and they don't have relations. So currently we're doing hacky nonsense where we find a table-like source for that codec and pull the relations out of that. It's really wrong and unpleasant.

Solution: remove relations from sources; instead create them as a later phase and put then onto build.input as well - so now we get build.input.pgSources and build.input.pgRelations. Relations will relate a codec to a source - doesn't matter where the original row came from, but when fulfilling it we have to get it from somewhere (a source).

We're still going to want things like source.getRelations() to work, not 100% sure how to address that right now - perhaps pass the input object into this method...

Anyway, I'm going to keep the hacky way for now so I can keep making progress, but I'm really looking forward to getting rid of PgSourceBuilder!

Codec tweaks: domains to represent type modifiers

TYPES.char represents char type. char[4] should use an anonymous (or named... doesn't matter?) "domain" over TYPES.char. Domains should be nestable. We should have a helper function that strips all the domains from a type, or allows something like isCodec(char4, TYPES.char).

This is particularly important for plugins like postgraphile-plugin-connection-filter that use the codecs to decide what behaviors to expose.

loadOne and loadMany should use a consistent item step?

const $user = loadOne(...);

const $users = loadMany(...);
return $users.each($user => {
  ...
});

The $user in both these example should have the same step class, but currently the former uses LoadOneStep and the latter LoadManySingleRecordStep. This means that for the fields on the User type, the plan resolver's first argument, $user, is a mess:

User.firstName = {
  type: GraphQLString,
  plan($user: LoadOneStep<...> | LoadManySingleRecordStep<...>) {
    return $user.get('first_name');
  }
}

Yuck.

Maybe LoadManyStep's listItem callback should use a LoadOneStep item in a special mode. Or maybe they should both defer to a new LoadSingleStep, but LoadOneStep doesn't really need an extra step for that and it unnecessarily grows the plan diagram.

🤔

Bug in `loadOne` with tamedevil causing issues with `users-and-friends` example

Really surface makeExtendSchemaPlugin in the docs

Users new to PostGraphile see a lot of options for changing the schema: makeExtendSchemaPlugin, makeWrapResolversPlugin, operation-hooks, etc etc. For those unfamiliar, it's hard to know which one to use.

makeExtendSchemaPlugin is the primary way that people should consider extending the GraphQL schema in JS/TS, so we should make sure it enters their consciousness early.

json5 dependency missing in graphile-utils?

Default behavior should not be applied at read time

The behavior of something should be concrete. If something has the behavior "list" then it shouldn't also have the behavior "filter" in one place but not in another because they use different defaults.

... or should it?

Currently we do build.behavior.matches(behaviorString, desiredBehavior, defaultBehavior) and plugins might feed different options for that default behavior. Maybe, instead, there should be a categorization phase where we add behaviors to everything, and then this defaultBehavior won't be required at match-time (and the final "behavior" strings will be inspectable).

Test pgSingleTablePolymorphic more

This comment:

https://github.com/benjie/postgraphile-private/blob/8d67fd17ef4b7889c45d6cd5ca3f14aab66b824f/grafast/dataplan-pg/src/steps/pgSingleTablePolymorphic.ts#L75-L76

Indicates that we may need to do more. But I don't remember why. We should push this from various directions and see if it breaks. If not, remove the TODO. If yes, having a test that shows the issue would be a great first step to fixing it.

Fix `loadOne` docs; bad `getUsersByIds` - confusing

Check SQL: full entity selected in addition to fields from it in `unions-table/bookmarks`?

https://github.com/benjie/postgraphile-private/blob/3d7d3f3345cf9bfcc840b1366b2c0bad6866e724/grafast/dataplan-pg/__tests__/queries/unions-table/bookmarks.sql#L15-L18

Probably the earlier expression should have been dropped. Needs checking.

Seriously need to spend some time optimizing planning

I've deliberately not expended any effort on optimizing the planning phase so far - focusing purely on execution. But planning is super important too, especially for servers handling dynamic queries. I'm sure there are places where I should be able to get 100x improvements in performance due to having done repeated re-computation of the same things over and over again for ease of development.

A place to start would be with postgraphile/postgraphile/__tests__/queries/v4/types.test.graphql which currently takes ~15s (15000ms!) to plan but then executes future queries in ~25ms.

Either force `.get` API support or use `.get` if available for default plan resolvers

Using access() does not leave any tracks; if you have const $user = loadOne() and you return that for a field result, the subfields' default plans should cause the loadOne to know which attributes were requested.

It may make sense to enforce that all plans that serve an object field return an "object capable plan" - that is one where the .get API is supported. Alternatively we could have the default plan resolver use .get if it's available, but fall back to access if not.

Add 'runExampleSchema' to CI

Want to ensure that anything that breaks running the exported schema should also break CI.

Partitioned tables

Ensure these are handled correctly/reasonably.

Replace *SimpleFieldName smart tags

We now have listField and connectionField inflectors; so lets get rid of foreignFieldName/foreignSimpleFieldName and similar tags - foreignFieldName should be used for both, then they should be fed through listField/connectionField.

Maybe.

`(select json_agg(_) from (`

_ is confusing, use a different letter.

SQL procedures (not functions) - need tests, etc

PgRelationsPlugin needs a test using "unsafe" column names for a relation

We have an error that will be thrown here if any columns are unsafe:

https://github.com/benjie/postgraphile-private/blob/31d9eea5667ac8aff205f8d591a415e516912f31/graphile-build/graphile-build-pg/src/plugins/PgRelationsPlugin.ts#L970-L974

Unsafe column names are things like __proto__ and constructor that have special meanings in JavaScript.

Theoretically these should fall back to safer (but more verbose) handling, but because we don't have tests I haven't enabled that code.

Someone needs to add to the tests:

two tables that relate by constructor fields on both the local and remote table
GraphQL test queries that query this relationship in both directions

If you're successful, the error containing UUID 94fe5fe8-74a4-418b-93ea-beac09b64b5d should be thrown. If so, try deleting the highlighted block above and re-running the tests. Check that they're successful, and if so, send a PR with the test and with the 5 lines above removed.

Minor snapshot improvements

Make SQL statements end in semicolons
Add newlines to ends of files

Watch fixtures are a mess

HT @TimoStolz

Need a "superuserConnectionString" or similar to install them, probably.
This maybe means that executors need a "superuser" mode. And maybe a "database owner" mode. These might be useful beyond this feature.
Watch fixtures should be installed for each executor, not for each pgSource. (Maybe even figure out the unique databases that the executors relate to - e.g. turn them all into connection strings and then unique those?)
The warning on failure should be more helpful.

Need HStore tests

See grafast/dataplan-pg/src/codecUtils/hstore.ts

Also make sure that __proto__ key is tested.

Alternative node-postgres adaptor that uses `SET` rather than `SET LOCAL`

Our set_config(..., ..., true) is great because it unsets at the end of a transaction; however we currently do:

begin;
select set_config(el->>0, el->>1, true) from json_array_elements($1::json) el;
...
commit;

around each withPgClient call that uses settings.

We could instead use global settings:

select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
...
reset all;

This would save us a single roundtrip.

We could save another roundtrip by removing the RESET ALL but a) we'd need to track every setting that had been made and be sure to clear those that the new $1 doesn't set, and b) this would prevent us sharing a PG pool between PostGraphile and another service (e.g. Worker) because the settings would leak between them. If we're happy with those tradeoffs, we could actually benefit more - it's reasonably likely that the same user will trigger another withPgClient call, maybe even just a tick later! We could see if we have a client for the same claims, and if so we could just reuse the client again:

-- withPgClient for user1
reset all;
select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user1
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user1
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user2
reset all;
select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
... -- ACTUAL QUERIES WE NEED HERE

Note how there's the initial 2 roundtrips, but then future calls to withPgClient don't need any extra roundtrips until a new set of claims is needed.

Polymorphism is branching, possibly unnecessarily

https://github.com/benjie/postgraphile-private/blob/c099aa4a8a0897981dc9abe67ebb0649a6cce37e/grafast/grafast/src/engine/OperationPlan.ts#L1420-L1421

This branching might not be good - especially if the position in the document is the same for all the polymorphisms. If we had one bucket with all the different mutually exclusive steps in, and the "root step" for a polymorphic bucket could be dependent on the polymorphic path, then we could make it so that we can do "polymorphic join" steps to undo branching and re-coalesce.

For example; imagine you're pulling down a field that's polymorphic: a Post or a Topic. Both of these have an author, and if we request author on each we'll get a User, so we should be able to "merge back together" and handle any fragments on this user in a similar way.

{
  item { # Post or Topic - branches polymorphism
    id
    author { # a User, no matter whether it was a post or topic, i.e. _could_ join polymorphism back together.
      friends { totalCount }
    }
  }
}

"identifiers" inefficient when casting to json

We have:

 (ids.value->>0)::"json" as "id0"

However, we should be able to just do:

 (ids.value->0)::"json" as "id0"

which would avoid casting to text only to cast straight back to JSON again.

FieldArgs needs to be a generic

In Grafast, we have plan (and subscribePlan) on fields, equivalent to GraphQL's resolve (and subscribe). But in addition to that, we also give the ability to attach plan resolvers to field arguments and input object fields too. These take two forms:

inputPlan() - used when "getting" an argument - can be used to transform a value from GraphQL into an internal representation (e.g. if you have a UserInput { dateOfBirth: String } the equivalent internal representation might be { date_of_birth: string }) - this is rarely useful for Grafast users directly, but is very handy for tools like PostGraphile
applyPlan() - use to apply side effects from the argument/field to (typically) the parent - e.g. first: Int might have applyPlan($pgSelect, val) { $pgSelect.setFirst(val.get()) } - heavily used by postgraphile-plugin-connection-filter for example, as the plans can build up a layer at a time and then when complete can apply to the parent $pgSelect in order to add the filter clause/etc - and this allows the filtering concern to be handled by the argument entirely, rather than needing the main plan to know about filter's existence (which, since filter comes from a plugin, the plan doesn't/can't).

Each of these plan resolvers (plan(), subscribePlan(), applyPlan() and inputPlan()) are passed a FieldArgs object that represents the input at that stage. For a field (plan()/subscribePlan()) the FieldArgs represents all the arguments of the field; for an argument it represents the value of that specific argument, for an input object field it represents the value of that input object field, and so on down the tree.

FieldArgs has three methods:

getRaw(path?) - get a step representing exactly what the user supplied at the given "input path". This is necessary in order to be able to evaluate the value (adding a constraint) if the plan needs to change based on an argument
get(path?) - like getRaw() except it invokes the inputPlan() (if any) which may transform the underlying value
apply($target, path?) - applies the applyPlan() at path path (or itself if no path) to $target

At the moment, path is allowed to be omitted, a single string, or an array of string/number indexes. It is NOT strongly typed - you can provide paths that don't make sense.

Similarly the result of getRaw / get are not strongly typed (they are InputStep and ExecutableStep respectively).

On top of this, we also enable a shortcut to getRaw, so rather than const $username = fieldArgs.getRaw(['input', 'user', 'username']) you can do const { $input: { $user: { $username } } } = fieldArgs;. Currently this is also not strongly typed, so any $... keys are allowed, and we tell you it'll be an InputStep even though it might be undefined (e.g. if you access a field/argument that doesn't actually exist).

If we make FieldArgs generic then we should be able to make it so that the user (or codegen!) can supply a type (probably based on the GraphQL definition of the arguments/etc) and then the user will get an error if they try and access a path/key/etc that doesn't exist. But we must make it so that this doesn't make programming too painful for people who don't want a codegen step. Ideally if writing the schema by hand, generics shouldn't need to be passed because they can be inferred.

Relevant links:

FieldArgs type definition: https://github.com/benjie/postgraphile-private/blob/32b561f230dc338b6faf816cca42d23378a4fd25/grafast/grafast/src/interfaces.ts#L323-L333

FieldArgs implementation: https://github.com/benjie/postgraphile-private/blob/32b561f230dc338b6faf816cca42d23378a4fd25/grafast/grafast/src/operationPlan-input.ts#L66 and https://github.com/benjie/postgraphile-private/blob/32b561f230dc338b6faf816cca42d23378a4fd25/grafast/grafast/src/operationPlan-input.ts#L344

An example of somewhere being poorly typed (in a hand-written mega-schema):

https://github.com/benjie/postgraphile-private/blob/32b561f230dc338b6faf816cca42d23378a4fd25/grafast/dataplan-pg/src/examples/exampleSchema.ts#L5050-L5054

Ruru: add "download schema" button

https://discord.com/channels/489127045289476126/498852330754801666/1055838852461371493

Integrate Node's http2 server

Mermaid diagrams should make streamed nodes more obvious - use subgraphs?

More tests for polymorphism

v4/procedure-computed-fields has redundant `left outer join lateral`

Same join twice; should be auto-eradicated and aliases amalgamated.

Create a test for apply on object fields inside a list

bcf2eb2

This commit needs tests to avoid regressions.

Was produced when testing https://github.com/graphile-contrib/postgraphile-plugin-connection-filter/blob/v5/__tests__/fixtures/queries/logicalOperators.graphql

Rename "smart comments" to "smart tag comments" to make equivalence clearer?

"Smart tags" are the over-arching feature.

"Smart comments" ("smart tag comments") are how to apply smart tags to things using database comments.

"Smart tag comments" is a bit of a mouthful...

Run docusaurus link check in CI

Maybe just yarn build of each website?

Don't sort by nullables by default

Related: graphile/graphile-engine#502

Remove `@deprecated` TypeScript interfaces before release

docs: Examples gallery needs importing

The gallery from here: https://www.graphile.org/postgraphile/examples/

Bug when executing interface with zero implementations

Surprised GraphQL even allows this to exist. Anyway; we shouldn't fail if it does.

Grafserv: Different server implementations

We need help with testing the implementations of various servers with Grafserv

express / connect
koa
fastify
Next.js
lambda
any other server you're using

For the server(s) you use, it would be useful if you could answer some of these, with (1) being the most important and least effort and (4) the most effort.

What middlewares/modules do you use with the server (compression, cookie parsing, body parsing, rate limiting, result masking, ...) - specifically what module names, and how are they configured. Do you have custom middleware you'd want Grafserv to honour? What is it?
Can you run Grafserv inside your existing stack (e.g. as a new endpoint) - we can supply a trivial config and schema.
Can you extend Benjie's examples to use your extended configs? Please submit PR.
Can you write a test for your setup?

Error filtering

A recent change means that planning errors are now exposed via field errors rather than being raised to the entire operation. We should have a way of flagging that the error raised (e.g. thrown inside an applyPlan on an input object field) is safe to send to the user, and should assume not by default. Maybe a special error class, maybe just a property on the error.