From a507941c8d6391a33788ae9be14466d259b537b1 Mon Sep 17 00:00:00 2001 From: Carl Bennett Date: Mon, 31 Aug 2015 10:22:18 +1200 Subject: [PATCH 1/5] Add beforeDelete transform The id list is deliberately not replaced with the value returned from the transform as the user should not be able to silently remove ids from the id list. Therefore, this 'transform' should be used purely as a way to throw errors when the requested deletion is not allowed. Closes #66 --- src/ResourceTypeRegistry.js | 2 +- src/controllers/API.js | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/ResourceTypeRegistry.js b/src/ResourceTypeRegistry.js index 43ea8d9b..70669608 100644 --- a/src/ResourceTypeRegistry.js +++ b/src/ResourceTypeRegistry.js @@ -5,7 +5,7 @@ * whose property is being retrieved/set, and the value to set it to, if any. */ const autoGetterSetterProps = ["dbAdapter", "beforeSave", "beforeRender", - "labelMappers", "defaultIncludes", "info", "parentType"]; + "beforeDelete", "labelMappers", "defaultIncludes", "info", "parentType"]; /** * To fulfill a JSON API request, you often need to know about all the resources diff --git a/src/controllers/API.js b/src/controllers/API.js index 6a7a94f9..b62aa517 100644 --- a/src/controllers/API.js +++ b/src/controllers/API.js @@ -2,6 +2,7 @@ import co from "co"; import Response from "../types/HTTP/Response"; import Document from "../types/Document"; +import Resource from "../types/Resource"; import Collection from "../types/Collection"; import APIError from "../types/APIError"; @@ -114,6 +115,24 @@ class APIController { } } + if (request.method === "delete") { + let toTransform; + + if (Array.isArray(request.idOrIds)) { + toTransform = new Collection( + request.idOrIds.map((id) => new Resource(request.type, id)) + ); + } + + else if (typeof request.idOrIds === "string") { + toTransform = new Resource(request.type, request.idOrIds); + } + + yield applyTransform( + toTransform, "beforeDelete", registry, frameworkReq, frameworkRes + ); + } + // Actually fulfill the request! // If we've already populated the primary resources, which is possible // because the label may have mapped to no id(s), we don't need to query. From 6de4d673fb9e1f8bce8e73352a4d51e0e6911de6 Mon Sep 17 00:00:00 2001 From: Carl Bennett Date: Mon, 31 Aug 2015 10:59:30 +1200 Subject: [PATCH 2/5] Add deletion integration tests --- test/app/database/index.js | 7 ++-- test/app/src/resource-descriptions/schools.js | 8 ++++- test/integration/delete-resource/index.js | 35 +++++++++++++++++++ test/integration/index.js | 1 + 4 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 test/integration/delete-resource/index.js diff --git a/test/app/database/index.js b/test/app/database/index.js index ca748b9d..4662ebc2 100644 --- a/test/app/database/index.js +++ b/test/app/database/index.js @@ -9,8 +9,10 @@ import OrganizationModelSchema from "./models/organization"; /*eslint-disable new-cap */ const ObjectId = mongoose.Types.ObjectId; const govtId = ObjectId("54419d550a5069a2129ef254"); +const otherGovtId = ObjectId("54419d550a5069a2129ef255"); const smithId = ObjectId("53f54dd98d1e62ff12539db2"); const doeId = ObjectId("53f54dd98d1e62ff12539db3"); +const stateCollegeId = ObjectId("53f54dd98d1e62ff12539db4"); /*eslint-enable new-cap */ const OrganizationModel = OrganizationModelSchema.model; @@ -29,11 +31,12 @@ fixtures.save("all", { { name: "Doug Wilson", gender: "male" } ], Organization: [ - {name: "State Government", description: "Representing the good people.", liaisons: [doeId, smithId], _id: govtId} + {name: "State Government", description: "Representing the good people.", liaisons: [doeId, smithId], _id: govtId}, + {name: "Other State Government", description: "Representing the other good people.", _id: otherGovtId} ], School: [ {name: "City College", description: "Just your average local college.", liaisons: [smithId]}, - {name: "State College", description: "Just your average state college."} + {name: "State College", description: "Just your average state college.", _id: stateCollegeId} ] }); diff --git a/test/app/src/resource-descriptions/schools.js b/test/app/src/resource-descriptions/schools.js index 6c71421e..7bc56285 100755 --- a/test/app/src/resource-descriptions/schools.js +++ b/test/app/src/resource-descriptions/schools.js @@ -1,4 +1,6 @@ import { Promise } from "q"; +import API from "../../../../../index"; +let APIError = API.types.Error; module.exports = { parentType: "organizations", @@ -23,10 +25,14 @@ module.exports = { } }, - beforeSave: function(resource) { + beforeSave(resource) { return new Promise((resolve, reject) => { resource.attrs.description = "Modified in a Promise"; resolve(resource); }); + }, + + beforeDelete(resource) { + throw new APIError(403, undefined, "You are not allowed to delete schools."); } }; diff --git a/test/integration/delete-resource/index.js b/test/integration/delete-resource/index.js new file mode 100644 index 00000000..4aa9d042 --- /dev/null +++ b/test/integration/delete-resource/index.js @@ -0,0 +1,35 @@ +import {expect} from "chai"; +import AgentPromise from "../../app/agent"; + +describe("Delete Resource", () => { + let Agent; + + before((done) => { + AgentPromise.then((A) => { + Agent = A; + done(); + }).catch(done); + }); + + describe("Valid deletion", () => { + it("should return 204", (done) => { + Agent.request("DEL", "/organizations/54419d550a5069a2129ef255") + .promise() + .then((res) => { + expect(res.status).to.equal(204); + done(); + }).catch(done); + }); + }); + + describe("Invalid deletion", () => { + it("should return 403", (done) => { + Agent.request("DEL", "/schools/53f54dd98d1e62ff12539db4") + .promise() + .then(done, (err) => { + expect(err.status).to.equal(403); + done(); + }).catch(done); + }); + }); +}); diff --git a/test/integration/index.js b/test/integration/index.js index b2aaef43..541a1a4c 100644 --- a/test/integration/index.js +++ b/test/integration/index.js @@ -16,6 +16,7 @@ before((done) => { require("./content-negotiation"); require("./fetch-collection"); require("./create-resource"); + require("./delete-resource"); done(); }).done(); }); From 8c0a5a0c203029965565bcd40ac4b3bc305b7da1 Mon Sep 17 00:00:00 2001 From: Carl Bennett Date: Mon, 31 Aug 2015 11:48:06 +1200 Subject: [PATCH 3/5] Add documentation --- README.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/README.md b/README.md index 24cdff12..15821f0a 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,8 @@ To use this library, you describe the special behavior (if any) that resources o - `beforeSave` (optional): a function called on each resource provided by the client (i.e. in a `POST` or `PATCH` request) before it's sent to the adapter for saving. You can transform the data here as necessary or pre-emptively reject the request. [Usage details](https://github.com/ethanresnick/json-api-example/blob/master/src/resource-descriptions/people.js#L25). +- `beforeDelete` (optional): a function called on each resource to be deleted, allowing you to throw an error if the action is not allowed. See [Handling Errors](#handling-errors) for more information. + - `labelMappers` (optional): this lets you create urls (or, in REST terminology, resources) that map to different database items over time. For example, you could have an `/events/upcoming` resource or a `/users/me` resource. In those examples, "upcoming" and "me" are called the labels and, in labelMappers, you provide a function that maps each label to the proper database id(s) at any given time. The function can return a Promise if needed. - `parentType` (optional): this allows you to designate one resource type being a sub-type of another (its `parentType`). This is often used when you have two resource types that live in the same database table/collection, and their type is determined with a discriminator key. See the [`schools` type](https://github.com/ethanresnick/json-api-example/blob/master/src/resource-descriptions/schools.js#L2) in the example repository. @@ -107,5 +109,31 @@ This library gives you a Front controller (shown in the example) that can handle In the example above, routing is handled with Express's built-in `app[VERB]` methods, and the three parameters are set properly using express's built-in `:param` syntax. If you're looking for something more robust, you might be interested in [Express Simple Router](https://github.com/ethanresnick/express-simple-router). For authentication, check out [Express Simple Firewall](https://github.com/ethanresnick/express-simple-firewall). +## Handling Errors + +This library provides an error contructor and a handler that you can use to return JSON API-compliant errors to the user. For example, here is how you would handle 404s. + +```javascript +var API = require("json-api"); +var APIError = API.types.Error; + +// Your route definitions would go here, but if none match... + +app.use(function(req, res, next) { + var err = new APIError(404, undefined, "Not Found"); + Front.sendError(err, req, res); +}); +``` + +You can also throw an APIError inside `beforeSave`, `beforeRender`, and `beforeDelete` transforms to cancel the request. + +```javascript +beforeDelete: function(resource, req, res, superFn) { + if (req.user.role !== "admin") { + throw APIError(403, undefined, "You are not allowed to delete this resource."); + } +} +``` + ## Database Adapters An adapter handles all the interaction with the database. It is responsible for turning requests into standard [`Resource`](https://github.com/ethanresnick/json-api/blob/master/src/types/Resource.js) or [`Collection`](https://github.com/ethanresnick/json-api/blob/master/src/types/Collection.js) objects that the rest of the library will use. See the built-in [MongooseAdapter](https://github.com/ethanresnick/json-api/blob/master/src/db-adapters/Mongoose/MongooseAdapter.js) for an example. From e9ef5f2333833f01c414e7e28f201e41d95dc556 Mon Sep 17 00:00:00 2001 From: Carl Bennett Date: Mon, 7 Sep 2015 14:06:31 +1200 Subject: [PATCH 4/5] README update as per review --- README.md | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/README.md b/README.md index 15821f0a..2b796a8b 100644 --- a/README.md +++ b/README.md @@ -111,19 +111,7 @@ In the example above, routing is handled with Express's built-in `app[VERB]` met ## Handling Errors -This library provides an error contructor and a handler that you can use to return JSON API-compliant errors to the user. For example, here is how you would handle 404s. - -```javascript -var API = require("json-api"); -var APIError = API.types.Error; - -// Your route definitions would go here, but if none match... - -app.use(function(req, res, next) { - var err = new APIError(404, undefined, "Not Found"); - Front.sendError(err, req, res); -}); -``` +This library provides an error contructor and a handler that you can use to return JSON API-compliant errors to the user. For an example, please see the [example repo](https://github.com/ethanresnick/json-api-example/blob/master/src/index.js#L64). You can also throw an APIError inside `beforeSave`, `beforeRender`, and `beforeDelete` transforms to cancel the request. From 0f5d81dff66a067dad5a7df27b57afc1203b8917 Mon Sep 17 00:00:00 2001 From: Carl Bennett Date: Fri, 11 Sep 2015 08:45:18 +1200 Subject: [PATCH 5/5] Add extra test --- test/integration/delete-resource/index.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/integration/delete-resource/index.js b/test/integration/delete-resource/index.js index 4aa9d042..bb8dc90e 100644 --- a/test/integration/delete-resource/index.js +++ b/test/integration/delete-resource/index.js @@ -20,6 +20,15 @@ describe("Delete Resource", () => { done(); }).catch(done); }); + + it("should have deleted the resource", (done) => { + Agent.request("GET", "/organizations/54419d550a5069a2129ef255") + .promise() + .then(done, (err) => { + expect(err.status).to.equal(404); + done(); + }).catch(done); + }) }); describe("Invalid deletion", () => {