Sorting by multiple fields about graphile-engine HOT 12 CLOSED

The whole defaultValue thing is a bit of a hack (as you can probably see); I'd consider a PR that improved it somehow - I'm not sure what you have in mind exactly.

We definitely need a better flow for replacement plugins; at the moment I'd recommend copying and pasting the entire thing into a new plugin, editing as you see fit, and then when you load the plugins inform the system to NOT load the original plugin. It would be nice if we came up with some kind of replaces: PgConnectionArgOrderBy or similar configuration thing, but I can imagine this being problematic... Needs more thought.

would you rather see an orderByMany plugin implemented as a PR or maintained separately?

I would like to keep the core as lean as possible; as much as possible additional plugins should be in their own npm modules/GitHub repos. If it turns out that a majority of people want a particular plugin in core we will look at merging it in once it's stable and proven, probably involving a major version bump.

from graphile-engine.

I'm not big on the replacement plugins approach.. once you start removing and reimplementing core plugins like OrderBy, it starts to feel like maintaining a fork instead of a plugin. So if possible, I'd like to find a way for PgConnectionArgOrderBy and PgConnectionArgOrderByMany to coexist, just like my custom filter plugin and the core condition plugin do.

My main question is.. what purpose is the defaultValue actually serving? When I drop it from the PgConnectionArgOrderBy and run the full test suite, the only failure is the postgraphile-core schema test where the default values get stripped out on all the OrderBy types (obviously). But I presume it's serving some other purpose that isn't covered by the current test suite.

from graphile-engine.

If memory serves, it's because otherwise the cursors don't make sense because the results from the database aren't ordered so Postgres can return them in any order. But it was quite a few months ago now so it may have been for a different reason.

from graphile-engine.

Ahhhh; no it was to maintain as much compatibility with v3 as possible:

https://github.com/postgraphql/postgraphql/blob/master/src/postgraphql/__tests__/__snapshots__/postgraphqlIntegrationSchema-test.js.snap#L107

Basically I tried to minimise all the diffs in all the tests. e.g.:

https://github.com/postgraphql/postgraphql/pull/506/files#diff-c12cf6b4454a6f09ca43c15fa9936592 (click to show diff)

from graphile-engine.

Gotcha. If minimizing those diffs is still a goal for v4, would you consider a PR that factored out the defaultValue assignment into a PgConnectionArgOrderByDefaultValue core plugin? With that in place, I could drop PgConnectionArgOrderByDefaultValue, keep PgConnectionArgOrderBy, and append a new PgConnectionArgOrderByMany plugin.

from graphile-engine.

Yes. But first; remove the default value logic and ensure that the queries generated still order by the primary key by default. I think they will because I'm pretty sure I added it as a fallback.

Use DEBUG="*:sql" envvar to see SQL queries.

If they don't then we'll need to solve that before you can factor it out.

from graphile-engine.

It appears to still order by the primary key by default. Using postgraphile v4.0.0-alpha2.26 with just this line removed, the following query:

query {
  allPosts {
    nodes {
      id
    }
  }
}

run against the forum_example schema produces:

with __local_0__ as (
      select to_json((__local_1__."id")) as "id"
      from "forum_example"."post" as __local_1__
      
      where (TRUE) and (TRUE)
      order by __local_1__."id" ASC
      
      
    ), __local_2__ as (select json_agg(to_json(__local_0__)) as data from __local_0__) select coalesce((select __local_2__.data from __local_2__), '[]'::json) as "data"

This is where the fallback is implemented, correct?

from graphile-engine.

Great - I thought I had it handled but it always pays to check.

I really need to get around to tidying up the SQL at some point 😳

from graphile-engine.

After reading about List Input Coercion in the GraphQL spec, I realized that's it possible for a single orderBy argument to support both single values and arrays, eliminating the need for a separate orderByMany argument.

Technically, support for passing an array to orderBy could be implemented as a non-breaking improvement, but it would cause a bunch of diffs in the schema tests.. (all instances of orderBy: TableOrderBy = PRIMARY_KEY_ASC would become something like orderBy: [TableOrderBy!] = [PRIMARY_KEY_ASC]) so that's not an option for v4.

I can think of a few ways to handle this:

1. Replace the core OrderBy plugin with a non-core plugin that supports array inputs. This would definitely work, but users would have to invoke replaceAllPlugins, at least until a replacePlugins function is added to graphile-build. It's also more of a maintenance burden.
2. Update the core OrderBy plugin to handle array inputs, but don't actually change the type of the orderBy argument. Then a simple non-core plugin could be written to allow array inputs, simply wrapping the orderBy types with new GraphQLList(new GraphQLNonNull(...)).
3. Update the core OrderBy plugin to handle array inputs, and allow setting the orderBy type with an option such as enableOrderByMany (defaulting to false).

Thoughts?

from graphile-engine.

Additional background here: graphql/graphql-relay-js#20 (comment)

from graphile-engine.

So long as none of people's existing queries will break then I don't see that as a breaking change even if the diffs change. I had noticed this behaviour of GraphQL before but had not realised it was deliberate - just thought it was a fluke! I should read the spec more often!

Basically if there's a query that's like this:

query Foo($orderBy: TableOrderBy) {
  allFoos(orderBy: $orderBy) {
    nodes { id }
  }
}

And that will still execute when orderBy changes from orderBy: TableOrderBy to orderBy:[TableOrderBy!] then I'm good with it.

I'm trying to keep diff changes to a minimum but there's no requirement that they be zero.

from graphile-engine.

Closed by #144

from graphile-engine.