Skip to content

Cursor Reconfigure For Order #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 19, 2016
Merged

Cursor Reconfigure For Order #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 40 additions & 50 deletions src/relay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,6 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
});
}

let defaultOrderBy = orderByEnum._values[0].value;

before = before || ((options) => options);

let $connectionArgs = {
Expand All @@ -143,20 +141,30 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
return orderBy[0][0];
};

let toCursor = function (value, orderBy) {
let id = value.get(model.primaryKeyAttribute);
let orderValue = value.get(orderByAttribute(orderBy));
return base64(PREFIX + id + SEPERATOR + orderValue);
/**
* Creates a cursor given a item returned from the Database
* @param {Object} item sequelize model instance
* @param {Integer} index the index of this item within the results, 0 indexed
* @return {String} The Base64 encoded cursor string
*/
let toCursor = function (item, index) {
let id = item.get(model.primaryKeyAttribute);
return base64(PREFIX + id + SEPERATOR + index);
};

/**
* Decode a cursor into its component parts
* @param {String} cursor Base64 encoded cursor
* @return {Object} Object containing ID and index
*/
let fromCursor = function (cursor) {
cursor = unbase64(cursor);
cursor = cursor.substring(PREFIX.length, cursor.length);
let [id, orderValue] = cursor.split(SEPERATOR);
let [id, index] = cursor.split(SEPERATOR);

return {
id,
orderValue
index
};
};

Expand All @@ -171,13 +179,12 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
return result;
};

