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

Feature/search filters #11

Merged
merged 14 commits into from
Sep 15, 2015
5 changes: 2 additions & 3 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
{
"extends": "xo-space",
"extends": "xo-space/esnext",
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for using esnext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the consistency rules from xo for ES6. Tried it with iojs but that wasn't working great because some stuff was behind flags.

"rules": {
"strict": [2, "global"],
"no-var": 1
"strict": [2, "global"]
},
"ecmaFeatures": {
"arrowFunctions": true,
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,6 @@ build/Release
# Dependency directory
# https://docs.npmjs.com/misc/faq#should-i-check-my-node-modules-folder-into-git
node_modules

# VS Code config directory
.vscode
4 changes: 2 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
'use strict';

require('./lib/server')(function (err, server) {
require('./lib/server')((err, server) => {
if (err) {
return console.error(err);
}

server.start(function (err) {
server.start(err => {
if (err) {
return console.error(err);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/handlers/indexes/{name}.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = {

index.insert(req.payload)
.then(o => {
reply(o).created(req.path + '/' + o.objectID);
reply(o).created(`${req.path}/${o.objectID}`);
}, reply.error.bind(reply));
}
};
26 changes: 0 additions & 26 deletions lib/search.js

This file was deleted.

21 changes: 21 additions & 0 deletions lib/search/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const _ = require('lodash');
const elasticsearchClient = require('../elasticsearchClient');
const queryBuilder = require('./query_builder');

const mapResults = function (results) {
return _.pluck(results.hits.hits, '_source');
};

class Search {
query(indexName, body) {
return elasticsearchClient.search({
index: indexName,
body: queryBuilder.build(body)
})
.then(mapResults);
}
}

module.exports = Search;
49 changes: 49 additions & 0 deletions lib/search/query_builder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';

const buildTermFilter = function (filterParams) {
// Can't currenlty be a 1 liner due to https://github.com/nodejs/node/issues/2507
const term = {[filterParams.field]: filterParams.term};
return {term};
};

const buildRangeFilter = function (filterParams) {
const range = {[filterParams.field]: {}};
if (filterParams.range.from) {
range[filterParams.field].gte = filterParams.range.from;
}
if (filterParams.range.to) {
range[filterParams.field].lte = filterParams.range.to;
}

return {range};
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure there's a good reason but this is new to me, so curious, why return {range} rather than just range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{range} is equivalent to {range: range} but the xo rules prefer the more compact form. Unfortunately at the moment, can't do {range: {[filterParams.field]: {}}} at line 10 because of a bug in node

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah of course. Gonna take a while to get used to that one. I did spot it elsewhere where it wasn't the only property!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's new to me as well

};

const buildFilter = function (filterParams) {
if (filterParams.term) {
return buildTermFilter(filterParams);
}

if (filterParams.range) {
return buildRangeFilter(filterParams);
}
};

const buildQuery = function (searchParams) {
const query = {query: {filtered: {}}};

if (searchParams.query) {
query.query.filtered.query = {match: {_all: searchParams.query}};
}

if (searchParams.filters && searchParams.filters.length) {
query.query.filtered = {
filter: {and: searchParams.filters.map(buildFilter)}
};
}

return query;
};

module.exports = {
build: buildQuery
};
2 changes: 1 addition & 1 deletion lib/version.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ exports.register = function (server, options, next) {
path: '/version',
config: {
description: 'Returns the version of the server',
handler: function (request, reply) {
handler(request, reply) {
reply(versionResponse);
}
}
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@
"swaggerize-hapi": "^1.0.0"
},
"devDependencies": {
"babel-eslint": "^4.1.2",
"chai": "^3.2.0",
"cuid": "^1.3.8",
"enjoi": "^1.0.0",
"eslint": "^1.3.1",
"eslint-config-xo-space": "^0.4.0",
"eslint-plugin-babel": "^2.1.1",
"grunt": "^0.4.5",
"grunt-contrib-watch": "^0.6.1",
"grunt-eslint": "^17.1.0",
Expand Down
2 changes: 1 addition & 1 deletion spec/indexes_{name}.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const specRequest = require('./spec_request');
const expect = require('chai').expect;

describe('/indexes/{name}', () => {
const testIndexName = 'test_index_' + cuid();
const testIndexName = `test_index_${cuid()}`;
const testObject = {name: 'object', field: 'value'};
let createResponse;

Expand Down
103 changes: 90 additions & 13 deletions spec/indexes_{name}_query.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,108 @@ const specRequest = require('./spec_request');
const expect = require('chai').expect;

describe('/indexes/{name}/query', () => {
const testIndexName = 'test_index_' + cuid();
const testDocument = {sku: '12345'};

beforeEach(() => {
return elasticsearchClient.index({
index: testIndexName,
type: 'test_type',
body: {sku: '12345'}
})
.then(() => elasticsearchClient.indices.refresh({index: testIndexName}));
const testIndexName = `test_index_${cuid()}`;
const testIndexType = 'test_type';
const document1 = {sku: '12345', price: 1};
const document2 = {sku: '98765', price: 5};

before(() => {
return elasticsearchClient.bulk({
body: [
{index: {_index: testIndexName, _type: testIndexType, _id: 1}},
document1,
{index: {_index: testIndexName, _type: testIndexType, _id: 2}},
document2
]
}).then(() => elasticsearchClient.indices.refresh({index: testIndexName}));
});

afterEach(() => {
after(() => {
return elasticsearchClient.indices.delete({index: testIndexName});
});

describe('post', () => {
it('queries by keyword', () => {
const payload = {query: '12345'};

return specRequest({url: `/1/indexes/${testIndexName}/query`, method: 'post', payload: payload})
return specRequest({url: `/1/indexes/${testIndexName}/query`, method: 'post', payload})
.then(response => {
expect(response.result).to.be.deep.equal([document1]);
expect(response.statusCode).to.equal(200);
});
});

it('filters results by terms', () => {
const payload = {filters: [{field: 'sku', term: '12345'}]};

return specRequest({url: `/1/indexes/${testIndexName}/query`, method: 'post', payload})
.then(response => {
expect(response.result).to.be.deep.equal([document1]);
expect(response.statusCode).to.equal(200);
});
});

it('filters results by ranges', () => {
const payload = {filters: [{field: 'sku', range: {from: 2}}]};

return specRequest({url: `/1/indexes/${testIndexName}/query`, method: 'post', payload})
.then(response => {
expect(response.result).to.be.deep.equal([testDocument]);
expect(response.result).to.be.deep.equal([document2]);
expect(response.statusCode).to.equal(200);
});
});

describe('validation', () => {
it('ensures query is a string', () => {
const payload = {query: {}};

return specRequest({url: '/1/indexes/test-index/query', method: 'post', payload})
.then(response => {
expect(response.result.message).to.match(/"query" must be a string]/);
expect(response.statusCode).to.equal(400);
});
});

it('ensures filters is an array', () => {
const payload = {filters: {}};

return specRequest({url: '/1/indexes/test-index/query', method: 'post', payload})
.then(response => {
expect(response.result.message).to.match(/"filters" must be an array]/);
expect(response.statusCode).to.equal(400);
});
});

it('ensures filter has field', () => {
const payload = {filters: [{term: '12345'}]};

return specRequest({url: '/1/indexes/test-index/query', method: 'post', payload})
.then(response => {
expect(response.result.message).to.match(/"field" is required]/);
expect(response.statusCode).to.equal(400);
});
});

it('ensures filter has term or range', () => {
const payload = {filters: [{field: 'sku'}]};

return specRequest({url: '/1/indexes/test-index/query', method: 'post', payload})
.then(response => {
expect(response.result.message).to.match(/"0" must have at least 2 children/);
expect(response.statusCode).to.equal(400);
});
});

it('ensures range filter has at least 1 bound', () => {
const payload = {filters: [{field: 'sku', range: {}}]};

return specRequest({url: `/1/indexes/test-index/query`, method: 'post', payload})
.then(response => {
expect(response.result.message).to.match(/"range" must have at least 1 children/);
expect(response.statusCode).to.equal(400);
});
});
});
});
});

4 changes: 2 additions & 2 deletions spec/indexes_{name}_{objectID}.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const specRequest = require('./spec_request');
const expect = require('chai').expect;

describe('/indexes/{name}/{objectID}', () => {
const testIndexName = 'test_index_' + cuid();
const testIndexName = `test_index_${cuid()}`;
let indexedObject;

before(() => {
Expand Down Expand Up @@ -42,7 +42,7 @@ describe('/indexes/{name}/{objectID}', () => {
});

it('returns a 404 when the index does not contain the identified object', () => {
return specRequest({url: '/1/indexes/' + testIndexName + '/12345'})
return specRequest({url: `/1/indexes/${testIndexName}/12345`})
.then(response => {
expect(response.statusCode).to.equal(404);
expect(response.result.message).to.equal('Index does not contain object with identifier 12345');
Expand Down
14 changes: 9 additions & 5 deletions spec/spec_request.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@ const enjoi = require('enjoi');
const swagger = require('../swagger.json');

module.exports = function (options) {
return new Promise(function (resolve, reject) {
require('../lib/server')(function (err, server) {
return new Promise((resolve, reject) => {
require('../lib/server')((err, server) => {
if (err) {
return reject(err);
}

server.inject(options, function (response) {
server.inject(options, response => {
if (!response.request.route || response.request.route.path === '/{p*}') {
return reject(new Error(`Undefined route ${options.url}`));
}

const apiBasePath = swagger.basePath || '';
const route = response.request.route.path.replace(new RegExp('^' + apiBasePath), '');
const route = response.request.route.path.replace(new RegExp(`^${apiBasePath}`), '');
const method = response.request.method;

const swaggerPath = swagger.paths[route];
Expand All @@ -33,13 +33,17 @@ module.exports = function (options) {

const swaggerResponse = swaggerMethod.responses[response.statusCode];

if (response.statusCode === 400) {
return resolve(response);
}

if (response.statusCode !== 404 && !swaggerResponse) {
return reject(new Error(`${response.statusCode} result for ${method} of route ${route} is undocumented. Please add to swagger.json.`));
}

const validator = enjoi(swaggerResponse.schema, {subSchemas: {'#': swagger}});

validator.validate(response.payload, function (err) {
validator.validate(response.payload, err => {
if (err) {
return reject(new Error(`Response does not match the documented schema: ${err}`));
}
Expand Down
34 changes: 34 additions & 0 deletions swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,40 @@
"query": {
"type": "string",
"description": "Search keywords"
},
"filters": {
"type": "array",
"description": "An array of search filter objects. Either 'term' or 'range' is required",
"items": {
"type": "object",
"minProperties": 2,
"required": ["field"],
"properties": {
"field": {
"type": "string",
"description": "The field to filter by"
},
"term": {
"type": "string",
"description": "The filter term"
},
"range": {
"type": "object",
"description": "The filter range",
"minProperties": 1,
"properties": {
"from": {
"type": "number",
"description": "The range's lower bound"
},
"to": {
"type": "number",
"description": "The range's upper bound"
}
}
}
}
}
}
}
}
Expand Down
Loading