From 0d83ac09fcecf4c8e163bb237e59fcd7997bf495 Mon Sep 17 00:00:00 2001 From: Tyler Renelle Date: Wed, 14 Aug 2013 21:50:17 -0400 Subject: [PATCH] api; on batch-update, override res.send & res.json to call the series callback instead, and simply skip error paths. we need to send down the error eventually --- src/server/api.coffee | 113 +++++++++++++++++++---------------- src/server/index.coffee | 1 + src/server/middleware.coffee | 6 +- src/server/routes.coffee | 46 ++++++-------- test/api.mocha.coffee | 48 +++++++++------ 5 files changed, 113 insertions(+), 101 deletions(-) diff --git a/src/server/api.coffee b/src/server/api.coffee index 59381f0b33..efc3b8d401 100644 --- a/src/server/api.coffee +++ b/src/server/api.coffee @@ -34,15 +34,12 @@ NO_USER_FOUND = err: "No user found." api.auth = (req, res, next) -> uid = req.headers['x-api-user'] token = req.headers['x-api-key'] - return res.json 401, NO_TOKEN_OR_UID unless uid and token + return res.json(401, NO_TOKEN_OR_UID) unless uid and token - model = req.getModel() - query = model.query('users').withIdAndToken(uid, token) - - query.fetch (err, user) -> - return res.json err: err if err + req.getModel().query('users').withIdAndToken(uid, token).fetch (err, user) -> + return res.json(500, {err}) if err (req.habit ?= {}).user = user - return res.json 401, NO_USER_FOUND if _.isEmpty(user.get()) + return res.json(401, NO_USER_FOUND) if _.isEmpty(user.get()) req._isServer = true next() @@ -81,8 +78,8 @@ api.scoreTask = (req, res, next) -> {id, direction} = req.params # Send error responses for improper API call - return res.json 500, {err: ':id required'} unless id - return res.json 500, {err: ":direction must be 'up' or 'down'"} unless direction in ['up','down'] + return res.json(500, {err: ':id required'}) unless id + return res.json(500, {err: ":direction must be 'up' or 'down'"}) unless direction in ['up','down'] {user} = req.habit @@ -90,7 +87,7 @@ api.scoreTask = (req, res, next) -> # TODO - could modify batchTxn to conform to this better delta = score req.getModel(), user, id, direction, -> result = user.get('stats') - sendResult req, next, 200, _.extend(result, delta: delta) + res.json 200, _.extend(result, delta: delta) # Set completed if type is daily or todo and task exists if (existing = user.at "tasks.#{id}").get() @@ -120,15 +117,15 @@ api.getTasks = (req, res, next) -> if /^(habit|todo|daily|reward)$/.test(req.query.type) then [req.query.type] else ['habit','todo','daily','reward'] tasks = _.toArray (_.filter req.habit.user.get('tasks'), (t)-> t.type in types) - sendResult req, next, 200, tasks + res.json 200, tasks ### Get Task ### api.getTask = (req, res, next) -> task = req.habit.user.get "tasks.#{req.params.id}" - return res.json 400, err: "No task found." if !task || _.isEmpty(task) - sendResult req, next, 200, task + return res.json(400, err: "No task found.") if !task || _.isEmpty(task) + res.json 200, task ### Validate task @@ -140,7 +137,7 @@ api.validateTask = (req, res, next) -> # If we're updating, get the task from the user if req.method is 'PUT' or req.method is 'DELETE' task = req.habit.user.get "tasks.#{req.params.id}" - return res.json 400, err: "No task found." if !task || _.isEmpty(task) + return res.json(400, err: "No task found.") if !task || _.isEmpty(task) # Strip for now type = undefined delete newTask.type @@ -148,7 +145,7 @@ api.validateTask = (req, res, next) -> newTask.value = sanitize(value).toInt() newTask.value = 0 if isNaN newTask.value unless /^(habit|todo|daily|reward)$/.test type - return res.json 400, err: 'type must be habit, todo, daily, or reward' + return res.json(400, err: 'type must be habit, todo, daily, or reward') newTask.text = sanitize(text).xss() if typeof text is "string" newTask.notes = sanitize(notes).xss() if typeof notes is "string" @@ -169,14 +166,14 @@ api.validateTask = (req, res, next) -> ### api.deleteTask = (req, res, next) -> deleteTask req.habit.user, req.habit.task, -> - sendResult req, next, 204 + res.send 204 ### Update Task ### api.updateTask = (req, res, next) -> req.habit.user.set "tasks.#{req.habit.task.id}", req.habit.task, -> - sendResult req, next, 200, req.habit.task + res.json 200, req.habit.task ### Update tasks (plural). This will update, add new, delete, etc all at once. @@ -206,12 +203,12 @@ api.updateTasks = (req, res, next) -> true async.series series, -> - sendResult req, next, 201, tasks + res.json 201, tasks api.createTask = (req, res, next) -> task = req.habit.task addTask req.habit.user, task, -> - sendResult req, next, 201, task + res.json 201, task api.sortTask = (req, res, next) -> {id} = req.params @@ -230,13 +227,13 @@ api.sortTask = (req, res, next) -> api.buy = (req, res, next) -> type = req.params.type unless type in ['weapon', 'armor', 'head', 'shield'] - return res.json 400, err: ":type must be in one of: 'weapon', 'armor', 'head', 'shield'" + return res.json(400, err: ":type must be in one of: 'weapon', 'armor', 'head', 'shield'") hasEnough = true done = -> if hasEnough - sendResult req, res, 200, req.habit.user.get("items") + res.json 200, req.habit.user.get("items") else - sendResult req, next, 200, {err: "Not enough GP"} + res.json 200, {err: "Not enough GP"} misc.batchTxn req.getModel(), (uObj, paths) -> hasEnough = items.buyItem(uObj, type, {paths}) ,{user:req.habit.user, done} @@ -255,17 +252,18 @@ api.registerUser = (req, res, next) -> {email, username, password, confirmPassword} = req.body unless username and password and email - return sendResult req, next, 401, err: ":username, :email, :password, :confirmPassword required" + return res.json 401, err: ":username, :email, :password, :confirmPassword required" if password isnt confirmPassword - return sendResult req, next, 401, err: ":password and :confirmPassword don't match" + return res.json 401, err: ":password and :confirmPassword don't match" try validator.check(email).isEmail() catch e - return sendResult req, next, 401, err: e.message + return res.json 401, err: e.message model = req.getModel() async.waterfall [ - (cb) -> model.query('users').withEmail(email).fetch cb + (cb) -> + model.query('users').withEmail(email).fetch(cb) , (user, cb) -> return cb("Email already taken") if user.get() @@ -280,9 +278,10 @@ api.registerUser = (req, res, next) -> newUser.auth.timestamps = {created: +new Date} req._isServer = true id = model.add "users", newUser, (err) -> cb(err, id) - ], (err, id) -> - return sendResult req, next, 401, {err} if err - sendResult req, next, 200, model.get("users.#{id}") + ] + , (err, id) -> + return res.json(401, {err}) if err + res.json 200, model.get("users.#{id}") ### Get User @@ -298,34 +297,33 @@ api.getUser = (req, res, next) -> delete uObj.auth.hashed_password delete uObj.auth.salt - sendResult req, next, 200, uObj + res.json(200, uObj) ### Register new user with uname / password ### api.loginLocal = (req, res, next) -> - username = req.body.username - password = req.body.password - return res.json 401, err: 'No username or password' unless username and password + {username, password} = req.body + return res.json(401, err: 'No username or password') unless username and password model = req.getModel() q = model.query("users").withUsername(username) q.fetch (err, result1) -> - return res.json 401, { err } if err + return res.json(401, {err}) if err u1 = result1.get() - return res.json 401, err: 'Username not found' unless u1 # user not found + return res.json(401, err: 'Username not found') unless u1 # user not found # We needed the whole user object first so we can get his salt to encrypt password comparison q = model.query("users").withLogin(username, utils.encryptPassword(password, u1.auth.local.salt)) q.fetch (err, result2) -> - return res.json 401, { err } if err + return res.json(401, {err}) if err # joshua tree? u2 = result2.get() - return res.json 401, err: 'Incorrect password' unless u2 + return res.json(401, err: 'Incorrect password') unless u2 - sendResult req, next, 200, + res.json 200, id: u2.id token: u2.apiToken @@ -334,19 +332,19 @@ api.loginLocal = (req, res, next) -> ### api.loginFacebook = (req, res, next) -> {facebook_id, email, name} = req.body - return res.json 401, err: 'No facebook id provided' unless facebook_id + return res.json(401, err: 'No facebook id provided') unless facebook_id model = req.getModel() q = model.query("users").withProvider('facebook', facebook_id) q.fetch (err, result) -> - return res.json 401, { err } if err + return res.json(401, {err}) if err u = result.get() if u - sendResult req, next, 200, + res.json 200, id: u.id token: u.apiToken else # FIXME: create a new user instead - return res.json 403, err: "Please register with Facebook on https://habitrpg.com, then come back here and log in." + return res.json(403, err: "Please register with Facebook on https://habitrpg.com, then come back here and log in.") ### Update user @@ -367,7 +365,7 @@ api.updateUser = (req, res, next) -> series.push (cb) -> req.habit.user.set(k, v, cb) async.series series, (err) -> return next(err) if err - sendResult req, next, 200, helpers.derbyUserToAPI(user) + res.json 200, helpers.derbyUserToAPI(user) api.cron = (req, res, next) -> {user} = req.habit @@ -379,7 +377,7 @@ api.cron = (req, res, next) -> api.revive = (req, res, next) -> {user} = req.habit done = -> - sendResult req, res, 200, helpers.derbyUserToAPI(user) + res.json 200, helpers.derbyUserToAPI(user) misc.batchTxn req.getModel(), (uObj, paths) -> algos.revive uObj, {paths} , {user, done} @@ -394,6 +392,9 @@ api.revive = (req, res, next) -> api.batchUpdate = (req, res, next) -> {user} = req.habit + oldSend = res.send + oldJson = res.json + performAction = (action, cb) -> # TODO come up with a more consistent approach here. like: # req.body=action.data; delete action.data; _.defaults(req.params, action) @@ -403,23 +404,28 @@ api.batchUpdate = (req, res, next) -> req.params.type = action.type req.body = action.data + res.send = res.json = (code, data) -> + console.error({code, data}) if _.isNumber(code) and code >= 400 + #FIXME send error messages down + cb() + switch action.op when "score" - api.scoreTask(req, res, cb) + api.scoreTask(req, res) when "buy" - api.buy(req, res, cb) + api.buy(req, res) when "sortTask" - api.sortTask(req, res, cb) + api.sortTask(req, res) when "addTask" api.validateTask req, res, -> - api.createTask(req, res, cb) + api.createTask(req, res) when "delTask" api.validateTask req, res, -> - api.deleteTask(req, res, cb) + api.deleteTask(req, res) when "set" - api.updateUser(req, res, cb) + api.updateUser(req, res) when "revive" - api.revive(req, res, cb) + api.revive(req, res) else cb() # Setup the array of functions we're going to call in parallel with async @@ -429,7 +435,8 @@ api.batchUpdate = (req, res, next) -> # call all the operations, then return the user object to the requester async.series actions, (err) -> - return res.json 500, {err} if err - res.json helpers.derbyUserToAPI(user) + res.json = oldJson; res.send = oldSend + return res.json(500, {err}) if err + res.json 200, helpers.derbyUserToAPI(user) console.log "Reply sent" diff --git a/src/server/index.coffee b/src/server/index.coffee index b235c949eb..b94a63641d 100644 --- a/src/server/index.coffee +++ b/src/server/index.coffee @@ -76,6 +76,7 @@ mongo_store = new MongoStore {url: process.env.NODE_DB_URI}, -> .use(store.modelMiddleware()) .use(middleware.translate) # API should be hit before all other routes + .use(middleware.apiv1Middleware) .use('/api/v1', require('./routes').middleware) .use(require('./deprecated').middleware) # Show splash page for newcomers diff --git a/src/server/middleware.coffee b/src/server/middleware.coffee index fa2ca2aef5..ab8ae9902f 100644 --- a/src/server/middleware.coffee +++ b/src/server/middleware.coffee @@ -32,4 +32,8 @@ translate = (req, res, next) -> next() -module.exports = { splash, view, allowCrossDomain, translate} \ No newline at end of file +apiv1Middleware = (req, res, next) -> + req.habit ?= {} + next() + +module.exports = { splash, view, allowCrossDomain, translate, apiv1Middleware} \ No newline at end of file diff --git a/src/server/routes.coffee b/src/server/routes.coffee index 1a6b938a47..2ea99cc71b 100644 --- a/src/server/routes.coffee +++ b/src/server/routes.coffee @@ -14,45 +14,33 @@ api = require './api' {auth, validateTask, cron} = api -### - We don't want the api functions to actually res.send results (unless there was an error) - because we'll be re-using the same functions when apiv2 rolls around, but returning different results. - So handle sending results for apiv1 here -### -v1Send = (req, res) -> - {result} = req.habit - if (result.code and result.data) then res.json result.code, result.data - else if result.code then res.send result.code - else if result.data then res.json result.data - else res.send 200 - router.get '/status', (req, res) -> res.json status: 'up' # Auth -router.post '/register', api.registerUser, v1Send +router.post '/register', api.registerUser # Scoring -router.post '/user/task/:id/:direction', auth, cron, api.scoreTask, v1Send -router.post '/user/tasks/:id/:direction', auth, cron, api.scoreTask, v1Send +router.post '/user/task/:id/:direction', auth, cron, api.scoreTask +router.post '/user/tasks/:id/:direction', auth, cron, api.scoreTask # Tasks -router.get '/user/tasks', auth, cron, api.getTasks, v1Send # plural -router.get '/user/task/:id', auth, cron, api.getTask, v1Send -router.put '/user/task/:id', auth, cron, validateTask, api.updateTask, v1Send -router.post '/user/tasks', auth, cron, api.updateTasks, v1Send # plural -router.delete '/user/task/:id', auth, cron, validateTask, api.deleteTask, v1Send -router.post '/user/task', auth, cron, validateTask, api.createTask, v1Send -router.put '/user/task/:id/sort', auth, cron, validateTask, api.sortTask, v1Send +router.get '/user/tasks', auth, cron, api.getTasks +router.get '/user/task/:id', auth, cron, api.getTask +router.put '/user/task/:id', auth, cron, validateTask, api.updateTask +router.post '/user/tasks', auth, cron, api.updateTasks +router.delete '/user/task/:id', auth, cron, validateTask, api.deleteTask +router.post '/user/task', auth, cron, validateTask, api.createTask +router.put '/user/task/:id/sort', auth, cron, validateTask, api.sortTask # Items -router.post '/user/buy/:type', auth, cron, api.buy, v1Send +router.post '/user/buy/:type', auth, cron, api.buy # User -router.get '/user', auth, cron, api.getUser, v1Send -router.post '/user/auth/local', api.loginLocal, v1Send -router.post '/user/auth/facebook', api.loginFacebook, v1Send -router.put '/user', auth, cron, api.updateUser, v1Send -router.post '/user/revive', auth, cron, api.revive, v1Send -router.post '/user/batch-update', auth, cron, api.batchUpdate # this one we're handling specially +router.get '/user', auth, cron, api.getUser +router.post '/user/auth/local', api.loginLocal +router.post '/user/auth/facebook', api.loginFacebook +router.put '/user', auth, cron, api.updateUser +router.post '/user/revive', auth, cron, api.revive +router.post '/user/batch-update', auth, cron, api.batchUpdate module.exports = router \ No newline at end of file diff --git a/test/api.mocha.coffee b/test/api.mocha.coffee index 80c289972e..acbae14f68 100644 --- a/test/api.mocha.coffee +++ b/test/api.mocha.coffee @@ -450,25 +450,37 @@ describe 'API', -> #expect(res.body.token).to.be newUser.apiToken done() - it 'PUT /api/v1/batch-update', (done) -> - userBefore = {} -# user.set "lastCron", +new Date #FIXME this shouldn't be handled here - query = model.query('users').withIdAndToken(currentUser.id, currentUser.apiToken) - query.fetch (err, user) -> userBefore = user.get() - #console.log {userBefore} + it 'POST /api/v1/batch-update', (done) -> + userBefore = _.cloneDeep(currentUser) - jsonRaw = - [{"op":"score","task":{"completed":true,"date":null,"down":null,"id":"049ee706-7992-408f-8bdd-a0f87b6cddee","notes":null,"price":null,"priority":null,"streak":1,"text":"asdasd","type":"daily","up":null,"value":-15.159032750819472},"dir":"down"},{"op":"score","task":{"completed":true,"date":null,"down":null,"id":"049ee706-7992-408f-8bdd-a0f87b6cddee","notes":null,"price":null,"priority":null,"streak":1,"text":"asdasd","type":"daily","up":null,"value":-15.159032750819472},"dir":"up"},{},{},{},{},{"op":"score","task":{"completed":true,"date":null,"down":null,"id":"049ee706-7992-408f-8bdd-a0f87b6cddee","notes":null,"price":null,"priority":null,"streak":1,"text":"asdasd","type":"daily","up":null,"value":-16.63136866553572},"dir":"down"},{"op":"score","task":{"completed":true,"date":null,"down":null,"id":"049ee706-7992-408f-8bdd-a0f87b6cddee","notes":null,"price":null,"priority":null,"streak":1,"text":"asdasd","type":"daily","up":null,"value":-16.63136866553572},"dir":"up"},{},{},{"op":"score","task":{"completed":true,"date":null,"down":null,"history":[{"date":1370796966979,"value":-1.9263318037820194},{"date":1371394179245,"value":-9.632667221983818},{"date":1371987764419,"value":-8.899142684639843},{"date":1371901709065,"value":-8.697714139260505},{"date":1371987764419,"value":-9.947389352351902},{"date":1372072605105,"value":-8.65704735207246},{"date":1372185464158,"value":-9.905420946093672},{"date":1372348155721,"value":-8.616465915449927},{"date":1372404365115,"value":-7.369389857231249},{"date":1372619315210,"value":-6.16153655829917},{"date":1372797766170,"value":-4.990495966065229},{"date":1373266931264,"value":-12.356300657493207},{"date":1373322727209,"value":-10.983796434663102},{"date":1373407801484,"value":-9.658725758132753},{"date":1373639117325,"value":-8.377893411957398},{"date":1373719671601,"value":-7.138418159258412},{"date":1373826521297,"value":-5.902428899644443},{"date":1373839445467,"value":-8.264210410818091},{"date":1373929050162,"value":-8.224444248693572},{"date":1374058202835,"value":-9.459055183497052},{"date":1374102966396,"value":-8.184759693251706},{"date":1374187619046,"value":-6.951403643619735},{"date":1374342649005,"value":-5.72148344218024},{"date":1374434356841,"value":-8.072174632205511},{"date":1374562742400,"value":-10.571154014907808},{"date":1374886027789,"value":-16.097395454285916},{"date":1375011848715,"value":-17.607991906162557},{"date":1375436884647,"value":-16.037773949824242},{"date":1375563074478,"value":-17.546064223707074},{"date":1375568230260,"value":-19.11379232897715},{"date":1375734631073,"value":-20.745784396408645},{"date":1375785010434,"value":-18.767794224069412},{"date":1375826853480,"value":-18.636651213045212}],"id":"fe4b9061-eb58-468c-9b25-10c72be772e6","notes":"","price":null,"priority":null,"repeat":{"su":true,"m":true,"t":true,"w":true,"th":true,"f":true,"s":true},"streak":1,"tags":{"40492758-1202-4d85-8cb3-d40e45f4dd1d":false},"text":"Read 50 pages","type":"daily","up":null,"value":-17.024492021142215},"dir":"up"},{},{},{}] + habits = _.where currentUser.tasks, {type: 'habit'} + dailys = _.where currentUser.tasks, {type: 'dailys'} + todos = _.where currentUser.tasks, {type: 'todos'} + rewards = _.where currentUser.tasks, {type: 'rewards'} - request.put("http://localhost:1337/api/v1/batch-update") - .set('Accept', 'application/json') - .set('X-API-User', currentUser.id) - .set('X-API-Key', currentUser.apiToken) - .send(jsonRaw) - .end (res) -> - expect(res.body.err).to.be undefined - expect(res.statusCode).to.be 200 - tasks = res.body.tasks + jsonRaw = [ - done() + # Good scores + op: 'score', task: habits[0], dir: 'up' + op: 'score', task: habits[1], dir: 'down' + op: 'score', task: dailys[1], dir: 'up' + op: 'score', task: todos[1], dir: 'up' + + # Bad scores, should handle gracefully + op: 'score', task: todos[2], dir: 'down' + op: 'score', task: {}, dir: 'up' + op: 'score', task: {id:null, value: NaN}, dir: 'up' + ] + + request.post("#{baseURL}/user/batch-update") + .set('Accept', 'application/json') + .set('X-API-User', currentUser.id) + .set('X-API-Key', currentUser.apiToken) + .send(jsonRaw) + .end (res) -> + expect(res.body.err).to.be undefined + expect(res.statusCode).to.be 200 + console.log {stats:res.body.stats, tasks:res.body.tasks} + expectUserEqual(userBefore, res.body) + done()