Try and maintain ordering of functions

.changeset/serious-donuts-bathe.md

@@ -0,0 +1,9 @@
+---
+"graphile-build-pg": patch
+"postgraphile": patch
+"@dataplan/pg": patch
+---
+
+Prevents inlining (via joins) child PgSelect queries into parents when the
+parent is relying on implicit ordering coming from a function or suitably
+flagged subquery.

grafast/dataplan-pg/src/datasource.ts

@@ -200,6 +200,7 @@ export interface PgResourceOptions<
   isUnique?: boolean;
   sqlPartitionByIndex?: SQL;
   isMutation?: boolean;
+  hasImplicitOrder?: boolean;
   /**
    * If true, this indicates that this was originally a list (array) and thus
    * should be treated as having a predetermined and reasonable length rather
@@ -233,6 +234,7 @@ export interface PgFunctionResourceOptions<
   uniques?: TUniques;
   extensions?: PgResourceExtensions;
   isMutation?: boolean;
+  hasImplicitOrder?: boolean;
   selectAuth?:
     | (($step: PgSelectStep<PgResource<any, any, any, any, any>>) => void)
     | null;
@@ -284,6 +286,7 @@ export class PgResource<
   public readonly description: string | undefined;
   public readonly isUnique: boolean;
   public readonly isMutation: boolean;
+  public readonly hasImplicitOrder: boolean;
   /**
    * If true, this indicates that this was originally a list (array) and thus
    * should be treated as having a predetermined and reasonable length rather
@@ -324,6 +327,7 @@ export class PgResource<
       isUnique,
       sqlPartitionByIndex,
       isMutation,
+      hasImplicitOrder,
       selectAuth,
       isList,
       isVirtual,
@@ -341,6 +345,7 @@ export class PgResource<
     this.isUnique = !!isUnique;
     this.sqlPartitionByIndex = sqlPartitionByIndex ?? null;
     this.isMutation = !!isMutation;
+    this.hasImplicitOrder = hasImplicitOrder ?? false;
     this.isList = !!isList;
     this.isVirtual = isVirtual ?? false;
     this.selectAuth = selectAuth;
@@ -443,6 +448,7 @@ export class PgResource<
       uniques,
       extensions,
       isMutation,
+      hasImplicitOrder,
       selectAuth: overrideSelectAuth,
       description,
     } = overrideOptions;
@@ -463,6 +469,7 @@ export class PgResource<
         extensions,
         isUnique: !returnsSetof,
         isMutation: Boolean(isMutation),
+        hasImplicitOrder,
         selectAuth,
         description,
       };
@@ -486,6 +493,7 @@ export class PgResource<
         extensions,
         isUnique: false, // set now, not unique
         isMutation: Boolean(isMutation),
+        hasImplicitOrder,
         selectAuth,
         isList: true,
         description,
@@ -515,6 +523,7 @@ export class PgResource<
         isUnique: false, // set now, not unique
         sqlPartitionByIndex,
         isMutation: Boolean(isMutation),
+        hasImplicitOrder,
         selectAuth,
         description,
       };

grafast/dataplan-pg/src/steps/pgSelect.ts

@@ -225,6 +225,16 @@ export interface PgSelectOptions<
    * `resource.from`.
    */
   from?: SQL | ((...args: PgSelectArgumentDigest[]) => SQL);
