From b9244ce8c3b4f2404739da983eab084a7b7a9fb4 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Mon, 31 Oct 2016 19:20:38 +0530 Subject: [PATCH 1/7] refactor(test): fix query typo --- test/unit/factory.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/factory.spec.js b/test/unit/factory.spec.js index d82719ad..1e86110b 100644 --- a/test/unit/factory.spec.js +++ b/test/unit/factory.spec.js @@ -289,7 +289,7 @@ describe('Factory', function () { yield modelFixtures.truncate(Database) }) - it('should be able to truncat the database table using the reset method', function * () { + it('should be able to truncate the database table using the reset method', function * () { Factory.blueprint('users', function (fake) { return { username: fake.username(), @@ -298,7 +298,7 @@ describe('Factory', function () { }) yield Factory.get('users').create(10) yield Factory.get('users').reset() - const ids = yield Database.table('users').pluck('ids') + const ids = yield Database.table('users').pluck('id') expect(ids).to.be.an('array') expect(ids.length).to.equal(0) }) From ac16baada5b6664f12522a1043747523fa5aca56 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Mon, 31 Oct 2016 19:24:08 +0530 Subject: [PATCH 2/7] fix(database): paginate count query to ignore order by order by is not required when getting records total inside paginate method, making sure to filter the orderby clause from the cloned query Closes #64 --- src/Database/index.js | 15 +++++++++++++++ test/unit/database.spec.js | 9 +++++++++ 2 files changed, 24 insertions(+) diff --git a/src/Database/index.js b/src/Database/index.js index 52d4409d..65d16867 100644 --- a/src/Database/index.js +++ b/src/Database/index.js @@ -66,6 +66,15 @@ const _emitSql = function (builder) { }) } +/** + * Following attributes should be removed from the + * paginate count query since things like orderBy + * is not required when fetching the count. + * + * @type {Array} + */ +const excludeAttrFromCount = ['order'] + /** * Database provider to build sql queries * @module Database @@ -315,6 +324,12 @@ Database.paginate = function * (page, perPage, countByQuery) { * query for getting results */ countByQuery = countByQuery || this.clone().count('* as total') + + /** + * Filter unnecessary statements from the cloned query + */ + countByQuery._statements = _.filter(countByQuery._statements, (statement) => excludeAttrFromCount.indexOf(statement.grouping) < 0) + const count = yield countByQuery if (!count[0] || parseInt(count[0].total, 10) === 0) { return util.makePaginateMeta(0, parsedPage, parsedPerPage) diff --git a/test/unit/database.spec.js b/test/unit/database.spec.js index f3e0c8d5..e1b800ba 100644 --- a/test/unit/database.spec.js +++ b/test/unit/database.spec.js @@ -252,6 +252,15 @@ describe('Database provider', function () { expect(paginatedUsers.lastPage).to.equal(1) }) + it('should be able paginate results using order by on the original query', function * () { + const paginatedUsers = yield Database.table('users').orderBy('id', 'desc').paginate(1) + expect(paginatedUsers).to.have.property('total') + expect(paginatedUsers).to.have.property('lastPage') + expect(paginatedUsers).to.have.property('perPage') + expect(paginatedUsers).to.have.property('data') + expect(paginatedUsers.total).to.equal(paginatedUsers.data.length) + }) + it('should be able to get results in chunks', function * () { let callbackCalledForTimes = 0 const allUsers = yield Database.table('users') From 6dd0f926691f77680309d19ce7939ed709a87058 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Mon, 31 Oct 2016 19:28:14 +0530 Subject: [PATCH 3/7] style(lint): make standard linter happy --- package.json | 9 ++++++--- src/Database/index.js | 3 +-- test/unit/fixtures/files.js | 4 ++-- test/unit/fixtures/relations.js | 1 - test/unit/helpers/mysqlConnections.js | 22 +++++++++++----------- test/unit/helpers/postgresConnection.js | 18 +++++++++--------- test/unit/helpers/sqliteConnections.js | 2 +- test/unit/migrations.spec.js | 1 - 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/package.json b/package.json index defec50a..e4aa5fe1 100644 --- a/package.json +++ b/package.json @@ -7,10 +7,13 @@ }, "version": "3.0.9", "scripts": { - "lint": "standard src/**/*.js src/**/**/*.js src/**/**/**/*.js lib/*.js test/**/*.js providers/*.js", + "lint": "standard", "test:all": "DB=sqlite3 npm run test && DB=mysql npm run test && DB=pg npm run test", - "coverage": "node --harmony_proxies ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha test/unit test/acceptance", - "test": "node --harmony_proxies ./node_modules/.bin/istanbul cover _mocha --report lcovonly -- -R spec test/unit test/acceptance && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage" + "coverage": "npm run lint && node --harmony_proxies ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha test/unit test/acceptance", + "test": "npm run lint && node --harmony_proxies ./node_modules/.bin/istanbul cover _mocha --report lcovonly -- -R spec test/unit test/acceptance && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage" + }, + "standard": { + "global": ["use", "describe", "it", "after", "before", "context", "make", "beforeEach"] }, "author": "adonisjs", "license": "MIT", diff --git a/src/Database/index.js b/src/Database/index.js index 65d16867..aa73a257 100644 --- a/src/Database/index.js +++ b/src/Database/index.js @@ -383,7 +383,7 @@ Database.chunk = function * (limit, cb, page) { */ Database.table = function (tableName) { const prefix = this._instancePrefix || this.client.config.prefix - const prefixedTableName = (prefix && !this._skipPrefix) ? `${prefix}${tableName}`: tableName + const prefixedTableName = (prefix && !this._skipPrefix) ? `${prefix}${tableName}` : tableName this._originalTable(prefixedTableName) return this } @@ -410,7 +410,6 @@ Database.withPrefix = function (prefix) { return this } - /** * these methods are not proxied and instead actual implementations * are returned diff --git a/test/unit/fixtures/files.js b/test/unit/fixtures/files.js index db2b992c..e33a4b9c 100644 --- a/test/unit/fixtures/files.js +++ b/test/unit/fixtures/files.js @@ -11,10 +11,10 @@ const path = require('path') module.exports = { cleanStorage: function * () { - return yield fs.emptyDir(path.join(__dirname,'../storage')) + return yield fs.emptyDir(path.join(__dirname, '../storage')) }, createDir: function * () { - return yield fs.ensureDir(path.join(__dirname,'../storage')) + return yield fs.ensureDir(path.join(__dirname, '../storage')) } } diff --git a/test/unit/fixtures/relations.js b/test/unit/fixtures/relations.js index 490a7514..0bbac147 100644 --- a/test/unit/fixtures/relations.js +++ b/test/unit/fixtures/relations.js @@ -6,7 +6,6 @@ * MIT Licensed */ -const path = require('path') const bluebird = require('bluebird') const files = require('./files') diff --git a/test/unit/helpers/mysqlConnections.js b/test/unit/helpers/mysqlConnections.js index f01a67d7..7e90a872 100644 --- a/test/unit/helpers/mysqlConnections.js +++ b/test/unit/helpers/mysqlConnections.js @@ -7,30 +7,30 @@ */ module.exports = { - default : { + default: { client: 'mysql', connection: { - user : 'root', - password : '', - database : 'default' + user: 'root', + password: '', + database: 'default' } }, alternateConnection: { client: 'mysql', connection: { - user : 'root', - password : '', - database : 'alternate' + user: 'root', + password: '', + database: 'alternate' } }, - defaultPrefix : { + defaultPrefix: { client: 'mysql', connection: { - user : 'root', - password : '', - database : 'default' + user: 'root', + password: '', + database: 'default' }, prefix: 'ad_' } diff --git a/test/unit/helpers/postgresConnection.js b/test/unit/helpers/postgresConnection.js index 1e4dad76..7c4faedc 100644 --- a/test/unit/helpers/postgresConnection.js +++ b/test/unit/helpers/postgresConnection.js @@ -7,30 +7,30 @@ */ module.exports = { - default : { + default: { client: 'pg', connection: { user: '', - password : '', - database : 'default' + password: '', + database: 'default' } }, alternateConnection: { client: 'pg', connection: { - user : '', - password : '', - database : 'alternate' + user: '', + password: '', + database: 'alternate' } }, - defaultPrefix : { + defaultPrefix: { client: 'pg', connection: { user: '', - password : '', - database : 'default' + password: '', + database: 'default' }, prefix: 'ad_' } diff --git a/test/unit/helpers/sqliteConnections.js b/test/unit/helpers/sqliteConnections.js index fcf4f2ad..f5b961bb 100644 --- a/test/unit/helpers/sqliteConnections.js +++ b/test/unit/helpers/sqliteConnections.js @@ -8,7 +8,7 @@ const path = require('path') module.exports = { - default : { + default: { client: 'sqlite3', connection: { filename: path.join(__dirname, '../storage/test.sqlite3') diff --git a/test/unit/migrations.spec.js b/test/unit/migrations.spec.js index 86091f6f..54f4af16 100644 --- a/test/unit/migrations.spec.js +++ b/test/unit/migrations.spec.js @@ -824,7 +824,6 @@ describe('Migrations', function () { it('should be able to rename the database table', function * () { const Runner = new Migrations(Database, Config) const runner = new Runner() - let db = null class Users extends Schema { up () { this.create('users', (table) => { From 08e1fef9dc2b16d68f125f29e99f72dcba9a9aa3 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Mon, 31 Oct 2016 22:25:32 +0530 Subject: [PATCH 4/7] chore(test): move test scripts to node files nodejs inbalanced support for proxies requires a conditional before running the tests --- bin/coverage.js | 12 ++++++++++++ bin/test.js | 12 ++++++++++++ package.json | 16 +++++++++++++--- 3 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 bin/coverage.js create mode 100644 bin/test.js diff --git a/bin/coverage.js b/bin/coverage.js new file mode 100644 index 00000000..e1f2a267 --- /dev/null +++ b/bin/coverage.js @@ -0,0 +1,12 @@ +'use strict' + +const semver = require('semver') +const shell = require('shelljs') +const nodeJsVersion = process.version +let proxiesFlag = '' + +if (semver.lt(nodeJsVersion, '6.0.0')) { + proxiesFlag = '--harmony_proxies' +} + +shell.exec(`DB=${process.env.DB} node ${proxiesFlag} ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha test/unit test/acceptance`) diff --git a/bin/test.js b/bin/test.js new file mode 100644 index 00000000..5604c197 --- /dev/null +++ b/bin/test.js @@ -0,0 +1,12 @@ +'use strict' + +const semver = require('semver') +const shell = require('shelljs') +const nodeJsVersion = process.version +let proxiesFlag = '' + +if (semver.lt(nodeJsVersion, '6.0.0')) { + proxiesFlag = '--harmony_proxies' +} + +shell.exec(`DB=${process.env.DB} node ${proxiesFlag} ./node_modules/.bin/istanbul cover _mocha --colors --report lcovonly -- -R spec test/unit test/acceptance && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage`) diff --git a/package.json b/package.json index e4aa5fe1..05400d3d 100644 --- a/package.json +++ b/package.json @@ -9,11 +9,20 @@ "scripts": { "lint": "standard", "test:all": "DB=sqlite3 npm run test && DB=mysql npm run test && DB=pg npm run test", - "coverage": "npm run lint && node --harmony_proxies ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha test/unit test/acceptance", - "test": "npm run lint && node --harmony_proxies ./node_modules/.bin/istanbul cover _mocha --report lcovonly -- -R spec test/unit test/acceptance && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage" + "coverage": "npm run lint && node ./bin/coverage", + "test": "npm run lint && node ./bin/test" }, "standard": { - "global": ["use", "describe", "it", "after", "before", "context", "make", "beforeEach"] + "global": [ + "use", + "describe", + "it", + "after", + "before", + "context", + "make", + "beforeEach" + ] }, "author": "adonisjs", "license": "MIT", @@ -32,6 +41,7 @@ "mysql": "^2.10.2", "pg": "^4.5.1", "semantic-release": "^4.3.5", + "semver": "^5.3.0", "shelljs": "^0.7.4", "sqlite3": "^3.1.1", "standard": "^8.0.0" From d4a060a2bc227e8096341d2313522c0a49a4ae64 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Mon, 31 Oct 2016 22:31:16 +0530 Subject: [PATCH 5/7] style(command:reset): correct latest spelling Closes #68 --- src/Commands/Reset.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Commands/Reset.js b/src/Commands/Reset.js index e5c68d22..58d4cca2 100644 --- a/src/Commands/Reset.js +++ b/src/Commands/Reset.js @@ -32,7 +32,7 @@ class Reset extends Command { * @public */ get description () { - return 'Reset migrations to lastest batch' + return 'Reset migrations to latest batch' } /** From 0067bcad8cd0fbac6ee391d353f02aac0b6e14b4 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Wed, 2 Nov 2016 12:16:08 +0530 Subject: [PATCH 6/7] fix(lucid:relations): implement delete method to delete relations delete method on related method was deleting everything from db table and should be adding a where clause for the related row Closes #63 --- src/Lucid/Relations/BelongsToMany.js | 9 ++++ src/Lucid/Relations/HasManyThrough.js | 9 ++++ src/Lucid/Relations/Relation.js | 11 ++++ test/unit/lucid.relations.spec.js | 77 +++++++++++++++++++++++++++ 4 files changed, 106 insertions(+) diff --git a/src/Lucid/Relations/BelongsToMany.js b/src/Lucid/Relations/BelongsToMany.js index 882ed563..fd1e9fbe 100644 --- a/src/Lucid/Relations/BelongsToMany.js +++ b/src/Lucid/Relations/BelongsToMany.js @@ -405,6 +405,15 @@ class BelongsToMany extends Relation { return relatedInstance } + /** + * Throws an exception since deleting the related model + * should be done via relation and detach should be + * used instead. + */ + * delete () { + throw new CE.ModelRelationException('delete is not supported by BelongsToMany, use detach instead') + } + } module.exports = BelongsToMany diff --git a/src/Lucid/Relations/HasManyThrough.js b/src/Lucid/Relations/HasManyThrough.js index 21e0f12c..884414f9 100644 --- a/src/Lucid/Relations/HasManyThrough.js +++ b/src/Lucid/Relations/HasManyThrough.js @@ -227,6 +227,15 @@ class HasManyThrough extends Relation { throw CE.ModelRelationException.unSupportedMethod('saveMany', this.constructor.name) } + /** + * Throws an exception since deleting the related model + * should be done via relation and detach should be + * used instead. + */ + * delete () { + throw CE.ModelRelationException.unSupportedMethod('delete', this.constructor.name) + } + } module.exports = HasManyThrough diff --git a/src/Lucid/Relations/Relation.js b/src/Lucid/Relations/Relation.js index ad59393b..1e81431a 100644 --- a/src/Lucid/Relations/Relation.js +++ b/src/Lucid/Relations/Relation.js @@ -87,6 +87,17 @@ class Relation { return this.relatedQuery.first() } + /** + * Removes the related record for the given relationship + * + * @return {Object} + */ + delete () { + this._validateRead() + this._decorateRead() + return this.relatedQuery.delete() + } + /** * calls the fetch method on the related query builder * diff --git a/test/unit/lucid.relations.spec.js b/test/unit/lucid.relations.spec.js index 28f25bda..cdf5aca3 100644 --- a/test/unit/lucid.relations.spec.js +++ b/test/unit/lucid.relations.spec.js @@ -3364,6 +3364,22 @@ describe('Relations', function () { expect(supplier.toJSON().account).to.equal(null) yield relationFixtures.truncate(Database, 'suppliers') }) + + it('should be able to delete related records', function * () { + const supplierId = yield relationFixtures.createRecords(Database, 'suppliers', {name: 'redtape'}) + class Account extends Model { + } + class Supplier extends Model { + account () { + return this.hasOne(Account) + } + } + const supplier = yield Supplier.find(1) + const query = supplier.account().delete().toSQL() + expect(queryHelpers.formatQuery(query.sql)).to.equal(queryHelpers.formatQuery('delete from "accounts" where "supplier_id" = ?')) + expect(queryHelpers.formatBindings(query.bindings)).deep.equal(queryHelpers.formatBindings(supplierId)) + yield relationFixtures.truncate(Database, 'suppliers') + }) }) context('Regression:BelongsTo', function () { @@ -3410,6 +3426,25 @@ describe('Relations', function () { expect(comment.toJSON().post).to.equal(null) yield relationFixtures.truncate(Database, 'comments') }) + + it('should be able to delete related records', function * () { + const commentId = yield relationFixtures.createRecords(Database, 'comments', {body: 'Nice article', post_id: 1}) + + class Post extends Model { + } + + class Comment extends Model { + post () { + return this.belongsTo(Post) + } + } + + const comment = yield Comment.find(commentId[0]) + const query = comment.post().delete().toSQL() + expect(queryHelpers.formatQuery(query.sql)).to.equal(queryHelpers.formatQuery('delete from "posts" where "id" = ?')) + expect(queryHelpers.formatBindings(query.bindings)).deep.equal(commentId) + yield relationFixtures.truncate(Database, 'comments') + }) }) context('Regression:HasMany', function () { @@ -3456,6 +3491,22 @@ describe('Relations', function () { expect(post.toJSON().comments).deep.equal([]) yield relationFixtures.truncate(Database, 'posts') }) + + it('should be able to delete the related records', function * () { + const postId = yield relationFixtures.createRecords(Database, 'posts', {title: 'Adonis 101', body: 'Let\'s learn Adonis'}) + class Comment extends Model { + } + class Post extends Model { + comments () { + return this.hasMany(Comment) + } + } + const post = yield Post.find(postId[0]) + const query = post.comments().delete().toSQL() + expect(queryHelpers.formatQuery(query.sql)).to.equal(queryHelpers.formatQuery('delete from "comments" where "post_id" = ?')) + expect(queryHelpers.formatBindings(query.bindings)).deep.equal(postId) + yield relationFixtures.truncate(Database, 'posts') + }) }) context('Regression:BelongsToMany', function () { @@ -3530,5 +3581,31 @@ describe('Relations', function () { yield relationFixtures.truncate(Database, 'courses') yield relationFixtures.truncate(Database, 'course_student') }) + + it('should throw an exception when trying to delete related records', function * () { + const savedStudent = yield relationFixtures.createRecords(Database, 'students', {name: 'ricky', id: 29}) + const savedCourse = yield relationFixtures.createRecords(Database, 'courses', {title: 'geometry'}) + yield relationFixtures.createRecords(Database, 'course_student', {student_id: savedStudent[0], course_id: savedCourse[0]}) + + class Course extends Model { + } + + class Student extends Model { + courses () { + return this.belongsToMany(Course) + } + } + + const student = yield Student.find(savedStudent[0]) + try { + const isDeleted = yield student.courses().delete() + expect(isDeleted).not.to.exist + } catch (e) { + expect(e.message).to.equal('delete is not supported by BelongsToMany, use detach instead') + yield relationFixtures.truncate(Database, 'students') + yield relationFixtures.truncate(Database, 'courses') + yield relationFixtures.truncate(Database, 'course_student') + } + }) }) }) From 738dd1c99e9697723920397916b1d7d8b958a4c2 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Wed, 2 Nov 2016 15:18:41 +0530 Subject: [PATCH 7/7] chore(release): release 3.0.10 --- CHANGELOG.md | 11 +++++++++++ package.json | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4250832..615cc4d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ + +## [3.0.10](https://github.com/adonisjs/adonis-lucid/compare/v3.0.9...v3.0.10) (2016-11-02) + + +### Bug Fixes + +* **database:** paginate count query to ignore order by ([ac16baa](https://github.com/adonisjs/adonis-lucid/commit/ac16baa)), closes [#64](https://github.com/adonisjs/adonis-lucid/issues/64) +* **lucid:relations:** implement delete method to delete relations ([0067bca](https://github.com/adonisjs/adonis-lucid/commit/0067bca)), closes [#63](https://github.com/adonisjs/adonis-lucid/issues/63) + + + ## [3.0.9](https://github.com/adonisjs/adonis-lucid/compare/v3.0.8...v3.0.9) (2016-10-19) diff --git a/package.json b/package.json index 05400d3d..d5f0224b 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "directories": { "test": "test" }, - "version": "3.0.9", + "version": "3.0.10", "scripts": { "lint": "standard", "test:all": "DB=sqlite3 npm run test && DB=mysql npm run test && DB=pg npm run test",