let resolveEdge = function (item, args = {}, source) {
if (!args.orderBy) {
args.orderBy = [defaultOrderBy];
}

let resolveEdge = function (item, index, queriedCursor, args = {}, source) {
let startIndex = 0;
if (queriedCursor) startIndex = Number(queriedCursor.index);
if (startIndex !== 0) startIndex++;
Copy link
Owner

@mickhansen mickhansen Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following exactly what goes on here, if we receive a cursor we generate a new cursor?

Copy link
Contributor Author

@brad-decker brad-decker Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a cursor supplied to this function, we are simply getting the index of that cursor to determine where to start numbering the next indexes. Because index in this case is going to always start at 0 we need to increment startIndex if we aren't dealing with a startIndex of 0.

Ex: we get the first three items from query, they will have a index of 0,1 and 2 in their cursor returned from the api. If we then send the cursor of the 2 to the after clause, the next 3 returned will start numbering at 2. Without the startIndex++ check the next set of results would be 2, 3 and 4 which would be wrong.

Copy link
Owner

@mickhansen mickhansen Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course. Maybe we should rename cursor to queriedCursor

Copy link
Contributor Author

@brad-decker brad-decker Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and renamed this to be clearer. I am holding off on committing until I can make any final changes we may need.

return {
cursor: toCursor(item, args.orderBy),
cursor: toCursor(item, index + startIndex),
node: item,
source: source
};
Expand All @@ -187,11 +194,10 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
handleConnection: false,
include: true,
list: true,
before: function (options, args, context, info) {
before: function (options, args, context) {
if (args.first || args.last) {
options.limit = parseInt(args.first || args.last, 10);
}

if (!args.orderBy) {
args.orderBy = [orderByEnum._values[0].value];
} else if (typeof args.orderBy === 'string') {
Expand Down Expand Up @@ -233,43 +239,22 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu

if (args.after || args.before) {
let cursor = fromCursor(args.after || args.before);
let orderValue = cursor.orderValue;
let startIndex = Number(cursor.index);

if (model.rawAttributes[orderAttribute].type instanceof model.sequelize.constructor.DATE) {
orderValue = new Date(orderValue);
}

let slicingWhere = {
$or: [
{
[orderAttribute]: {
[orderDirection === 'ASC' ? '$gt' : '$lt']: orderValue
}
},
{
[orderAttribute]: {
$eq: orderValue
},
[model.primaryKeyAttribute]: {
$gt: cursor.id
}
}
]
};

// TODO, do a proper merge that won't kill another $or
_.assign(options.where, slicingWhere);
if (startIndex > 0) options.offset = startIndex + 1;
}

// apply uniq to the attributes
options.attributes = _.uniq(options.attributes);
return before(options, args, root, context);
},
after: function (values, args, root, {source}) {
var cursor = null;

if (args.after || args.before) {
cursor = fromCursor(args.after || args.before);
}

return before(options, args, context, info);
},
after: function (values, args, context, {source}) {
let edges = values.map((value) => {
return resolveEdge(value, args, source);
let edges = values.map((value, idx) => {
return resolveEdge(value, idx, cursor, args, source);
});
Copy link
Owner

@mickhansen mickhansen Feb 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the index not be incorrect if we're already querying after a cursor? It would be 0 where it should be N + 0

Copy link
Contributor Author

@brad-decker brad-decker Feb 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolveEdge function takes care of the starting index. Though it could be modified instead of passing the cursor to the function to instead do all the calculations here and pass the resulting index to resolveEdge. Might be more efficient as well.

Copy link
Owner

@mickhansen mickhansen Feb 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, you're taking the existing cursor and applying based on that.


let firstEdge = edges[0];
Expand All @@ -282,6 +267,11 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
if (model.sequelize.dialect.name === 'postgres' && (args.first || args.last)) {
if (fullCount === null || fullCount === undefined) throw new Error('No fullcount available');
}
let hasMorePages = false;
if (args.first || args.last) {
let index = cursor ? Number(cursor.index) : 0;
hasMorePages = index + 1 + parseInt(args.first || args.last, 10) < fullCount;
}

return {
source,
Expand All @@ -291,8 +281,8 @@ export function sequelizeConnection({name, nodeType, target, orderBy: orderByEnu
pageInfo: {
startCursor: firstEdge ? firstEdge.cursor : null,
endCursor: lastEdge ? lastEdge.cursor : null,
hasPreviousPage: args.last !== null && args.last !== undefined ? fullCount > parseInt(args.last, 10) : false,
hasNextPage: args.first !== null && args.first !== undefined ? fullCount > parseInt(args.first, 10) : false,
hasPreviousPage: hasMorePages,
hasNextPage: hasMorePages
}
};
}
Expand Down
114 changes: 106 additions & 8 deletions test/integration/relay/connection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import attributeFields from '../../../src/attributeFields';
import resolver from '../../../src/resolver';
import {uniq} from 'lodash';


const {
sequelize,
Promise
Expand All @@ -18,13 +19,10 @@ import {
} from '../../../src/relay';

import {
GraphQLString,
GraphQLInt,
GraphQLFloat,
GraphQLNonNull,
GraphQLBoolean,
GraphQLEnumType,
GraphQLList,
GraphQLObjectType,
GraphQLSchema,
graphql
Expand Down Expand Up @@ -121,9 +119,19 @@ if (helper.sequelize.dialect.name === 'postgres') {
values: {
ID: {value: [this.Task.primaryKeyAttribute, 'ASC']},
LATEST: {value: ['createdAt', 'DESC']},
CUSTOM: {value: ['updatedAt', 'DESC']},
NAME: {value: ['name', 'ASC']}
}
}),
before: (options) => {
if (options.order[0][0] === 'updatedAt') {
options.order = Sequelize.literal(`
CASE
WHEN completed = true THEN "createdAt"
ELSE "updatedAt" End ASC`);
}
return options;
},
Copy link
Owner

@mickhansen mickhansen Feb 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply add the literal to the value of the ENUM i believe.

Copy link
Contributor Author

@brad-decker brad-decker Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't work for me because literal returns an object with {val : }

Copy link
Owner

@mickhansen mickhansen Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, i believe i have that in one of my projects. Having an object shouldn't matter, the value in ENUM can be anything AFAIR.

Copy link
Contributor Author

@brad-decker brad-decker Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

      let orderBy = args.orderBy;
      let orderAttribute = orderByAttribute(orderBy);
      let orderDirection = args.orderBy[0][1];```

specifically this part of the connections configuration prevents that case because its looking directly for an array value. I tried setting it up directly in the order configuration first and it failed.

Copy link
Owner

@mickhansen mickhansen Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah alright, was sure i tried that out, but maybe i used a hook too.

connectionFields: () => ({
totalCount: {
type: GraphQLInt,
Expand Down Expand Up @@ -193,12 +201,12 @@ if (helper.sequelize.dialect.name === 'postgres') {
orderBy: new GraphQLEnumType({
name: 'Viewer' + this.Task.name + 'ConnectionOrder',
values: {
ID: {value: [this.Task.primaryKeyAttribute, 'ASC']},
ID: {value: [this.Task.primaryKeyAttribute, 'ASC']}
}
}),
before: (options, args, root) => {
before: (options, args, context, {viewer}) => {
options.where = options.where || {};
options.where.userId = root.viewer.get('id');
options.where.userId = viewer.get('id');
return options;
Copy link
Owner

@mickhansen mickhansen Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be fixed to use context instead of rootValue for viewer.

Copy link
Contributor Author

@brad-decker brad-decker Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions on how to do that? the test was failing when it was just using root, and root.viewer.get('id') saying viewer was not defined. Root would have been context in this case.

Copy link
Owner

@mickhansen mickhansen Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually the test needs to be changed from graphql(query, {values}) to graphql(query, null, {values})

Copy link
Contributor Author

@brad-decker brad-decker Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

let result = await graphql(this.schema, `
          {
            viewer {
              tasks {
                edges {
                  cursor
                  node {
                    id
                    name
                  }
                }
              }
            }
          }
        `, null, {
          viewer: viewer
        });

Looks like it already follows that format.

Copy link
Owner

@mickhansen mickhansen Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd, viewer should be on context then.

}
});
Expand Down Expand Up @@ -340,7 +348,7 @@ if (helper.sequelize.dialect.name === 'postgres') {
name: 'userProject',
nodeType: this.projectType,
target: this.User.Projects,
before(options){
before(options) {
// compare a uniq set of attributes against what is returned by the sequelizeConnection resolver
let getUnique = uniq(options.attributes);
projectConnectionAttributesUnique = getUnique.length === options.attributes.length;
Expand Down Expand Up @@ -384,7 +392,7 @@ if (helper.sequelize.dialect.name === 'postgres') {
})
});

let result = await graphql(schema, `
await graphql(schema, `
{
user(id: ${this.userA.id}) {
projects {
Expand Down Expand Up @@ -476,6 +484,96 @@ if (helper.sequelize.dialect.name === 'postgres') {
expect(lastResult.data.user.tasks.pageInfo.hasNextPage).to.equal(false);
});

it('should support in-query slicing and pagination with first and CUSTOM orderBy', async function () {
const correctOrder = await graphql(this.schema, `
{
user(id: ${this.userA.id}) {
tasks(first: 9, orderBy: CUSTOM) {
edges {
cursor
node {
id
name
}
}
pageInfo {
hasNextPage
endCursor
}
}
}
}
`);
const reordered = correctOrder.data.user.tasks.edges.map(({node}) => {
const targetId = fromGlobalId(node.id).id;
return this.userA.tasks.find(task => {
return task.id === Number(targetId);
});
});
Copy link
Owner

@mickhansen mickhansen Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extend this test to also verify that the order from the custom orderBy is correct?

Copy link
Contributor Author

@brad-decker brad-decker Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that sequelize returns the items in the order we expect from the api? The rest of the test checks the order of the paged queries to match what is returned from relay queries.

Copy link
Owner

@mickhansen mickhansen Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the test tests that paging works against the first result, however we haven't exactly verified the first result :)
Isn't stricly necessary but does increase robustness.


let lastThree = reordered.slice(this.userA.tasks.length - 3, this.userA.tasks.length);
let nextThree = reordered.slice(this.userA.tasks.length - 6, this.userA.tasks.length - 3);
let firstThree = reordered.slice(this.userA.tasks.length - 9, this.userA.tasks.length - 6);

expect(firstThree.length).to.equal(3);
expect(nextThree.length).to.equal(3);
expect(lastThree.length).to.equal(3);


let verify = function (result, expectedTasks) {
if (result.errors) throw new Error(result.errors[0].stack);

var resultTasks = result.data.user.tasks.edges.map(function (edge) {
return edge.node;
});

let resultIds = resultTasks.map((task) => {
return parseInt(fromGlobalId(task.id).id, 10);
}).sort();

let expectedIds = expectedTasks.map(function (task) {
return task.get('id');
}).sort();

expect(resultTasks.length).to.equal(3);
expect(resultIds).to.deep.equal(expectedIds);
};

let query = (after) => {
return graphql(this.schema, `
{
user(id: ${this.userA.id}) {
tasks(first: 3, ${after ? 'after: "' + after + '", ' : ''} orderBy: CUSTOM) {
edges {
cursor
node {
id
name
}
}
pageInfo {
hasNextPage
endCursor
}
}
}
}
`);
};

let firstResult = await query();
verify(firstResult, firstThree);
expect(firstResult.data.user.tasks.pageInfo.hasNextPage).to.equal(true);

let nextResult = await query(firstResult.data.user.tasks.pageInfo.endCursor);
verify(nextResult, nextThree);
expect(nextResult.data.user.tasks.pageInfo.hasNextPage).to.equal(true);

let lastResult = await query(nextResult.data.user.tasks.edges[2].cursor);
verify(lastResult, lastThree);
expect(lastResult.data.user.tasks.pageInfo.hasNextPage).to.equal(false);
});

it('should support in-query slicing with user provided args/where', async function () {
Copy link
Owner

@mickhansen mickhansen Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this test was changed although i don't think it was intentional.

let result = await graphql(this.schema, `
{
Expand Down