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.

Sign up for GitHub

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

Jump to bottom

allows more flexible timelion queries #10961

Merged
merged 10 commits into from
Apr 28, 2017
Merged

allows more flexible timelion queries #10961

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
840f50d
allows more flexible timelion queries
ppisljar Mar 30, 2017
a7bdaef
removing semi colon, adding support for new lines
ppisljar Apr 6, 2017
720ee94
Allow for newlines
Apr 6, 2017
7f0d92b
adding tests
ppisljar Apr 7, 2017
7193932
updating tests
ppisljar Apr 10, 2017
cd6b457
using bluebird Promise
ppisljar Apr 10, 2017
6810136
Series now returns an array, don't need this +
Apr 12, 2017
231b6d4
fixing tests to only test parseSheet
ppisljar Apr 24, 2017
fee2d6b
adding test for whitespace chars (newline and return)
ppisljar Apr 25, 2017
068d4bd
improving readability
ppisljar Apr 28, 2017
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
View file
Open in desktop
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
23 changes: 23 additions & 0 deletions src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
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('does not allow whitespace used between two expressions', async function () {
const data = ['.es() .es(404)'];
const ast = parseSheet(data);

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

it('allows comma-delimited expressions', function () {
const data = ['.es(), .es(404)'];
const ast = parseSheet(data);

expect(ast[0].length).to.equal(2);
expect(ast[0][0].type).to.equal('chain');
expect(ast[0][1].type).to.equal('chain');
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a little confused by the use of "allow" and "does not allow" in the test descriptions, because I assumed we'd be throwing errors or somehow preventing disallowed input. After I dug into the tests, I realized this was an incorrect assumption. I think it will be a little clearer if we describe the behavior as either "splitting expressions" or "not splitting expressions". Thoughts?

Because I'm not familiar with the structure of the generated AST, I also found it useful to break out the part of the object that represents the list of expressions into a variable, to increase readability.

const parseSheet = require('../lib/parse_sheet');

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

describe('timelion parse_sheet function', function () {
  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');
  });
});

24 changes: 23 additions & 1 deletion src/core_plugins/timelion/server/series_functions/__tests__/fixtures/tlConfig.js
View file
Open in desktop
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