Skip to content

Commit

Permalink
Refactored the API layer: do not handle API response after pipelining
Browse files Browse the repository at this point in the history 
no issue

- this has a big underlying problem
- each task in the pipeline can modify the options
- e.g. add a proper permission context
- if we chain after the pipeline, we don't have access to the modified options object
- and then we pass the wrong options into the `toJSON` function of a model
- the toJSON function decides what to return based on options
- this is the easiest solution for now, but i am going to write a spec if we can solve this problem differently
  • Loading branch information
@kirrg001 @ErisDS
kirrg001 authored and ErisDS committed Sep 28, 2017
1 parent a6d57d6 commit 1e2befa
Show file tree
Hide file tree
Showing 9 changed files with 224 additions and 154 deletions.
22 changes: 14 additions & 8 deletions core/server/api/clients.js
View file
Expand Up @@ -35,7 +35,19 @@ clients = {
function doQuery(options) {
// only User Agent (type = `ua`) clients are available at the moment.
options.data = _.extend(options.data, {type: 'ua'});
return models.Client.findOne(options.data, _.omit(options, ['data']));

return models.Client.findOne(options.data, _.omit(options, ['data']))
.then(function onModelResponse(model) {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: i18n.t('common.api.clients.clientNotFound')
}));
}

return {
clients: [model.toJSON(options)]
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -47,13 +59,7 @@ clients = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, options).then(function formatResponse(result) {
if (result) {
return {clients: [result.toJSON(options)]};
}

return Promise.reject(new errors.NotFoundError({message: i18n.t('common.api.clients.clientNotFound')}));
});
return pipeline(tasks, options);
}
};

Expand Down
27 changes: 17 additions & 10 deletions core/server/api/invites.js
View file
Expand Up @@ -37,7 +37,18 @@ invites = {
tasks;

function modelQuery(options) {
return models.Invite.findOne(options.data, _.omit(options, ['data']));
return models.Invite.findOne(options.data, _.omit(options, ['data']))
.then(function onModelResponse(model) {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: i18n.t('errors.api.invites.inviteNotFound')
}));
}

return {
invites: [model.toJSON(options)]
};
});
}

tasks = [
Expand All @@ -47,14 +58,7 @@ invites = {
modelQuery
];

return pipeline(tasks, options)
.then(function formatResponse(result) {
if (result) {
return {invites: [result.toJSON(options)]};
}

return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.invites.inviteNotFound')}));
});
return pipeline(tasks, options);
},

destroy: function destroy(options) {
Expand Down Expand Up @@ -131,7 +135,10 @@ invites = {
}).then(function () {
invite.set('status', 'sent');
var inviteAsJSON = invite.toJSON();
return {invites: [inviteAsJSON]};

return {
invites: [inviteAsJSON]
};
}).catch(function (error) {
if (error && error.errorType === 'EmailError') {
error.message = i18n.t('errors.api.invites.errorSendingEmail.error', {message: error.message}) + ' ' +
Expand Down
8 changes: 4 additions & 4 deletions core/server/api/notifications.js
View file
Expand Up @@ -103,7 +103,9 @@ notifications = {
}
});

return addedNotifications;
return {
notifications: addedNotifications
};
}

tasks = [
Expand All @@ -112,9 +114,7 @@ notifications = {
saveNotifications
];

return pipeline(tasks, object, options).then(function formatResponse(result) {
return {notifications: result};
});
return pipeline(tasks, object, options);
},

