sequelizeConnection will break on any string primary keys containing $

[jedwards1211]

Imagine that the primary keys of a table are strings and the row at index 30 of the results has the primary key FU$BAR.

The cursor comes out to base64('arrayconnection$FU$BAR$30'). fromCursor will then split at the wrong $ and return {id: 'FU', index: 'BAR'} instead of {id: 'FU$BAR', index: 30}.

Oops! 💩

I don't understand why anyone would use an arbitrary (hence more error-prone) delimiting scheme when they could just JSON.stringify([id, index]) and JSON.parse it back out. JSON is hard to improve upon.

Problem source code in relay.js

  let toCursor = function (item, index) {
    let id = item.get(item.constructor ? item.constructor.primaryKeyAttribute : item.Model.primaryKeyAttribute);
    return base64(PREFIX + id + SEPERATOR + index);
  };

  let fromCursor = function (cursor) {
    cursor = unbase64(cursor);
    cursor = cursor.substring(PREFIX.length, cursor.length);
    let [id, index] = cursor.split(SEPERATOR);
     ...
  };

[mickhansen]

I don't understand why anyone would use an arbitrary (hence more error-prone) delimiting scheme

Code mostly lifted from https://github.com/graphql/graphql-relay-js/blob/master/src/connection/arrayconnection.js

[mickhansen]

As for not using JSON, i imagine that size was considered in relation to mobile.

[jedwards1211]

Yeah, but the size difference is very minor. Ignorning the prefix, which seems like an unnecessary waste of size, for the common integer id case, it's 123$456 vs. [123,456], only two extra characters per cursor. For the string id case it's "FU$BAR$456" vs. ["FU$BAR",456], only four extra characters. There could be a few more extra characters if something needs to be escaped in the string, but the current strategy should be adding something to escape $ in string ids anyway.

[intellix]

While you're on the subject of size savings, why not shorten "arrayconnection"? Perhaps it'll likely get gzipped

[jedwards1211]

@intellix my PR gets rid of it entirely