From f37b3e4dfde5a507d81eced7c9935bf737773409 Mon Sep 17 00:00:00 2001 From: Jonathan Werner Date: Fri, 11 Mar 2016 21:47:18 +0100 Subject: [PATCH 1/3] Improve eslint config, lint (1) .eslintrc: Ignore testing globals Mocha globals like `it` & `describe` trigger eslint warnings because they are not imported explicitly. Setting them as `globals` in .eslintrc fixes that. (2) Make eslintrc compatible with chai assertion expressions There is no way to make eslint shut up about chai's assertion expressions (`expect(foo).to.be.true`) without disabling this rule. See https://github.com/eslint/eslint/issues/2102 (3) Lint resolver.test.js --- .eslintrc | 14 +++- test/integration/relay/connection.test.js | 4 +- test/integration/resolver.test.js | 90 +++++++++++------------ 3 files changed, 57 insertions(+), 51 deletions(-) diff --git a/.eslintrc b/.eslintrc index fc5ae5d0..5cfdc417 100644 --- a/.eslintrc +++ b/.eslintrc @@ -88,7 +88,7 @@ "no-self-compare": 0, "no-sequences": 2, "no-throw-literal": 2, - "no-unused-expressions": 2, + "no-unused-expressions": 0, "no-void": 2, "no-warning-comments": 0, "no-with": 2, @@ -246,5 +246,15 @@ "wrap-regex": 0, "no-var": 0, "max-len": [2, 130, 4] + }, + "globals": { + "it": false, + "afterEach": false, + "beforeEach": false, + "before": false, + "after": false, + "describe": false, + "expect": false, + "assert": false } -} \ No newline at end of file +} diff --git a/test/integration/relay/connection.test.js b/test/integration/relay/connection.test.js index 4c52c65f..9ba58e61 100644 --- a/test/integration/relay/connection.test.js +++ b/test/integration/relay/connection.test.js @@ -765,7 +765,7 @@ if (helper.sequelize.dialect.name === 'postgres') { edges { ...projectOwner node { - id + id } } } @@ -877,4 +877,4 @@ if (helper.sequelize.dialect.name === 'postgres') { }); }); }); -} \ No newline at end of file +} diff --git a/test/integration/resolver.test.js b/test/integration/resolver.test.js index 16e5c0d4..66a70918 100644 --- a/test/integration/resolver.test.js +++ b/test/integration/resolver.test.js @@ -1,13 +1,11 @@ 'use strict'; -var chai = require('chai') - , expect = chai.expect - , resolver = require('../../src/resolver') - , helper = require('./helper') - , Sequelize = require('sequelize') - , sinon = require('sinon') - , sequelize = helper.sequelize - , Promise = helper.Promise; +import {expect} from 'chai'; +import sinon from 'sinon'; +import Sequelize from 'sequelize'; + +import {sequelize, Promise} from './helper'; +import resolver from '../../src/resolver'; import { graphql, @@ -38,7 +36,7 @@ describe('resolver', function () { name: Sequelize.STRING, myVirtual: { type: Sequelize.VIRTUAL, - get: function() { + get: function () { return 'lol'; } } @@ -55,7 +53,7 @@ describe('resolver', function () { }, taskVirtual: { type: Sequelize.VIRTUAL, - get: function() { + get: function () { return 'tasktask'; } } @@ -159,7 +157,7 @@ describe('resolver', function () { } }, resolve: resolver(User.Tasks, { - before: function(options, args) { + before: function (options, args) { if (args.first) { options.order = options.order || []; options.order.push(['created_at', 'ASC']); @@ -207,15 +205,14 @@ describe('resolver', function () { }); before(function () { - var userId = 0 - , taskId = 0 + var taskId = 0 , projectId = 0; return this.sequelize.sync({force: true}).bind(this).then(function () { return Promise.join( Project.create({ id: ++projectId, - name: 'b'+Math.random().toString(), + name: 'b' + Math.random().toString(), labels: [ {name: Math.random().toString()}, {name: Math.random().toString()} @@ -227,7 +224,7 @@ describe('resolver', function () { }), Project.create({ id: ++projectId, - name: 'a'+Math.random().toString(), + name: 'a' + Math.random().toString(), labels: [ {name: Math.random().toString()}, {name: Math.random().toString()} @@ -244,7 +241,7 @@ describe('resolver', function () { return Promise.join( User.create({ id: 1, - name: 'b'+Math.random().toString(), + name: 'b' + Math.random().toString(), tasks: [ { id: ++taskId, @@ -270,7 +267,7 @@ describe('resolver', function () { }), User.create({ id: 2, - name: 'a'+Math.random().toString(), + name: 'a' + Math.random().toString(), tasks: [ { id: ++taskId, @@ -381,7 +378,7 @@ describe('resolver', function () { } rest: tasks(offset: 1, limit: 99) { - title + title } } } @@ -433,7 +430,7 @@ describe('resolver', function () { } }, resolve: resolver(User, { - before: function(options, args, root) { + before: function (options, args, root) { options.where = options.where || {}; options.where.name = root.name; return options; @@ -479,11 +476,11 @@ describe('resolver', function () { } }, resolve: resolver(User, { - after: function(result, args, root) { + after: function (result) { return result.map(function () { return { name: '11!!' - } + }; }); } }) @@ -503,8 +500,8 @@ describe('resolver', function () { expect(result.data.users).to.have.length(users.length); result.data.users.forEach(function (user) { - expect(user.name).to.equal('11!!') - }) + expect(user.name).to.equal('11!!'); + }); }); }); @@ -544,13 +541,13 @@ describe('resolver', function () { var $resolver = resolver(User.Tasks) , $proxy; - $proxy = function() { - return $resolver.apply(null, Array.prototype.slice.call(arguments)) + $proxy = function () { + return $resolver.apply(null, Array.prototype.slice.call(arguments)); }; $proxy.$proxy = $resolver; return $proxy; - })() + }()) } } }); @@ -723,17 +720,17 @@ describe('resolver', function () { } } }), - resolve: (function() { + resolve: (function () { var $resolver; - $resolver = function(source) { + $resolver = function (source) { return source; }; $resolver.$passthrough = true; return $resolver; - })() + }()) } } }); @@ -791,8 +788,6 @@ describe('resolver', function () { }); it('should resolve an array result with a single model and limit', function () { - var users = this.users; - return graphql(schema, ` { users(limit: 1) { @@ -810,7 +805,7 @@ describe('resolver', function () { var user = this.userB; return graphql(schema, ` - { + { user(id: ${user.id}) { name tasks { @@ -838,7 +833,7 @@ describe('resolver', function () { var user = this.userB; return graphql(schema, ` - { + { user(id: ${user.id}) { name tasks(limit: 1) { @@ -858,7 +853,7 @@ describe('resolver', function () { return graphql(schema, ` { - users(order: "id") { + users(order: "id") { name tasks(order: "id") { title @@ -878,7 +873,7 @@ describe('resolver', function () { return { name: user.name, tasks: user.tasks.map(task => ({title: task.title})) - } + }; }) }); }); @@ -889,7 +884,7 @@ describe('resolver', function () { return graphql(schema, ` { - users { + users { name tasks(limit: 1) { title @@ -908,11 +903,11 @@ describe('resolver', function () { it('should resolve a array result with a single limited hasMany association with a nested belongsTo relation', function () { var users = this.users - , sqlSpy = sinon.spy() + , sqlSpy = sinon.spy(); return graphql(schema, ` { - users { + users { tasks(limit: 2) { title project { @@ -940,11 +935,11 @@ describe('resolver', function () { it('should resolve a array result with a single hasMany association with a nested belongsTo relation', function () { var users = this.users - , sqlSpy = sinon.spy() + , sqlSpy = sinon.spy(); return graphql(schema, ` { - users { + users { tasks { title project { @@ -970,13 +965,14 @@ describe('resolver', function () { }); }); - it('should resolve a array result with a single hasMany association with a nested belongsTo relation with a nested hasMany relation', function () { + it('should resolve a array result with a single hasMany association' + + 'with a nested belongsTo relation with a nested hasMany relation', function () { var users = this.users - , sqlSpy = sinon.spy() + , sqlSpy = sinon.spy(); return graphql(schema, ` { - users { + users { tasks { title project { @@ -1056,7 +1052,7 @@ describe('resolver', function () { resolver.filterAttributes = this.defaultValue; }); - it('should be able to disable via a global option', async function () { + it('should be able to disable via a global option', async function () { resolver.filterAttributes = false; var user = this.userB @@ -1094,9 +1090,9 @@ describe('resolver', function () { }); }); - it('should be able to disable via a resolver option', async function () { + it('should be able to disable via a resolver option', async function () { resolver.filterAttributes = true; - + var user = this.userB , schema; @@ -1133,4 +1129,4 @@ describe('resolver', function () { }); }); }); -}); \ No newline at end of file +}); From 4387527bfdb5e078e57b7c3845c2b5aa18134f05 Mon Sep 17 00:00:00 2001 From: Jonathan Werner Date: Fri, 11 Mar 2016 22:56:11 +0100 Subject: [PATCH 2/3] Fix flaky tests in resolver.test.js There were two flaky tests, both related to assertions on specific orderings of arrays. One case expected the order of `user.task` to be deterministic which is not the case, the other expected the order of the graphql schemas `users` field response to be deterministic. The issues were fixed by just asserting that members of the compared arrays were deep equal. --- test/integration/resolver.test.js | 39 ++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/test/integration/resolver.test.js b/test/integration/resolver.test.js index 66a70918..7c248735 100644 --- a/test/integration/resolver.test.js +++ b/test/integration/resolver.test.js @@ -28,6 +28,12 @@ describe('resolver', function () { , labelType , schema; + /** + * Setup the a) testing db schema and b) the according GraphQL types + * + * The schema consists of a User that has Tasks. + * A Task belongs to a Project, which can have Labels. + */ before(function () { sequelize.modelManager.models = []; sequelize.models = {}; @@ -204,6 +210,11 @@ describe('resolver', function () { }); }); + /** + * Now fill the testing DB with fixture values + * We'll have projectA & projectB with two random labels each, + * and two users each with some tasks that belong to those projects. + */ before(function () { var taskId = 0 , projectId = 0; @@ -298,6 +309,7 @@ describe('resolver', function () { return graphql(schema, ` { user(id: ${user.id}) { + id name myVirtual } @@ -307,6 +319,7 @@ describe('resolver', function () { expect(result.data).to.deep.equal({ user: { + id: user.id, name: user.name, myVirtual: 'lol' } @@ -395,6 +408,7 @@ describe('resolver', function () { it('should resolve an array result with a single model', function () { var users = this.users; + return graphql(schema, ` { users { @@ -405,13 +419,15 @@ describe('resolver', function () { if (result.errors) throw new Error(result.errors[0].stack); expect(result.data.users).to.have.length.above(0); - expect(result.data).to.deep.equal({ - users: users.map(user => ({name: user.name})) - }); + + const usersNames = users.map(user => ({name: user.name})); + // As the GraphQL query doesn't specify an ordering, + // the order of the two lists can not be asserted. + expect(result.data.users).to.deep.have.members(usersNames); }); }); - it('should allow ammending the find for a array result with a single model', function () { + it('should allow amending the find for a array result with a single model', function () { var user = this.userA , schema; @@ -802,7 +818,7 @@ describe('resolver', function () { }); it('should resolve a plain result with a single hasMany association', function () { - var user = this.userB; + const user = this.userB; return graphql(schema, ` { @@ -819,14 +835,15 @@ describe('resolver', function () { }).then(function (result) { if (result.errors) throw new Error(result.errors[0].stack); + expect(result.data.user.name).to.equal(user.name); + expect(result.data.user.tasks).to.have.length.above(0); - expect(result.data).to.deep.equal({ - user: { - name: user.name, - tasks: user.tasks.map(task => ({title: task.title, taskVirtual: 'tasktask'})) - } - }); + // As the order of user.tasks is nondeterministic, we only assert on equal members + // of both the user's tasks and the tasks the graphql query responded with. + const userTasks = user.tasks.map(task => ({title: task.title, taskVirtual: 'tasktask'})); + expect(result.data.user.tasks).to.deep.have.members(userTasks); }); + }); it('should resolve a plain result with a single limited hasMany association', function () { From c6b050b840ae338115d2e4bede9e8bd58d0b9e37 Mon Sep 17 00:00:00 2001 From: Jonathan Werner Date: Sat, 12 Mar 2016 01:05:59 +0100 Subject: [PATCH 3/3] Add defaultAttributes resolver option --- README.md | 15 ++++--- package.json | 1 + src/resolver.js | 15 +++++++ test/integration/resolver.test.js | 66 +++++++++++++++++++++++++++---- 4 files changed, 84 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index ac644e7c..fff7b1ec 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # graphql-sequelize [![NPM](https://img.shields.io/npm/v/graphql-sequelize.svg)](https://www.npmjs.com/package/graphql-sequelize) -[![Build Status](https://travis-ci.org/mickhansen/graphql-sequelize.svg?branch=master)](https://travis-ci.org/mickhansen/graphql-sequelize) +[![Build Status](https://travis-ci.org/mickhansen/graphql-sequelize.svg?branch=master)](https://travis-ci.org/mickhansen/graphql-sequelize) [![Slack Status](http://sequelize-slack.herokuapp.com/badge.svg)](http://sequelize-slack.herokuapp.com) - [Installation](#installation) @@ -26,6 +26,9 @@ The graphql-sequelize resolver will by default only select those attributes requ If you have non-database values that depend on other values you can either solve this by using virtual attributes with dependencies on your model or disable attribute filtering via `resolver.filterAttributes = false` or for specific resolvers with `resolver(target, {filterAttributes: false})`. +#### Default attributes +When filtering attributes you might need some fields every time, regardless of wether they have been requested in the query. You can specify those fields with the `defaultAttributes` resolver option like `resolver(target, {defaultAttributes: ['default', 'field', 'names']})`. A common use case would be the need to fetch a `userId` for every query and every resource of your domain model for permission checking -- in that case you would write your `resolver` functions like `resolver(target, {defaultAttributes: ['userId']})`. + ### Features - Automatically converts args to where if arg keys matches model attributes @@ -38,7 +41,7 @@ If you have non-database values that depend on other values you can either solve [Relay documentation](docs/relay.md) -### Examples +### Examples ```js import {resolver} from 'graphql-sequelize'; @@ -236,7 +239,7 @@ attributeFields(Model); ### Renaming generated fields attributeFields accepts a ```map``` option to customize the way the attribute fields are named. The ```map``` option accepts -an object or a function that returns a string. +an object or a function that returns a string. ```js @@ -336,12 +339,12 @@ fullName: { ### defaultArgs -`defaultArgs(Model)` will return an object containing an arg with a key and type matching your models primary key and +`defaultArgs(Model)` will return an object containing an arg with a key and type matching your models primary key and the "where" argument for passing complex query operations described [here](http://docs.sequelizejs.com/en/latest/docs/querying/) ```js var Model = sequelize.define('User', { - + }); defaultArgs(Model); @@ -387,7 +390,7 @@ defaultArgs(Model); type: GraphQLString }, where: { - type JSONType + type JSONType } } ``` diff --git a/package.json b/package.json index 243bf973..25af0795 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "dependencies": { "babel-runtime": "^6.0.8", "graphql-relay": "^0.3.6", + "invariant": "2.2.1", "lodash": "^4.0.0" } } diff --git a/src/resolver.js b/src/resolver.js index 343ccffb..f60a7a75 100644 --- a/src/resolver.js +++ b/src/resolver.js @@ -4,11 +4,19 @@ import simplifyAST from './simplifyAST'; import generateIncludes from './generateIncludes'; import argsToFindOptions from './argsToFindOptions'; import { isConnection, handleConnection, nodeAST, nodeType } from './relay'; +import invariant from 'invariant'; function inList(list, attribute) { return ~list.indexOf(attribute); } +function validateOptions(options) { + invariant( + !options.defaultAttributes || Array.isArray(options.defaultAttributes), + 'options.defaultAttributes must be an array of field names.' + ); +} + function resolverFactory(target, options) { var resolver , targetAttributes @@ -26,6 +34,8 @@ function resolverFactory(target, options) { if (options.handleConnection === undefined) options.handleConnection = true; if (options.filterAttributes === undefined) options.filterAttributes = resolverFactory.filterAttributes; + validateOptions(options); + resolver = function (source, args, info) { var root = info.rootValue || {} , ast = info.fieldASTs @@ -61,6 +71,11 @@ function resolverFactory(target, options) { findOptions.attributes = Object.keys(fields) .map(key => fields[key].key || key) .filter(inList.bind(null, targetAttributes)); + + if (options.defaultAttributes) { + findOptions.attributes = findOptions.attributes.concat(options.defaultAttributes); + } + } else { findOptions.attributes = targetAttributes; } diff --git a/test/integration/resolver.test.js b/test/integration/resolver.test.js index 7c248735..abc22dcd 100644 --- a/test/integration/resolver.test.js +++ b/test/integration/resolver.test.js @@ -1,10 +1,10 @@ 'use strict'; -import {expect} from 'chai'; +import { expect } from 'chai'; import sinon from 'sinon'; import Sequelize from 'sequelize'; -import {sequelize, Promise} from './helper'; +import { sequelize, Promise } from './helper'; import resolver from '../../src/resolver'; import { @@ -46,8 +46,6 @@ describe('resolver', function () { return 'lol'; } } - }, { - timestamps: false }); Task = sequelize.define('task', { @@ -309,7 +307,6 @@ describe('resolver', function () { return graphql(schema, ` { user(id: ${user.id}) { - id name myVirtual } @@ -319,7 +316,6 @@ describe('resolver', function () { expect(result.data).to.deep.equal({ user: { - id: user.id, name: user.name, myVirtual: 'lol' } @@ -408,7 +404,6 @@ describe('resolver', function () { it('should resolve an array result with a single model', function () { var users = this.users; - return graphql(schema, ` { users { @@ -1146,4 +1141,61 @@ describe('resolver', function () { }); }); }); + + describe('defaultAttributes', () => { + it('should only accept an array as input', () => { + const f = () => resolver(User, { + defaultAttributes: 'not_an_array', + }); + expect(f).to.throw(); + }); + + it('should automatically include fields specified in that array', function () { + const user = this.userA; + const sqlSpy = sinon.spy(); + + const makeSchema = (defaultAttributes) => { + return new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'RootQueryType', + fields: { + user: { + type: userType, + args: { id: { type: GraphQLInt } }, + resolve: resolver(User, { + filterAttributes: true, + defaultAttributes, + }) + } + } + }) + }); + }; + + const normalSchema = makeSchema(); + + const defaultAttributes = ['createdAt']; + const schemaWithDefaultAttributes = makeSchema(defaultAttributes); + + const query = `{ user(id: ${user.id}) { name } } `; + + return graphql(normalSchema, query, {logging: sqlSpy}).then(function () { + const [sqlQuery] = sqlSpy.args[0]; + // As we have + // 1) `filterAttributes` set to true, + // 2) no `defaultAttributes` defined and + // 3) `createdAt` hasn't been requested in the graphql query, + // `createdAt` should not be fetched + expect(sqlQuery).to.not.contain('createdAt'); + + return graphql(schemaWithDefaultAttributes, query, {logging: sqlSpy}); + }).then(function (result) { + // With the schema that defined `createdAt` as a default attribute for + // user, it should be fetched + const [sqlQuery] = sqlSpy.args[1]; + expect(sqlQuery).to.contain('createdAt'); + expect(result.data.user.createdAt); + }); + }); + }); });