From 5af0f5fa9cfd16f027064db1b57a337bd26163ab Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Wed, 17 May 2017 18:30:01 -0700 Subject: [PATCH 1/3] don't use express-request for infra-code copy use the deepCV method, and fix it to make it actually work --- lib/models/services/build-service.js | 33 ++++----- .../services/context-version-service.js | 56 +++++++-------- lib/models/services/instance-fork-service.js | 15 ++-- lib/routes/contexts/versions/index.js | 68 ++----------------- 4 files changed, 49 insertions(+), 123 deletions(-) diff --git a/lib/models/services/build-service.js b/lib/models/services/build-service.js index 57826114a..ffe98697b 100644 --- a/lib/models/services/build-service.js +++ b/lib/models/services/build-service.js @@ -470,27 +470,24 @@ BuildService.createNewContextVersion = function (instance, pushInfo) { if (!contextOwnerGithubId) { throw new Error('createNewContextVersion requires the context to have an owner') } - return Promise.fromCallback(function (callback) { - var user = { - accounts: { - github: { - id: pushInfoGithubId - } + var user = { + accounts: { + github: { + id: pushInfoGithubId } } - var opts = { - owner: { - github: contextOwnerGithubId - } + } + var opts = { + owner: { + github: contextOwnerGithubId } - ContextVersionService.handleVersionDeepCopy( - context, - contextVersion, - user, - opts, - callback - ) - }) + } + ContextVersionService.handleVersionDeepCopy( + context, + contextVersion, + user, + opts + ) }) .then(function (newContextVersion) { return ContextVersion.modifyAppCodeVersionByRepoAsync( diff --git a/lib/models/services/context-version-service.js b/lib/models/services/context-version-service.js index ac123a2b6..0714bc714 100644 --- a/lib/models/services/context-version-service.js +++ b/lib/models/services/context-version-service.js @@ -55,51 +55,41 @@ ContextVersionService.findContextVersion = function (id) { * @param {Number} [opts.owner.github] Github ID of Owner with which to override. * @param {Function} cb Callback which returns (err, newContextVersion) */ -ContextVersionService.handleVersionDeepCopy = function (context, contextVersion, user, opts, cb) { +ContextVersionService.handleVersionDeepCopy = function (context, contextVersion, user, opts) { const log = this.logger.child({ contextId: context._id, contextVersionId: contextVersion._id, method: 'handleVersionDeepCopy' }) log.info('called') - if (isFunction(opts)) { - cb = opts + if (!opts) { opts = {} } const contextOwnerId = keypather.get(context, 'owner.github') const userGithubId = keypather.get(user, 'accounts.github.id') // 1. deep copy contextVersion - ContextVersion.createDeepCopy(user, contextVersion, function (err, newContextVersion) { - if (err) { return cb(err) } - // check if the context version is (owned by hellorunnable AND the user isn't hellorunnable) - if (contextOwnerId !== process.env.HELLO_RUNNABLE_GITHUB_ID || userGithubId === contextOwnerId) { - return cb(null, newContextVersion) - } - // 2. create new context - const ownerGithubId = keypather.get(opts, 'owner.github') || userGithubId - const newContext = new Context({ - name: uuid(), - owner: { github: ownerGithubId } - }) - // 3. 'move' new contextVerion -> new context - newContextVersion.context = newContext._id + return ContextVersion.createDeepCopyAsync(user, contextVersion) + .tap(newContextVersion => { + // check if the context version is (owned by hellorunnable AND the user isn't hellorunnable) + if (contextOwnerId !== process.env.HELLO_RUNNABLE_GITHUB_ID || userGithubId === contextOwnerId) { + return newContextVersion + } + // 2. create new context + const ownerGithubId = keypather.get(opts, 'owner.github') || userGithubId + const newContext = new Context({ + name: uuid(), + owner: { github: ownerGithubId } + }) + // 3. 'move' new contextVerion -> new context + newContextVersion.set('context', newContext._id) - // 4. update the owner of the contextVersion - newContextVersion.owner.github = ownerGithubId + // 4. update the owner of the contextVersion + newContextVersion.set('owner', { github: ownerGithubId }) - async.series([ - // 4.1. save context, version - newContext.save.bind(newContext), - newContextVersion.save.bind(newContextVersion), - function (cb) { - // 5. copy icv to the new cv - InfraCodeVersionService.copyInfraCodeToContextVersion(newContextVersion, contextVersion.infraCodeVersion._id).asCallback(cb) - } - ], function (err, results) { - // [1]: newContextVersion.save results - // [1][0]: newContextVersion.save document - // [1][1]: newContextVersion.save number affected - cb(err, keypather.get(results, '[1][0]')) + return newContext.saveAsync() + .then(() => newContextVersion.saveAsync()) + .then(() => + InfraCodeVersionService.copyInfraCodeToContextVersion(newContextVersion, contextVersion.infraCodeVersion) + ) }) - }) } diff --git a/lib/models/services/instance-fork-service.js b/lib/models/services/instance-fork-service.js index 65a5c55b0..a5c5e2c18 100644 --- a/lib/models/services/instance-fork-service.js +++ b/lib/models/services/instance-fork-service.js @@ -242,15 +242,12 @@ InstanceForkService._createNewNonRepoContextVersion = function (contextVersion, .then(function (context) { var user = { accounts: { github: { id: createdById } } } var opts = { owner: { github: ownerId } } - return Promise.fromCallback(function (callback) { - ContextVersionService.handleVersionDeepCopy( - context, - contextVersion, - user, - opts, - callback - ) - }) + return ContextVersionService.handleVersionDeepCopy( + context, + contextVersion, + user, + opts + ) }) .then(function (newContextVersion) { // non-repo context versions _must_ have advanced: true set. diff --git a/lib/routes/contexts/versions/index.js b/lib/routes/contexts/versions/index.js index 82ff90612..11178a544 100644 --- a/lib/routes/contexts/versions/index.js +++ b/lib/routes/contexts/versions/index.js @@ -243,73 +243,15 @@ app.post('/contexts/:contextId/versions/:id/actions/copy', findContextVersion, // only supports deep copy for now mw.query('deep').require().validate(validations.equals('true')), - - mw.req('context.owner.github').require() - .validate(validations.equals(process.env.HELLO_RUNNABLE_GITHUB_ID)) - .validate(validations.notEqualsKeypath('sessionUser.accounts.github.id')) - .then( - // if the build owner is hello-runnable and user is not hello-runnable - deepCopyCvFromHelloRunnable('contextVersion') - ).else( - function (req, res, next) { - ContextVersion.createDeepCopyAsync(req.sessionUser, req.contextVersion) - .tap(function (contextVersion) { - req.contextVersion = contextVersion - }) - .asCallback(next) - } - ), - mw.res.status(201), - mw.res.json('contextVersion')) - -function deepCopyCvFromHelloRunnable (cvKey) { - return flow.series( - // maybe error if the contextVersion has acvs? - // make a new context - mw.req('contextVersion', cvKey), - // create context requires name - function (req, res, next) { - req.body.name = uuid() - ContextService.createNew(req.sessionUser, req.body) - .then(function (context) { - req.newContext = context - next() - }) - .catch(next) - }, - // make a new context-version - function (req, res, next) { - var runnable = new Runnable(req.headers) - runnable.createContextVersion( - keypather.get(req, 'newContext._id'), - function (err, result) { - if (err) { - return next(err) - } - req.newContextVersion = result - next() - }) - }, - // use infracodeversion copy route to copy the files - function (req, res, next) { - var runnable = new Runnable(req.headers) - runnable.copyVersionIcvFiles( - keypather.get(req, 'newContext._id'), - keypather.get(req, 'newContextVersion._id'), - keypather.get(req, 'contextVersion.infraCodeVersion'), - next) - }, - function (req, res, next) { - ContextVersionService.findContextVersion(keypather.get(req, 'newContextVersion._id')) + function (req, res, next) { + ContextVersionService.handleVersionDeepCopy(req.context, req.contextVersion, req.sessionUser, req.body) .tap(function (contextVersion) { - req.contextVersion = contextVersion + res.json(201, contextVersion.toJSON()) }) - .asCallback(function (err) { + .catch(function (err) { next(err) }) - } - ) -} + }) /** * We're assuming this is only called when reverting files back to source From 4397c3f77e3ce2e30304a13d1a13d55ab300049f Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 18 May 2017 14:55:15 -0700 Subject: [PATCH 2/3] Fix tests Fix Linter Clean up the code a little --- .../services/context-version-service.js | 32 ++-- lib/routes/contexts/versions/index.js | 3 - .../services/context-version-service.js | 158 ++++++++++-------- 3 files changed, 106 insertions(+), 87 deletions(-) diff --git a/lib/models/services/context-version-service.js b/lib/models/services/context-version-service.js index 0714bc714..cf2aa577c 100644 --- a/lib/models/services/context-version-service.js +++ b/lib/models/services/context-version-service.js @@ -1,8 +1,6 @@ 'use strict' -const async = require('async') const Boom = require('dat-middleware').Boom -const isFunction = require('101/is-function') const keypather = require('keypather')() const uuid = require('uuid') @@ -47,13 +45,15 @@ ContextVersionService.findContextVersion = function (id) { * the owner with the user provided (session user), creating a new context and * copying the infracode files as well. Otherwise, it just creates a new context * version. - * @param {Object} context Context object that 'owns' the contextVersion. - * @param {Object} contextVersion Context Version to copy. - * @param {Object} user User object (most likely sessionUser). - * @param {Object} [opts] Opts to allow overrides for the owner. - * @param {Object} [opts.owner] Owner override - * @param {Number} [opts.owner.github] Github ID of Owner with which to override. - * @param {Function} cb Callback which returns (err, newContextVersion) + * @param {Context} context - Context object that 'owns' the contextVersion. + * @param {ContextVersion} contextVersion - Context Version to copy. + * @param {SessionUser} user - User object (most likely sessionUser). + * @param {Object} opts - Opts to allow overrides for the owner. + * @param {Object} opts.owner - Owner override + * @param {Number} opts.owner.github - Github ID of Owner with which to override. + * + * @resolves {ContextVersion} Newly forked ContextVersion + * */ ContextVersionService.handleVersionDeepCopy = function (context, contextVersion, user, opts) { const log = this.logger.child({ @@ -75,18 +75,16 @@ ContextVersionService.handleVersionDeepCopy = function (context, contextVersion, return newContextVersion } // 2. create new context - const ownerGithubId = keypather.get(opts, 'owner.github') || userGithubId + const ownerGithubObject = { + github: keypather.get(opts, 'owner.github') || userGithubId + } + newContextVersion.set('owner', ownerGithubObject) const newContext = new Context({ name: uuid(), - owner: { github: ownerGithubId } + owner: ownerGithubObject }) - // 3. 'move' new contextVerion -> new context - newContextVersion.set('context', newContext._id) - - // 4. update the owner of the contextVersion - newContextVersion.set('owner', { github: ownerGithubId }) - return newContext.saveAsync() + .tap(newContext => newContextVersion.set('context', newContext._id)) .then(() => newContextVersion.saveAsync()) .then(() => InfraCodeVersionService.copyInfraCodeToContextVersion(newContextVersion, contextVersion.infraCodeVersion) diff --git a/lib/routes/contexts/versions/index.js b/lib/routes/contexts/versions/index.js index 11178a544..37482d7dc 100644 --- a/lib/routes/contexts/versions/index.js +++ b/lib/routes/contexts/versions/index.js @@ -2,10 +2,8 @@ const assign = require('101/assign') const express = require('express') -const flow = require('middleware-flow') const keypather = require('keypather')() const mw = require('dat-middleware') -const uuid = require('uuid') const Build = require('models/mongo/build') const ContextService = require('models/services/context-service') @@ -15,7 +13,6 @@ const InfraCodeVersion = require('models/mongo/infra-code-version') const InfraCodeVersionService = require('models/services/infracode-version-service') const Instance = require('models/mongo/instance') const PermissionService = require('models/services/permission-service') -const Runnable = require('models/apis/runnable') const transformations = require('middlewares/transformations') const validations = require('middlewares/validations') diff --git a/unit/models/services/context-version-service.js b/unit/models/services/context-version-service.js index 0079296dd..e6e4d8950 100644 --- a/unit/models/services/context-version-service.js +++ b/unit/models/services/context-version-service.js @@ -81,28 +81,31 @@ describe('ContextVersionService', function () { }) describe('handleVersionDeepCopy', function () { beforeEach(function (done) { - sinon.stub(ContextVersion, 'createDeepCopy').yieldsAsync() - sinon.stub(InfraCodeVersionService, 'copyInfraCodeToContextVersion').returns(Promise.resolve()) - sinon.stub(Context.prototype, 'save').yieldsAsync() + sinon.stub(ContextVersion, 'createDeepCopyAsync').resolves() + sinon.stub(InfraCodeVersionService, 'copyInfraCodeToContextVersion').resolves() ctx.mockContextVersion = { infraCodeVersion: 'pizza', - owner: { github: 1234 } + owner: { github: 1234 }, + saveAsync: sinon.stub().resolves(), + set: sinon.stub() } ctx.mockContext = { - owner: { github: 1234 } + owner: { github: 1234 }, + saveAsync: sinon.stub().resolves() } ctx.mockUser = { accounts: { github: { id: 1234 } } } + sinon.stub(Context.prototype, 'saveAsync').resolves(ctx.mockContext) done() }) afterEach(function (done) { InfraCodeVersionService.copyInfraCodeToContextVersion.restore() - ContextVersion.createDeepCopy.restore() - Context.prototype.save.restore() + ContextVersion.createDeepCopyAsync.restore() + Context.prototype.saveAsync.restore() done() }) @@ -111,110 +114,131 @@ describe('ContextVersionService', function () { ctx.returnedMockedContextVersion = { _id: 'deadb33f', owner: { github: -1 }, - // createDeepCopy sets the correct createdBy createdBy: { github: 1234 }, infraCodeVersion: { _id: 'old' }, - save: sinon.stub().yieldsAsync() + saveAsync: sinon.stub().resolves(), + set: sinon.stub() } ctx.mockContextVersion.owner.github = process.env.HELLO_RUNNABLE_GITHUB_ID - ContextVersion.createDeepCopy.yieldsAsync(null, ctx.returnedMockedContextVersion) + ContextVersion.createDeepCopyAsync.resolves(ctx.returnedMockedContextVersion) ctx.mockContext.owner.github = process.env.HELLO_RUNNABLE_GITHUB_ID done() }) - it('should allow the body of the request to override the owner', function (done) { - // save's callback returns [ document, numberAffected ] - ctx.returnedMockedContextVersion.save.yields(null, ctx.returnedMockedContextVersion, 1) + it('should allow the body of the request to override the owner', function () { var opts = { owner: { github: 88 } } - ContextVersionService.handleVersionDeepCopy( + return ContextVersionService.handleVersionDeepCopy( ctx.mockContext, ctx.mockContextVersion, ctx.mockUser, - opts, - function (err, contextVersion) { - if (err) { return done(err) } + opts + ) + .then(contextVersion => { expect(contextVersion).to.equal(ctx.returnedMockedContextVersion) - expect(contextVersion.owner.github).to.equal(opts.owner.github) - // createdBy should _not_ be overridden - expect(contextVersion.createdBy.github).to.not.equal(opts.owner.github) - expect(contextVersion.createdBy.github).to.equal(ctx.mockUser.accounts.github.id) - done() + sinon.assert.calledWith( + ctx.returnedMockedContextVersion.set, + 'context', + ctx.mockContext._id + ) + sinon.assert.calledWith( + ctx.returnedMockedContextVersion.set, + 'owner', + { github: 88 } + ) }) }) - it('should do a hello runnable copy', function (done) { - // save's callback returns [ document, numberAffected ] - ctx.returnedMockedContextVersion.save.yields(null, ctx.returnedMockedContextVersion, 1) - ContextVersionService.handleVersionDeepCopy( + it('should do a hello runnable copy', function () { + return ContextVersionService.handleVersionDeepCopy( ctx.mockContext, ctx.mockContextVersion, - ctx.mockUser, - function (err, contextVersion) { - if (err) { return done(err) } + ctx.mockUser + ) + .then(contextVersion => { // the contextVersion that we receive should be the new one we 'creatd' expect(contextVersion).to.equal(ctx.returnedMockedContextVersion) - sinon.assert.calledOnce(ContextVersion.createDeepCopy) + sinon.assert.calledOnce(ContextVersion.createDeepCopyAsync) sinon.assert.calledWith( - ContextVersion.createDeepCopy, + ContextVersion.createDeepCopyAsync, ctx.mockUser, - ctx.mockContextVersion, - sinon.match.func) - sinon.assert.calledOnce(ctx.returnedMockedContextVersion.save) - expect(ctx.returnedMockedContextVersion.owner.github).to.equal(ctx.mockUser.accounts.github.id) - sinon.assert.calledOnce(Context.prototype.save) + ctx.mockContextVersion + ) + sinon.assert.calledOnce(ctx.returnedMockedContextVersion.saveAsync) + sinon.assert.calledOnce(Context.prototype.saveAsync) sinon.assert.calledOnce(InfraCodeVersionService.copyInfraCodeToContextVersion) sinon.assert.calledWith(InfraCodeVersionService.copyInfraCodeToContextVersion, ctx.returnedMockedContextVersion, - ctx.mockContextVersion.infraCodeVersion._id) - done() + ctx.mockContextVersion.infraCodeVersion + ) }) }) it('should propogate save contextVersion failures', function (done) { var error = new Error('Whoa!') - ctx.returnedMockedContextVersion.save.yieldsAsync(error) - ContextVersionService.handleVersionDeepCopy(ctx.mockContext, ctx.mockContextVersion, ctx.mockUser, function (err) { - expect(err).to.equal(error) - sinon.assert.calledOnce(ctx.returnedMockedContextVersion.save) - done() - }) + ctx.returnedMockedContextVersion.saveAsync.rejects(error) + ContextVersionService.handleVersionDeepCopy( + ctx.mockContext, + ctx.mockContextVersion, + ctx.mockUser + ) + .asCallback(function (err) { + expect(err).to.equal(error) + sinon.assert.calledOnce(ctx.returnedMockedContextVersion.saveAsync) + done() + }) }) it('should propogate contextVersion.createDeepCopy failures', function (done) { var error = new Error('Whoa Nelly!') - ContextVersion.createDeepCopy.yieldsAsync(error) - ContextVersionService.handleVersionDeepCopy(ctx.mockContext, ctx.mockContextVersion, ctx.mockUser, function (err) { - expect(err).to.equal(error) - sinon.assert.calledOnce(ContextVersion.createDeepCopy) - sinon.assert.notCalled(ctx.returnedMockedContextVersion.save) - done() - }) + ContextVersion.createDeepCopyAsync.rejects(error) + ContextVersionService.handleVersionDeepCopy( + ctx.mockContext, + ctx.mockContextVersion, + ctx.mockUser + ) + .asCallback(function (err) { + expect(err).to.equal(error) + sinon.assert.calledOnce(ContextVersion.createDeepCopyAsync) + sinon.assert.notCalled(ctx.returnedMockedContextVersion.saveAsync) + done() + }) }) }) describe('a CV owned by a any user, not hellorunnable', function () { it('should do a regular deep copy', function (done) { - ContextVersionService.handleVersionDeepCopy(ctx.mockContext, ctx.mockContextVersion, ctx.mockUser, function (err) { - if (err) { return done(err) } - sinon.assert.calledOnce(ContextVersion.createDeepCopy) - sinon.assert.calledWith( - ContextVersion.createDeepCopy, - ctx.mockUser, - ctx.mockContextVersion, - sinon.match.func) - done() - }) + ContextVersionService.handleVersionDeepCopy( + ctx.mockContext, + ctx.mockContextVersion, + ctx.mockUser + ) + .asCallback(function (err) { + if (err) { return done(err) } + sinon.assert.calledOnce(ContextVersion.createDeepCopyAsync) + sinon.assert.calledWith( + ContextVersion.createDeepCopyAsync, + ctx.mockUser, + ctx.mockContextVersion + ) + done() + }) }) it('should propogate copy failures', function (done) { var error = new Error('foobar') - ContextVersion.createDeepCopy.yieldsAsync(error) - ContextVersionService.handleVersionDeepCopy(ctx.mockContext, ctx.mockContextVersion, ctx.mockUser, function (err) { - expect(err).to.equal(error) - sinon.assert.calledOnce(ContextVersion.createDeepCopy) - done() - }) + ContextVersion.createDeepCopyAsync.rejects(error) + + ContextVersionService.handleVersionDeepCopy( + ctx.mockContext, + ctx.mockContextVersion, + ctx.mockUser + ) + .asCallback(function (err) { + expect(err).to.equal(error) + sinon.assert.calledOnce(ContextVersion.createDeepCopyAsync) + done() + }) }) }) }) From 8e56f491ed4fe1f689cb7fe54ac9f22fce8a8054 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 18 May 2017 15:13:41 -0700 Subject: [PATCH 3/3] more fixing --- lib/models/services/build-service.js | 2 +- unit/models/services/build-service.js | 7 +++---- unit/models/services/instance-fork-service.js | 7 +++---- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/models/services/build-service.js b/lib/models/services/build-service.js index ffe98697b..75153c924 100644 --- a/lib/models/services/build-service.js +++ b/lib/models/services/build-service.js @@ -482,7 +482,7 @@ BuildService.createNewContextVersion = function (instance, pushInfo) { github: contextOwnerGithubId } } - ContextVersionService.handleVersionDeepCopy( + return ContextVersionService.handleVersionDeepCopy( context, contextVersion, user, diff --git a/unit/models/services/build-service.js b/unit/models/services/build-service.js index 0cdfd3145..6e331716d 100644 --- a/unit/models/services/build-service.js +++ b/unit/models/services/build-service.js @@ -742,7 +742,7 @@ describe('BuildService', function () { _id: 21 } sinon.stub(Context, 'findOne').yieldsAsync(null, mockContext) - sinon.stub(ContextVersionService, 'handleVersionDeepCopy').yieldsAsync(null, mockContextVersion) + sinon.stub(ContextVersionService, 'handleVersionDeepCopy').resolves(mockContextVersion) sinon.stub(ContextVersion, 'modifyAppCodeVersionByRepo').yieldsAsync(null, mockContextVersion) done() }) @@ -847,7 +847,7 @@ describe('BuildService', function () { describe('in ContextVersionService.handleVersionDeepCopy', function () { beforeEach(function (done) { error = new Error('robot') - ContextVersionService.handleVersionDeepCopy.yieldsAsync(error) + ContextVersionService.handleVersionDeepCopy.rejects(error) done() }) @@ -913,8 +913,7 @@ describe('BuildService', function () { mockContext, // returned from `findOne` contextVersion, // from the Instance { accounts: { github: { id: 7 } } }, // from pushInfo (like sessionUser) - { owner: { github: 14 } }, // from mockContext.owner.github (owner object) - sinon.match.func + { owner: { github: 14 } } // from mockContext.owner.github (owner object) ) sinon.assert.calledOnce(ContextVersion.modifyAppCodeVersionByRepo) sinon.assert.calledWithExactly( diff --git a/unit/models/services/instance-fork-service.js b/unit/models/services/instance-fork-service.js index 41e891226..1a4acb051 100644 --- a/unit/models/services/instance-fork-service.js +++ b/unit/models/services/instance-fork-service.js @@ -306,7 +306,7 @@ describe('InstanceForkService', function () { mockNewContextVersion = {} mockNewContextVersion.update = sinon.stub().yieldsAsync(null, mockNewContextVersion) sinon.stub(Context, 'findOne').yieldsAsync(null, mockFoundContext) - sinon.stub(ContextVersionService, 'handleVersionDeepCopy').yieldsAsync(null, mockNewContextVersion) + sinon.stub(ContextVersionService, 'handleVersionDeepCopy').resolves(mockNewContextVersion) done() }) @@ -369,7 +369,7 @@ describe('InstanceForkService', function () { it('should reject with any context version copy error', function (done) { var error = new Error('robot') - ContextVersionService.handleVersionDeepCopy.yieldsAsync(error) + ContextVersionService.handleVersionDeepCopy.rejects(error) InstanceForkService._createNewNonRepoContextVersion(mockContextVersion, mockOwnerId, mockCreatedById) .asCallback(function (err) { expect(err).to.exist() @@ -414,8 +414,7 @@ describe('InstanceForkService', function () { mockFoundContext, mockContextVersion, { accounts: { github: { id: mockCreatedById } } }, - { owner: { github: mockOwnerId } }, - sinon.match.func + { owner: { github: mockOwnerId } } ) done() })