Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically add OPTIONS action #59

Merged
merged 10 commits into from
Jul 2, 2015
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ By default a CORS header is sent, you can disable it with the --disableCORS swit

`drakov -f "../com/foo/contracts/*.md" --disableCORS`

## Automatic response to OPTIONS requests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge thank you for updating the README 👍 🎱


When you run server for testing API on different port than your app is it's handy to allow cross origin resource sharing (CORS). For this working you need also to listen on every route for OPTIONS requests.

`drakov -f "../com/foo/contracts/*.md" --autoOptions`

## Run on Public Interface

By default Drakov only binds to localhost, to run on the public IP interface use the --public switch.
Expand Down
3 changes: 3 additions & 0 deletions lib/arguments.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ var optimistOptions = {
public: {
description: 'Allow external requests',
default: false
},
autoOptions: {
description: 'Automatically response to OPTIONS request for used routes'
}

};
Expand Down
41 changes: 41 additions & 0 deletions lib/json/auto-options-action.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"name": "",
"description": "",
"method": "OPTIONS",
"parameters": [],
"attributes": {
"relation": "",
"uriTemplate": ""
},
"content": [],
"examples": [
{
"name": "",
"description": "",
"requests": [],
"responses": [
{
"name": "200",
"description": "",
"headers": [
{
"name": "Content-Type",
"value": "text/plain"
}
],
"body": "",
"schema": "",
"content": [
{
"element": "asset",
"attributes": {
"role": "bodyExample"
},
"content": ""
}
]
}
]
}
]
}
3 changes: 2 additions & 1 deletion lib/middleware/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ var bootstrapMiddleware = function(app, argv) {

exports.init = function(app, argv, cb) {
bootstrapMiddleware(app, argv);
routeHandlers(argv.sourceFiles, function(err, middleware) {
var options = {sourceFiles: argv.sourceFiles, autoOptions: argv.autoOptions};
routeHandlers(options, function(err, middleware) {
cb(err, middleware);
});
};
1 change: 1 addition & 0 deletions lib/middleware/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ exports.corsHeaders = function(disableCORS) {
return function(req, res, next) {
if (!disableCORS) {
res.set('Access-Control-Allow-Origin', '*');
res.set('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept');
}
next();
};
Expand Down
46 changes: 45 additions & 1 deletion lib/middleware/route-handlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ var fs = require('fs');
var protagonist = require('protagonist');
var async = require('async');
var pathToRegexp = require('path-to-regexp');
var _ = require('lodash');

var urlParser = require('../url-parser');
var route = require('../route');
var queryComparator = require('../query-comparator');

var ROUTE_MAP = null;
var autoOptions;
var autoOptionsAction = require('../json/auto-options-action.json');

var parseAction = function(uriTemplate, action) {
var parsedUrl = urlParser.parse(uriTemplate);
Expand All @@ -32,14 +35,53 @@ var parseBlueprint = function(filePath) {
return;
}

var allRoutesList = [];
result.ast.resourceGroups.forEach(function(resourceGroup){
resourceGroup.resources.forEach(function(resource){
resource.actions.forEach(function(action){
parseAction(resource.uriTemplate, action);
saveRouteToTheList(resource, action);
});
});
});

// add OPTIONS route where its missing - this must be done after all routes are parsed
if (autoOptions) {
addOptionsRoutesWhereMissing(allRoutesList);
}

cb();

/**
* Adds route and its action to the allRoutesList. It appends the action when route already exists in the list.
* @param resource Route URI
* @param action HTTP action
*/
function saveRouteToTheList(resource, action) {
// used to add options routes later
if (typeof allRoutesList[resource.uriTemplate] === 'undefined') {
allRoutesList[resource.uriTemplate] = [];
}
allRoutesList[resource.uriTemplate].push(action);
}

function addOptionsRoutesWhereMissing(allRoutes) {
var routesWithoutOptions = [];
// extracts only routes without OPTIONS
_.forIn(allRoutes, function (actions, route) {
var containsOptions = _.reduce(actions, function (previousResult, iteratedAction) {
return previousResult || (iteratedAction.method === 'OPTIONS');
}, false);
if (containsOptions === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to just test variable for falseyness, i.e. !containsOptions

routesWithoutOptions.push(route);
}
});

_.forEach(routesWithoutOptions, function (uriTemplate) {
// adds prepared OPTIONS action for route
parseAction(uriTemplate, autoOptionsAction);
});
}
});
};
};
Expand All @@ -65,7 +107,9 @@ var setup = function(sourceFiles, cb) {

};

module.exports = function(sourceFiles, cb) {
module.exports = function(options, cb) {
var sourceFiles = options.sourceFiles;
autoOptions = options.autoOptions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just declare autoOptions here with var ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is proper way to do this. Imho blueprint parser should be pulled out of route handler because its a different concern.

This way routes are registered during parsing process. I feel that the list of routes/actions should be a result of parsing and we should register them after.

This is why autoOptions flag is in global scope so I don't mess function parameters.

I will refactor it and let you review it :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep agree totally about moving the parsing out, that's definitely something we have on our "roadmap".

We want to eventually be able to modify routes at runtime.

var middleware = function(req, res, next) {
var handlers = null;
Object.keys(ROUTE_MAP).forEach(function(urlPattern) {
Expand Down
34 changes: 34 additions & 0 deletions test/api/options-auto-response-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
var helper = require('../lib');
var request = helper.getRequest();

describe('Auto OPTIONS', function () {

before(function (done) {
helper.drakov.run({sourceFiles: 'test/example/md/simple-api.md', autoOptions: true}, done);
});

after(function (done) {
helper.drakov.stop(done);
});

it('should respond for OPTIONS despite its not defined in api blueprint', function (done) {
request.options('/api/things/2')
.expect(200)
.expect('Access-Control-Allow-Origin', '*')
.end(helper.endCb(done));
});

it('should not override OPTIONS route specified in api blueprint', function (done) {
request.options('/api/things')
.expect(200)
.expect('Access-Control-Allow-Origin', 'custom-domain.com')
.end(helper.endCb(done));
});

it('should not respond for OPTIONS for paths missing in api blueprint', function (done) {
request.options('/fjselifjsleifjselij')
.expect(404)
.end(helper.endCb(done));
});

});
51 changes: 39 additions & 12 deletions test/example/md/simple-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,33 @@ Lists all the things from the API
"id": "5"
}
]

### Create a new thing [POST]

+ Request (application/json)
Create a new thing

+ Body

{
"text": "Hyperspeed jet",
}

+ Response 200 (application/json;charset=UTF-8)

+ Body

{
"text": "Hyperspeed jet",
"id": "1"
}

### Allow cross site origin [OPTIONS]

+ Response 200
+ Headers

Access-Control-Allow-Origin: custom-domain.com

## Things [/api/things/{thingId}]

Expand All @@ -59,19 +86,19 @@ Update the text of the thing

+ Body

{
"text": "Hyperspeed jet",
"id": "1"
}
{
"text": "Hyperspeed jet",
"id": "1"
}

+ Response 200 (application/json;charset=UTF-8)

+ Body

{
"text": "Hyperspeed jet",
"id": "1"
}
{
"text": "Hyperspeed jet",
"id": "1"
}

## Likes [/api/things/{thingId}/like]

Expand All @@ -95,7 +122,7 @@ Update the text of the thing

+ Body

{
"charset":"not present",
"id": "1"
}
{
"charset":"not present",
"id": "1"
}