fix(graphql): non-unique cursors (#95)

* fix(graphql): non-unique cursors

* test(nodes): add extra test cases

* chore(*): upgrade postgres

* chore(*): downgrade postgres

* fix(connection): fix failing tests

* test(pagination): fix tests…again

* doc(connection): add reasoning behind filtering out orderBy

* refactor(connection): use array length syntax for placeholder generation

examples/kitchen-sink/schema.sql

@@ -25,7 +25,10 @@ create table another_thing (
   thing_id         int references thing(id) on delete cascade
 );
 
+-- Do not add a primary key to this table.
 create table anything_goes (
+  foo              int,
+  bar              int,
   interval         interval
 );
 
@@ -159,4 +162,9 @@ insert into another_thing (note, published, tags, thing_id) values
   ('world', true, '{"c", "d"}', null),
   ('foo', false, '{"a"}', 2);
 
+insert into anything_goes (foo, bar) values
+  (1, 2),
+  (2, 3),
+  (3, 4);
+
 commit;

src/graphql/resolveConnection.js

@@ -8,6 +8,7 @@ const resolveConnection = (
   getFromClause = constant(table.getIdentifier()),
 ) => {
   const columns = table.getColumns()
+  const primaryKey = table.getPrimaryKeys()
 
   return (source, args, { client }) => {
     const { orderBy: orderByName, first, last, after, before, offset, descending, ...conditions } = args
@@ -27,8 +28,19 @@ const resolveConnection = (
     const orderBy = columns.find(({ name }) => orderByName === name)
     const fromClause = getFromClause(source, args)
 
+    // Here we take the full primary key for our table and filter out columns
+    // with the same name as the `orderBy`. This is because in some instances
+    // of PostgreSQL (specifically 9.5beta1) an operation like
+    // `(id, id) > (1, 1)` would *include* the row with an `id` of `1`. Even if
+    // this bug were a beta bug, however, it still makes sense to dedupe a
+    // comparison with a tuple like `(id, id)`.
+    const primaryKeyNoOrderBy = primaryKey.filter(({ name }) => name !== orderBy.name)
+
     // Get the cursor value for a row using the `orderBy` column.
-    const getRowCursorValue = row => row[orderBy.name] || ''
+    const getRowCursorValue = row => ({
+      value: row[orderBy.name],
+      primaryKey: primaryKeyNoOrderBy.map(({ name }) => row[name]),
+    })
 
     // Transforms object keys (which are field names) into column names.
     const getWhereClause = once(() => {
@@ -51,14 +63,36 @@ const resolveConnection = (
       return sql
     })
 
+    // Gets the condition for filtering our result set using a cursor.
+    const getCursorCondition = (cursor, operator) => {
+      const sql = new SQLBuilder()
+
+      const cursorCompareLHS = `(${
+        [orderBy.name, ...primaryKeyNoOrderBy.map(({ name }) => name)]
+          .map(name => `"${name}"`)
+          .join(', ')
+      })`
+
+      const cursorCompareRHS = `(${
+        // Here we only want to create a string of placeholders: `$1, $2, $3`
+        // etc. So we create an array of nulls of the appropriate length and
+        // then use the index (`i`) to generate the actual placeholder.
+        Array(1 + primaryKeyNoOrderBy.length).fill(null).map((x, i) => `$${i + 1}`).join(', ')
+      })`
+
+      sql.add(`${cursorCompareLHS} ${operator} ${cursorCompareRHS}`, [cursor.value, ...cursor.primaryKey])
+
+      return sql
+    }
+
     const getRows = once(async () => {
       // Start our query.
       const sql = new SQLBuilder().add('select * from').add(fromClause).add('where')
 
       // Add the conditions for `after` and `before` which will narrow our
       // range.
-      if (before) sql.add(`"${orderBy.name}" < $1 and`, [before])
-      if (after) sql.add(`"${orderBy.name}" > $1 and`, [after])
+      if (before) sql.add(getCursorCondition(before, '<')).add('and')
+      if (after) sql.add(getCursorCondition(after, '>')).add('and')
 
       // Add the conditions…
       sql.add(getWhereClause())
@@ -67,7 +101,13 @@ const resolveConnection = (
       // If a `last` argument was defined we are querying from the bottom so we
       // need to flip our order.
       const actuallyDescending = last ? !descending : descending
-      sql.add(`order by "${orderBy.name}" ${actuallyDescending ? 'desc' : 'asc'}`)
+
+      const orderings = [
+        `"${orderBy.name}" ${actuallyDescending ? 'desc' : 'asc'}`,
+        ...primaryKeyNoOrderBy.map(({ name }) => `"${name}" ${last ? 'desc' : 'asc'}`),
+      ].join(', ')
+
+      sql.add(`order by ${orderings}`)
 
       // Set the correct range.
       if (first) sql.add('limit $1', [first])
@@ -119,7 +159,9 @@ const resolveConnection = (
                 new SQLBuilder()
                 .add('select null from')
                 .add(fromClause)
-                .add(`where "${orderBy.name}" ${descending ? '<' : '>'} $1 and`, [endCursor])
+                .add('where')
+                .add(getCursorCondition(endCursor, descending ? '<' : '>'))
+                .add('and')
                 .add(getWhereClause())
                 .add('limit 1')
               )
@@ -142,7 +184,9 @@ const resolveConnection = (
                 new SQLBuilder()
                 .add('select null from')
                 .add(fromClause)
-                .add(`where "${orderBy.name}" ${descending ? '>' : '<'} $1 and`, [startCursor])
+                .add('where')
+                .add(getCursorCondition(startCursor, descending ? '>' : '<'))
+                .add('and')
                 .add(getWhereClause())
                 .add('limit 1')
               )

src/graphql/types.js

@@ -55,13 +55,21 @@ export const NodeType =
  * Connection Types
  * ========================================================================= */
 
+const serializeCursor = cursor =>
+  toBase64(JSON.stringify([cursor.primaryKey, cursor.value]))
+
+const deserializeCursor = serializedCursor => {
+  const [primaryKey, value] = JSON.parse(fromBase64(serializedCursor))
+  return { primaryKey, value }
+}
+
 export const CursorType =
   new GraphQLScalarType({
     name: 'Cursor',
     description: 'An opaque base64 encoded string describing a location in a list of items.',
-    serialize: toBase64,
-    parseValue: fromBase64,
-    parseLiteral: ast => (ast.kind === Kind.STRING ? fromBase64(ast.value) : null),
+    serialize: serializeCursor,
+    parseValue: deserializeCursor,
+    parseLiteral: ast => (ast.kind === Kind.STRING ? deserializeCursor(ast.value) : null),
   })
 
 export const PageInfoType =

tests/integration/fixtures/mutation-insert.json

@@ -8,19 +8,19 @@
         "note": "a"
       },
       "withArg": {
-        "cursor": "OA==",
+        "cursor": "W251bGwsbnVsbF0=",
         "node": {
           "id": "dGhpbmc6OA==",
-          "note": "a",
-          "rowId": 8
+          "rowId": 8,
+          "note": "a"
         }
       },
       "withoutArg": {
-        "cursor": "OA==",
+        "cursor": "W251bGwsbnVsbF0=",
         "node": {
           "id": "dGhpbmc6OA==",
-          "note": "a",
-          "rowId": 8
+          "rowId": 8,
+          "note": "a"
         }
       }
     },
@@ -32,19 +32,19 @@
         "note": "b"
       },
       "withArg": {
-        "cursor": "OQ==",
+        "cursor": "W251bGwsbnVsbF0=",
         "node": {
           "id": "dGhpbmc6OQ==",
-          "note": "b",
-          "rowId": 9
+          "rowId": 9,
+          "note": "b"
         }
       },
       "withoutArg": {
-        "cursor": "OQ==",
+        "cursor": "W251bGwsbnVsbF0=",
         "node": {
           "id": "dGhpbmc6OQ==",
-          "note": "b",
-          "rowId": 9
+          "rowId": 9,
+          "note": "b"
         }
       }
     },
@@ -65,19 +65,19 @@
         }
       },
       "relationEdge": {
-        "cursor": "OA==",
+        "cursor": "W251bGwsbnVsbF0=",
         "node": {
           "aThingId": 8,
           "bThingId": 9,
           "thingByAThingId": {
             "id": "dGhpbmc6OA==",
-            "note": "a",
-            "rowId": 8
+            "rowId": 8,
+            "note": "a"
           },
           "thingByBThingId": {
             "id": "dGhpbmc6OQ==",
-            "note": "b",
-            "rowId": 9
+            "rowId": 9,
+            "note": "b"
           }
         }
       }

tests/integration/fixtures/nodes.graphql

@@ -16,12 +16,39 @@ query Nodes {
   e: thingNodes(orderBy: NOTE, descending: true) {
     edges { cursor, node { ...thing } }
   }
-  f: thingNodes(before: "NQ==") {
+  f: thingNodes(before: "W1tdLDVd") {
     nodes { ...thing }
   }
-  g: thingNodes(after: "NA==") {
+  g: thingNodes(after: "W1tdLDRd") {
     nodes { ...thing }
   }
+  h: thingNodes(orderBy: LUCKY_NUMBER, after: "W1s2XSw2Nl0=") {
+    edges {
+      cursor
+      node { ...thing, luckyNumber }
+    }
+  }
+  # Test a table that does not have a primary key.
+  i: anythingGoesNodes(orderBy: FOO, after: "W1tdLDFd") {
+    edges {
+      cursor
+      node {
+        foo
+        bar
+      }
+    }
+  }
+  # If we only checked cursor values and not primary keys as well, this would
+  # fail.
+  j: relationNodes(orderBy: A_THING_ID, after: "W1syXSwxXQ==") {
+    edges {
+      cursor
+      node {
+        aThingId
+        bThingId
+      }
+    }
+  }
 }
 
 fragment thing on Thing {