Volans has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/363750 )
Change subject: QueryBuilder: move query string to build() method
......................................................................
QueryBuilder: move query string to build() method
* Breaking change: the constructor of the QueryBuilder was changed to
not accept anymore a query string directly, but just the configuration
and the optional logger. The query string is now a required parameter
of the build() method.
This properly split configuration and parameters, allowing to easily
build() multiple queries with the same QueryBuilder instance.
Change-Id: I27e4576d44b0772ab9b843b5816a932fb352a962
---
M cumin/cli.py
M cumin/query.py
M cumin/tests/unit/test_query.py
3 files changed, 63 insertions(+), 77 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/operations/software/cumin
refs/changes/50/363750/1
diff --git a/cumin/cli.py b/cumin/cli.py
index 5bc038b..a318cbe 100644
--- a/cumin/cli.py
+++ b/cumin/cli.py
@@ -231,7 +231,7 @@
args -- ArgumentParser instance with parsed command line arguments
config -- a dictionary with the parsed configuration file
"""
- query = QueryBuilder(args.hosts, config, logger).build()
+ query = QueryBuilder(config, logger).build(args.hosts)
hosts = query.execute()
if not hosts:
diff --git a/cumin/query.py b/cumin/query.py
index bf2ff59..26a5e1d 100644
--- a/cumin/query.py
+++ b/cumin/query.py
@@ -35,23 +35,26 @@
Parse a given query string and converts it into a query object for the
configured backend
"""
- def __init__(self, query_string, config, logger=None):
+ def __init__(self, config, logger=None):
"""Query builder constructor.
Arguments:
- query_string -- the query string to be parsed and passed to the query
builder
config -- the configuration dictionary
logger -- an optional logging instance [optional, default: None]
"""
self.logger = logger or logging.getLogger(__name__)
- self.query_string = query_string.strip()
self.query = Query.new(config, logger=self.logger)
self.aliases = config.get(config['backend'], {}).get('aliases', {})
- self.level = 0 # Nesting level for sub-groups
+ self.level = None # Nesting level for sub-groups
- def build(self):
- """Parse the query string according to the grammar and build the query
object for the configured backend."""
- parsed = grammar.parseString(self.query_string, parseAll=True)
+ def build(self, query_string):
+ """Parse the query string according to the grammar and build the query
object for the configured backend.
+
+ Arguments:
+ query_string -- the query string to be parsed and passed to the query
builder
+ """
+ self.level = 0
+ parsed = grammar.parseString(query_string.strip(), parseAll=True)
for token in parsed:
self._parse_token(token)
@@ -64,9 +67,9 @@
token -- a single token returned by the grammar parsing
level -- nesting level in case of sub-groups in the query [optional,
default: 0]
"""
- if not isinstance(token, ParseResults):
- raise InvalidQueryError("Invalid query string syntax '{query}'.
Token is '{token}'".format(
- query=self.query_string, token=token))
+ if not isinstance(token, ParseResults): # Non-testable block, this
should never happen
+ raise InvalidQueryError('Expected an instance of
pyparsing.ParseResults, got {instance}: {token}'.format(
+ instance=type(token), token=token))
token_dict = token.asDict()
if not token_dict:
@@ -103,6 +106,8 @@
self.query.add_and()
elif token_dict['bool'] == 'or':
self.query.add_or()
+ else: # Non-testable block, this should never happen with the
current grammar
+ raise InvalidQueryError("Got bool '{bool}', one of and|or
expected".format(bool=token_dict['bool']))
elif 'hosts' in keys:
token_dict['hosts'] = NodeSet(token_dict['hosts'])
@@ -111,6 +116,10 @@
elif 'category' in keys:
self.query.add_category(**token_dict)
+ else: # Non-testable block, this should never happen with the current
grammar
+ raise InvalidQueryError(
+ "No valid key found in token, one of bool|hosts|category
expected: {token}".format(token=token_dict))
+
def _replace_alias(self, token_dict, level):
"""Replace any alias in the query in a recursive way, alias can
reference other aliases.
diff --git a/cumin/tests/unit/test_query.py b/cumin/tests/unit/test_query.py
index 7ea6a08..1a2d67e 100644
--- a/cumin/tests/unit/test_query.py
+++ b/cumin/tests/unit/test_query.py
@@ -69,115 +69,92 @@
}
@mock.patch('cumin.query.Query', QueryFactory)
+ def setup_method(self, _):
+ """Set method for each test, init a QueryBuilder."""
+ self.query_builder = QueryBuilder(self.config) # pylint:
disable=attribute-defined-outside-init
+
def test_instantiation(self):
"""Class QueryBuilder should create an instance of a query_class for
the given backend."""
- query_builder = QueryBuilder(self.query_string, self.config)
- assert isinstance(query_builder, QueryBuilder)
- assert isinstance(query_builder.query, BaseQuery)
- assert query_builder.query_string == self.query_string
- assert query_builder.level == 0
+ assert isinstance(self.query_builder, QueryBuilder)
+ assert isinstance(self.query_builder.query, BaseQuery)
+ assert self.query_builder.level is None
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_valid(self):
"""QueryBuilder.build() should parse and build the query object for a
valid query."""
- query_builder = QueryBuilder(self.query_string, self.config)
- query_builder.build()
+ self.query_builder.build(self.query_string)
- query_builder.query.add_hosts.assert_has_calls(
+ self.query_builder.query.add_hosts.assert_has_calls(
[mock.call(hosts=NodeSet('host1')),
mock.call(hosts=NodeSet('host2'))])
- query_builder.query.add_or.assert_has_calls([mock.call(), mock.call()])
- query_builder.query.open_subgroup.assert_called_once_with()
- query_builder.query.add_category.assert_has_calls([
+ self.query_builder.query.add_or.assert_has_calls([mock.call(),
mock.call()])
+ self.query_builder.query.open_subgroup.assert_called_once_with()
+ self.query_builder.query.add_category.assert_has_calls([
mock.call(category='F', key='key1', operator='=', value='value',
neg='not'),
mock.call(category='R', key='key2', operator='~', value='regex')])
- query_builder.query.add_and.assert_called_once_with()
- query_builder.query.close_subgroup.assert_called_once_with()
- assert query_builder.level == 0
+ self.query_builder.query.add_and.assert_called_once_with()
+ self.query_builder.query.close_subgroup.assert_called_once_with()
+ assert self.query_builder.level == 0
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_valid_with_aliases(self):
"""QueryBuilder.build() should replace any aliases and build the query
object for a valid query."""
- query_builder = QueryBuilder('host100 or A:group1 or host2',
self.config)
- query_builder.build()
+ self.query_builder.build('host100 or A:group1 or host2')
- query_builder.query.add_hosts.assert_has_calls(
+ self.query_builder.query.add_hosts.assert_has_calls(
[mock.call(hosts=NodeSet('host100')),
mock.call(hosts=NodeSet('host1')),
mock.call(hosts=NodeSet('host10[10-22]')),
mock.call(hosts=NodeSet('host2'))])
- query_builder.query.add_or.assert_has_calls([mock.call(), mock.call(),
mock.call()])
- query_builder.query.open_subgroup.assert_called_once_with()
- query_builder.query.close_subgroup.assert_called_once_with()
- assert query_builder.level == 0
+ self.query_builder.query.add_or.assert_has_calls([mock.call(),
mock.call(), mock.call()])
+ self.query_builder.query.open_subgroup.assert_called_once_with()
+ self.query_builder.query.close_subgroup.assert_called_once_with()
+ assert self.query_builder.level == 0
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_valid_with_nested_aliases(self): # pylint:
disable=invalid-name
"""QueryBuilder.build() should replace any aliases and build the query
object for a valid query."""
- query_builder = QueryBuilder('host100 or A:nested_group', self.config)
- query_builder.build()
+ self.query_builder.build('host100 or A:nested_group')
- query_builder.query.add_hosts.assert_has_calls(
+ self.query_builder.query.add_hosts.assert_has_calls(
[mock.call(hosts=NodeSet('host100')),
mock.call(hosts=NodeSet('host10[40-42]')),
mock.call(hosts=NodeSet('host1')),
mock.call(hosts=NodeSet('host10[10-22]'))])
- query_builder.query.add_or.assert_has_calls([mock.call(), mock.call()])
- query_builder.query.open_subgroup.assert_has_calls([mock.call(),
mock.call()])
- query_builder.query.close_subgroup.assert_has_calls([mock.call(),
mock.call()])
- assert query_builder.level == 0
+ self.query_builder.query.add_or.assert_has_calls([mock.call(),
mock.call()])
+ self.query_builder.query.open_subgroup.assert_has_calls([mock.call(),
mock.call()])
+ self.query_builder.query.close_subgroup.assert_has_calls([mock.call(),
mock.call()])
+ assert self.query_builder.level == 0
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_invalid_alias_syntax(self):
"""QueryBuilder.build() should raise InvalidQueryError if an alias has
invalid syntax."""
- query_builder = QueryBuilder('host1 or A:name = value', self.config)
with pytest.raises(InvalidQueryError, match='Invalid alias syntax,
aliases can be only of the form'):
- query_builder.build()
+ self.query_builder.build('host1 or A:name = value')
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_missing_alias(self):
"""QueryBuilder.build() should raise InvalidQueryError if a non
existent alias is found."""
- query_builder = QueryBuilder('host1 or A:non_existent_group',
self.config)
with pytest.raises(InvalidQueryError, match='Unable to find alias
replacement for'):
- query_builder.build()
+ self.query_builder.build('host1 or A:non_existent_group')
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_glob_host(self):
"""QueryBuilder.build() should parse a glob host."""
- query_builder = QueryBuilder('host1*', self.config)
- query_builder.build()
-
query_builder.query.add_hosts.assert_called_once_with(hosts=NodeSet('host1*'))
- assert query_builder.level == 0
+ self.query_builder.build('host1*')
+
self.query_builder.query.add_hosts.assert_called_once_with(hosts=NodeSet('host1*'))
+ assert self.query_builder.level == 0
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_invalid(self):
"""QueryBuilder.build() should raise ParseException for an invalid
query."""
- query_builder = QueryBuilder(self.invalid_query_string, self.config)
with pytest.raises(ParseException, match='Expected end of text'):
- query_builder.build()
+ self.query_builder.build(self.invalid_query_string)
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_subgroup(self):
"""QueryBuilder.build() should open and close a subgroup properly."""
- query_builder = QueryBuilder('(host1)', self.config)
- query_builder.build()
+ self.query_builder.build('(host1)')
-
query_builder.query.add_hosts.assert_has_calls([mock.call(hosts=NodeSet('host1'))])
- query_builder.query.open_subgroup.assert_called_once_with()
- query_builder.query.close_subgroup.assert_called_once_with()
- assert query_builder.level == 0
+
self.query_builder.query.add_hosts.assert_has_calls([mock.call(hosts=NodeSet('host1'))])
+ self.query_builder.query.open_subgroup.assert_called_once_with()
+ self.query_builder.query.close_subgroup.assert_called_once_with()
+ assert self.query_builder.level == 0
- @mock.patch('cumin.query.Query', QueryFactory)
def test_build_subgroups(self):
"""QueryBuilder.build() should open and close multiple subgroups
properly."""
- query_builder = QueryBuilder('(host1 or (host2 or host3))',
self.config)
- query_builder.build()
+ self.query_builder.build('(host1 or (host2 or host3))')
- query_builder.query.add_hosts.assert_has_calls(
+ self.query_builder.query.add_hosts.assert_has_calls(
[mock.call(hosts=NodeSet('host1')),
mock.call(hosts=NodeSet('host2')),
mock.call(hosts=NodeSet('host3'))])
- query_builder.query.open_subgroup.assert_has_calls([mock.call(),
mock.call()])
- query_builder.query.close_subgroup.assert_has_calls([mock.call(),
mock.call()])
- assert query_builder.level == 0
-
- @mock.patch('cumin.query.Query', QueryFactory)
- def test__parse_token(self):
- """QueryBuilder._parse_token() should raise RuntimeError for an
invalid token."""
- query_builder = QueryBuilder(self.invalid_query_string, self.config)
- with pytest.raises(InvalidQueryError, match='Invalid query string
syntax'):
- query_builder._parse_token('invalid_token') # pylint:
disable=protected-access
+ self.query_builder.query.open_subgroup.assert_has_calls([mock.call(),
mock.call()])
+ self.query_builder.query.close_subgroup.assert_has_calls([mock.call(),
mock.call()])
+ assert self.query_builder.level == 0
--
To view, visit https://gerrit.wikimedia.org/r/363750
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I27e4576d44b0772ab9b843b5816a932fb352a962
Gerrit-PatchSet: 1
Gerrit-Project: operations/software/cumin
Gerrit-Branch: master
Gerrit-Owner: Volans <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