+  /**
+   * You should never rely on implicit order - use explicit `ORDER BY` (via
+   * `$select.orderBy(...)`) instead. However, if you _are_ relying on implicit
+   * order in your `from` result (e.g. a subquery or function call that has its
+   * own internal ordering), setting this to `true` will prevent PgSelect from
+   * inlining some queries (joins) that it thinks might impact the order of
+   * results. Setting this to `true` does NOT guarantee that you can rely on
+   * your order being maintained, but it does increase the chances.
+   */
+  hasImplicitOrder?: false;
 
   /**
    * If you pass a custom `from` (or otherwise want to aid in debugging),
@@ -274,6 +284,7 @@ export class PgSelectStep<
   private readonly from:
     | SQL
     | ((...args: Array<PgSelectArgumentDigest>) => SQL);
+  private readonly hasImplicitOrder: boolean;
 
   /**
    * This defaults to the name of the resource but you can override it. Aids
@@ -481,6 +492,7 @@ export class PgSelectStep<
         identifiers,
         args: inArgs,
         from: inFrom = null,
+        hasImplicitOrder: inHasImplicitOrder,
         name: customName,
         mode: inMode,
         joinAsLateral: inJoinAsLateral = false,
@@ -493,6 +505,7 @@ export class PgSelectStep<
               resource: optionsOrCloneFrom.resource,
               identifiers: null,
               from: optionsOrCloneFrom.from,
+              hasImplicitOrder: optionsOrCloneFrom.hasImplicitOrder,
               args: null,
               name: optionsOrCloneFrom.name,
               mode: undefined,
@@ -549,6 +562,7 @@ export class PgSelectStep<
       : new Map();
     this.alias = cloneFrom ? cloneFrom.alias : sql.identifier(this.symbol);
     this.from = inFrom ?? resource.from;
+    this.hasImplicitOrder = inHasImplicitOrder ?? resource.hasImplicitOrder;
     this.placeholders = cloneFrom ? [...cloneFrom.placeholders] : [];
     this.placeholderValues = cloneFrom
       ? new Map(cloneFrom.placeholderValues)
@@ -2506,6 +2520,12 @@ ${lateralText};`;
             continue;
           }
 
+          // Don't want to make this a join as it can result in the order being
+          // messed up
+          if (t2.hasImplicitOrder && !this.joinAsLateral && this.isUnique) {
+            continue;
+          }
+
           /*
           if (!planGroupsOverlap(this, t2)) {
             // We're not in the same group (i.e. there's probably a @defer or

graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts

@@ -417,6 +417,10 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
           const namespaceName = namespace.nspname;
           const procName = pgProc.proname;
 
+          // TODO: use smart tags to override this one way or the other?
+          // Perhaps `@forceOrder` or `@ignoreOrder`?
+          const hasImplicitOrder = returnsSetof;
+
           const sqlIdent = info.helpers.pgBasics.identifier(
             namespaceName,
             procName,
@@ -516,6 +520,7 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
               returnsArray,
               returnsSetof,
               isMutation,
+              hasImplicitOrder,
               extensions,
               description,
             };
@@ -551,6 +556,7 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
                 executor,
                 extensions,
                 fromCallback,
+                hasImplicitOrder,
                 identifier,
                 isMutation,
                 name,
@@ -567,6 +573,7 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
                 codec: returnCodec,
                 uniques: [],
                 isMutation,
+                hasImplicitOrder,
                 extensions,
                 description,
               }),
@@ -575,6 +582,7 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
                 executor,
                 extensions,
                 fromCallback,
+                hasImplicitOrder,
                 identifier,
                 isMutation,
                 name,

postgraphile/postgraphile/__tests__/kitchen-sink-data.sql

@@ -77,6 +77,8 @@ delete from partitions.users cascade;
 
 delete from d.post cascade;
 delete from d.person cascade;
+delete from issue_2210.test_message cascade;
+delete from issue_2210.test_user cascade;
 
 alter table b.types enable trigger user;
 
@@ -1008,3 +1010,15 @@ insert into space.static_pad(name) select i::text from generate_series(1, 10) i;
 insert into space.temp_pad(name) select i::text from generate_series(1, 10) i;
 insert into space.spacecraft(name, return_to_earth) select i::text, tsrange((date_trunc('day', '2024-03-13T12:00:00Z'::timestamptz) - (i+1) * interval '1 day')::timestamp, (date_trunc('day', '2024-03-13T12:00:00Z'::timestamptz) - (i) * interval '1 day')::timestamp, '[)') from generate_series(1, 10) i;
 
+insert into issue_2210.test_user (id, name)
+values ('a13b8bac-f2c7-4444-bac6-4ae7c9c28bbc', 'Bob')
+     , ('935945c1-d824-4a98-93e5-c22215c58982', 'John');
+
+insert into issue_2210.test_message (id, test_chat_id, message, test_user_id, created_at)
+values ('e0849772-7070-4fdf-8438-1ef846fc0daf', '0d126c0c-9710-478c-9aee-0be34b250573', 'Bob says 3', 'a13b8bac-f2c7-4444-bac6-4ae7c9c28bbc', '2020-01-01T00:00:00Z'::timestamptz - '1 minute'::interval)
+     , ('c8a660af-7021-4360-b019-ee404014b3cb', '0d126c0c-9710-478c-9aee-0be34b250573', 'Bob says 2', 'a13b8bac-f2c7-4444-bac6-4ae7c9c28bbc', '2020-01-01T00:00:00Z'::timestamptz - '3 minutes'::interval)
+     , ('6e2db5cb-8757-4b8a-9d19-a6a676a214d2', '0d126c0c-9710-478c-9aee-0be34b250573', 'John says 3', '935945c1-d824-4a98-93e5-c22215c58982', '2020-01-01T00:00:00Z'::timestamptz - '2 minutes'::interval)
+     , ('7dbc5c82-3c1f-463e-a97a-aaff09dc8a28', '0d126c0c-9710-478c-9aee-0be34b250573', 'Bob says 1', 'a13b8bac-f2c7-4444-bac6-4ae7c9c28bbc', '2020-01-01T00:00:00Z'::timestamptz - '5 minutes'::interval)
+     , ('5751f977-209d-45ab-8620-b647ff67ded6', 'c46b4b59-0a29-4211-8e0f-659cb3e01c2f', 'A different chat', 'a13b8bac-f2c7-4444-bac6-4ae7c9c28bbc', '2020-01-01T00:00:00Z'::timestamptz - '3 minutes 30 seconds'::interval)
+     , ('8b9e89dc-2e1b-461a-94d5-3afafa4f87ad', '0d126c0c-9710-478c-9aee-0be34b250573', 'John says 2', '935945c1-d824-4a98-93e5-c22215c58982', '2020-01-01T00:00:00Z'::timestamptz - '4 minutes'::interval)
+     , ('cc20ffeb-0701-4619-acc3-4a9b67671272', '0d126c0c-9710-478c-9aee-0be34b250573', 'John says 1', '935945c1-d824-4a98-93e5-c22215c58982', '2020-01-01T00:00:00Z'::timestamptz - '6 minutes'::interval)

postgraphile/postgraphile/__tests__/kitchen-sink-schema.sql

@@ -21,7 +21,8 @@ drop schema if exists
   nested_arrays,
   composite_domains,
   refs,
-  space
+  space,
+  issue_2210
 cascade;
 drop extension if exists tablefunc;
 drop extension if exists intarray;
@@ -2002,3 +2003,32 @@ AS $$
 BEGIN
     RETURN $1.return_to_earth;
 END $$;
+
+--------------------------------------------------------------------------------
+
+create schema issue_2210;
+
+create table issue_2210.test_user (
+  id uuid primary key
+, name varchar
+, created_at timestamptz default now()
+);
+
+create table issue_2210.test_message (
+  id uuid primary key
+, test_chat_id uuid
+, test_user_id uuid references issue_2210.test_user (id)
+, message text
+, created_at timestamptz
+);
+create index test_message_test_user_id_idx on issue_2210.test_message(test_user_id);
+
+create function issue_2210.some_messages (test_chat_id uuid) returns setof issue_2210.test_message as $$
+  select m.*
+  from issue_2210.test_message m
+  where m.test_chat_id = $1
+  order by created_at desc;
+$$ language sql stable;
+
+comment on table issue_2210.test_message is E'@behaviour -connection';
+comment on function issue_2210.some_messages(uuid) is E'@behaviour +connection';

postgraphile/postgraphile/__tests__/queries/v4/issue2210.json5

@@ -0,0 +1,64 @@
+{
+  someMessages: {
+    nodes: [
+      {
+        id: "<UUID 1>",
+        message: "Bob says 3",
+        createdAt: "2019-12-31T19:59:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 2>",
+          name: "Bob",
+        },
+      },
+      {
+        id: "<UUID 3>",
+        message: "John says 3",
+        createdAt: "2019-12-31T19:58:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 4>",
+          name: "John",
+        },
+      },
+      {
+        id: "<UUID 5>",
+        message: "Bob says 2",
+        createdAt: "2019-12-31T19:57:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 2>",
+          name: "Bob",
+        },
+      },
+      {
+        id: "<UUID 6>",
+        message: "John says 2",
+        createdAt: "2019-12-31T19:56:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 4>",
+          name: "John",
+        },
+      },
+      {
+        id: "<UUID 7>",
+        message: "Bob says 1",
+        createdAt: "2019-12-31T19:55:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 2>",
+          name: "Bob",
+        },
+      },
+      {
+        id: "<UUID 8>",
+        message: "John says 1",
+        createdAt: "2019-12-31T19:54:00.000000-04:00",
+        testUserByTestUserId: {
+          id: "<UUID 4>",
+          name: "John",
+        },
+      },
+    ],
+    pageInfo: {
+      hasNextPage: false,
+      endCursor: "WyJuYXR1cmFsIiw2XQ==",
+    },
+  },
+}