/**
Expand Down
81 changes: 47 additions & 34 deletions core/server/api/posts.js
View file
Expand Up @@ -89,7 +89,18 @@ posts = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Post.findOne(options.data, _.omit(options, ['data']));
return models.Post.findOne(options.data, _.omit(options, ['data']))
.then(function onModelResponse(model) {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: i18n.t('errors.api.posts.postNotFound')
}));
}

return {
posts: [model.toJSON(options)]
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -101,14 +112,7 @@ posts = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, options).then(function formatResponse(result) {
// @TODO make this a formatResponse task?
if (result) {
return {posts: [result.toJSON(options)]};
}

return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.posts.postNotFound')}));
});
return pipeline(tasks, options);
},

/**
Expand All @@ -130,7 +134,27 @@ posts = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Post.edit(options.data.posts[0], _.omit(options, ['data']));
return models.Post.edit(options.data.posts[0], _.omit(options, ['data']))
.then(function onModelResponse(model) {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: i18n.t('errors.api.posts.postNotFound')
}));
}

var post = model.toJSON(options);

// If previously was not published and now is (or vice versa), signal the change
// @TODO: `statusChanged` get's added for the API headers only. Reconsider this.
post.statusChanged = false;
if (model.updated('status') !== model.get('status')) {
post.statusChanged = true;
}

return {
posts: [post]
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -142,20 +166,7 @@ posts = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, object, options).then(function formatResponse(result) {
if (result) {
var post = result.toJSON(options);

// If previously was not published and now is (or vice versa), signal the change
post.statusChanged = false;
if (result.updated('status') !== result.get('status')) {
post.statusChanged = true;
}
return {posts: [post]};
}

return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.posts.postNotFound')}));
});
return pipeline(tasks, object, options);
},

/**
Expand All @@ -177,7 +188,17 @@ posts = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Post.add(options.data.posts[0], _.omit(options, ['data']));
return models.Post.add(options.data.posts[0], _.omit(options, ['data']))
.then(function onModelResponse(model) {
var post = model.toJSON(options);

if (post.status === 'published') {
// When creating a new post that is published right now, signal the change
post.statusChanged = true;
}

return {posts: [post]};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -189,15 +210,7 @@ posts = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, object, options).then(function formatResponse(result) {
var post = result.toJSON(options);

if (post.status === 'published') {
// When creating a new post that is published right now, signal the change
post.statusChanged = true;
}
return {posts: [post]};
});
return pipeline(tasks, object, options);
},

/**
Expand Down
43 changes: 23 additions & 20 deletions core/server/api/roles.js
View file
Expand Up @@ -39,7 +39,28 @@ roles = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Role.findAll(options);
return models.Role.findAll(options)
.then(function onModelResponse(models) {
var roles = models.map(function (role) {
return role.toJSON();
});

if (options.permissions !== 'assign') {
return {roles: roles};
}

return Promise.filter(roles.map(function (role) {
return canThis(options.context).assign.role(role)
.return(role)
.catch(function () {});
}), function (value) {
return value && value.name !== 'Owner';
}).then(function (roles) {
return {
roles: roles
};
});
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -50,25 +71,7 @@ roles = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, options).then(function formatResponse(results) {
var roles = results.map(function (r) {
return r.toJSON();
});

if (options.permissions !== 'assign') {
return {roles: roles};
}

return Promise.filter(roles.map(function (role) {
return canThis(options.context).assign.role(role)
.return(role)
.catch(function () {});
}), function (value) {
return value && value.name !== 'Owner';
}).then(function (roles) {
return {roles: roles};
});
});
return pipeline(tasks, options);
}
};

Expand Down
21 changes: 13 additions & 8 deletions core/server/api/slugs.js
View file
Expand Up @@ -57,7 +57,18 @@ slugs = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Base.Model.generateSlug(allowedTypes[options.type], options.data.name, {status: 'all'});
return models.Base.Model.generateSlug(allowedTypes[options.type], options.data.name, {status: 'all'})
.then(function onModelResponse(slug) {
if (!slug) {
return Promise.reject(new errors.GhostError({
message: i18n.t('errors.api.slugs.couldNotGenerateSlug')
}));
}

return {
slugs: [{slug: slug}]
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand All @@ -69,13 +80,7 @@ slugs = {
];

// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, options).then(function (slug) {
if (!slug) {
return Promise.reject(new errors.GhostError({message: i18n.t('errors.api.slugs.couldNotGenerateSlug')}));
}

return {slugs: [{slug: slug}]};
});
return pipeline(tasks, options);
}
};

Expand Down

0 comments on commit 1e2befa

Please sign in to comment.