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

allows more flexible timelion queries #10961

Merged
merged 10 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions src/core_plugins/timelion/public/chain.peg
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
}

start
= tree:series+ {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why this is removed? Don't entirely see the relation with the other changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

series now return an array see here

= tree:series {
return {
tree: tree.filter(function (o) {return o != null}),
functions: functions,
Expand All @@ -31,15 +31,12 @@ start
}

arg_list
= first:argument rest:more_args* {
= first:argument rest:(space? ',' space? arg:argument {return arg})* space? ','? {
return [first].concat(rest);
}

more_args
= ','? space? arg:argument {return arg;}

argument
= name:function_name '=' value:arg_type {
= name:function_name space? '=' space? value:arg_type {
return {
type: 'namedArg',
name: name,
Expand Down Expand Up @@ -82,13 +79,15 @@ series_type
/ reference

series
= series:series_type ','? space? { return series }
= first:series_type rest:(space? ',' space? series:series_type {return series})* ','? {
return [first].concat(rest)
}

function_name
= (first:[a-zA-Z]+ rest:[.a-zA-Z0-9_-]* ) { return first.join('') + rest.join('') }
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this change makes this compatible with PegJS 0.10.0

= first:[a-zA-Z]+ rest:[.a-zA-Z0-9_-]* { return first.join('') + rest.join('') }

function "function"
= '.' name:function_name '(' space? arg_list:arg_list? space? ')' {
= space? '.' name:function_name space? '(' space? arg_list:arg_list? space? ')' {
var result = {
type: 'function',
function: name,
Expand Down Expand Up @@ -132,10 +131,10 @@ reference


chain
= func:function rest:function* {return {type: 'chain', chain: [func].concat(rest)}}
= func:function space? rest:function* {return {type: 'chain', chain: [func].concat(rest)}}

group
= '(' space? grouped:series+ space? ')' functions:function* {
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

Copy link
Member Author

Choose a reason for hiding this comment

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

dito :)

= '(' space? grouped:series space? ')' functions:function* {
var first = {
type: 'chainList',
list: grouped
Expand All @@ -162,7 +161,7 @@ literal "literal"
}

space
= [\ \t]+
= [\ \t\r\n]+

dq_char
= "\\" sequence:('"' / "\\") { return sequence; }
Expand Down
34 changes: 34 additions & 0 deletions src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
const parseSheet = require('../lib/parse_sheet');

const expect = require('chai').expect;

describe('timelion parse_sheet function', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add tests for the change in white-space behavior too (not just for the new comma-separated behavior).

Copy link
Member Author

Choose a reason for hiding this comment

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

we do test for the white space behavior in the test below (it should not be allowed)

or do you mean testing that \n and \r are also accepted as white space ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, test that \n and \r are now accepted.

but not sure if we already have tests in place. if not, this could be a separate effort.

it(`doesn't split expressions on whitespace`, async function () {
const data = ['.es() .es(404)'];
const ast = parseSheet(data);

const expressions = ast[0];
expect(expressions.length).to.equal(1);
expect(expressions[0].type).to.equal('chain');
});

it('splits expressions on commas', function () {
const data = ['.es(), .es(404)'];
const ast = parseSheet(data);

const expressions = ast[0];
expect(expressions.length).to.equal(2);
expect(expressions[0].type).to.equal('chain');
expect(expressions[1].type).to.equal('chain');
});

it('splits expressions on newlines', function () {
const data = [`.es()\n\r ,\n\r .es(404)`];
const ast = parseSheet(data);

const expressions = ast[0];
expect(expressions.length).to.equal(2);
expect(expressions[0].type).to.equal('chain');
expect(expressions[1].type).to.equal('chain');
});
});
Original file line number Diff line number Diff line change
@@ -1,9 +1,31 @@
import moment from 'moment';
import sinon from 'sinon';
import timelionDefaults from '../../../lib/get_namespaced_settings';
import esResponse from './es_response';

module.exports = function () {

const functions = require('../../../lib/load_functions')('series_functions');
const server = {
plugins: {
timelion: {
getFunction: (name) => {
if (!functions[name]) throw new Error ('No such function: ' + name);
return functions[name];
}
},
elasticsearch: {
getCluster: sinon.stub().withArgs('data').returns({
callWithRequest: function () {
return Promise.resolve(esResponse);
}
})
}
}
};

const tlConfig = require('../../../handlers/lib/tl_config.js')({
server: {},
server: server,
request: {}
});

Expand Down