Refactor timestamp existence validation in V2 API

There was a bug in AlarmThresholdRule in v2.py, the timestamp_keys
were defined with a missing comma:
['timestamp', 'start', 'start_timestamp' 'end', 'end_timestamp'].

The validation, if the timestamp field is used or not, was unclear
in the current implementation. A new parameter, 'timestamp_is_valid',
was added to the validation functions to show, if the timestamp is
allowed to use or not in the validated query.

Fixes bug 1280975

(cherry picked from commit b1834e55fce08f1536c7d9392ae035b9583be8ed)

Change-Id: I2aaef54354926f9afe3bb1d4fae8f8aa0ae600ab

3 files changed, 121 insertions, 26 deletions

diff --git a/ceilometer/api/controllers/v2.py b/ceilometer/api/controllers/v2.py
index f4877ef8..90faf96f 100644
--- a/ceilometer/api/controllers/v2.py
+++ b/ceilometer/api/controllers/v2.py
@@ -329,7 +329,8 @@ def _verify_query_segregation(query, auth_project=None):
                 raise ProjectNotAuthorized(q.value)
 
 
-def _validate_query(query, db_func, internal_keys=[]):
+def _validate_query(query, db_func, internal_keys=[],
+                    is_timestamp_valid=True):
     _verify_query_segregation(query)
 
     valid_keys = inspect.getargspec(db_func)[0]
@@ -338,23 +339,28 @@ def _validate_query(query, db_func, internal_keys=[]):
     translation = {'user_id': 'user',
                    'project_id': 'project',
                    'resource_id': 'resource'}
-    has_timestamp = False
+
+    has_timestamp_query = _validate_timestamp_fields(query,
+                                                     'timestamp',
+                                                     ('lt', 'le', 'gt', 'ge'),
+                                                     is_timestamp_valid)
+    has_search_offset_query = _validate_timestamp_fields(query,
+                                                         'search_offset',
+                                                         ('eq'),
+                                                         is_timestamp_valid)
+
+    if has_search_offset_query and not has_timestamp_query:
+        raise wsme.exc.InvalidInput('field', 'search_offset',
+                                    "search_offset cannot be used without " +
+                                    "timestamp")
+
     for i in query:
-        if i.field == 'timestamp':
-            has_timestamp = True
-            if i.op not in ('lt', 'le', 'gt', 'ge'):
-                raise wsme.exc.InvalidInput('op', i.op,
-                                            'unimplemented operator for %s' %
-                                            i.field)
-        else:
+        if i.field not in ('timestamp', 'search_offset'):
             if i.op == 'eq':
-                if i.field == 'search_offset':
-                    has_timestamp = True
-                elif i.field == 'enabled':
+                if i.field == 'enabled':
                     i._get_value_as_type('boolean')
-                elif i.field.startswith('metadata.'):
-                    i._get_value_as_type()
-                elif i.field.startswith('resource_metadata.'):
+                elif (i.field.startswith('metadata.') or
+                      i.field.startswith('resource_metadata.')):
                     i._get_value_as_type()
                 else:
                     key = translation.get(i.field, i.field)
@@ -367,14 +373,30 @@ def _validate_query(query, db_func, internal_keys=[]):
                                             'unimplemented operator for %s' %
                                             i.field)
 
-    if has_timestamp and not ('start' in valid_keys or
-                              'start_timestamp' in valid_keys):
-        raise wsme.exc.UnknownArgument('timestamp',
-                                       "not valid for this resource")
+
+def _validate_timestamp_fields(query, field_name, operator_list,
+                               is_timestamp_valid):
+    for item in query:
+        if item.field == field_name:
+            #If *timestamp* or *search_offset* field was specified in the
+            #query, but timestamp is not supported on that resource, on
+            #which the query was invoked, then raise an exception.
+            if not is_timestamp_valid:
+                raise wsme.exc.UnknownArgument(field_name,
+                                               "not valid for " +
+                                               "this resource")
+            if item.op not in operator_list:
+                raise wsme.exc.InvalidInput('op', item.op,
+                                            'unimplemented operator for %s' %
+                                            item.field)
+            return True
+    return False
 
 
-def _query_to_kwargs(query, db_func, internal_keys=[]):
-    _validate_query(query, db_func, internal_keys=internal_keys)
+def _query_to_kwargs(query, db_func, internal_keys=[],
+                     is_timestamp_valid=True):
+    _validate_query(query, db_func, internal_keys=internal_keys,
+                    is_timestamp_valid=is_timestamp_valid)
     query = _sanitize_query(query, db_func)
     internal_keys.append('self')
     valid_keys = set(inspect.getargspec(db_func)[0]) - set(internal_keys)
@@ -875,7 +897,9 @@ class MetersController(rest.RestController):
 
         :param q: Filter rules for the meters to be returned.
         """
-        kwargs = _query_to_kwargs(q, pecan.request.storage_conn.get_meters)
+        #Timestamp field is not supported for Meter queries
+        kwargs = _query_to_kwargs(q, pecan.request.storage_conn.get_meters,
+                                  is_timestamp_valid=False)
         return [Meter.from_db_model(m)
                 for m in pecan.request.storage_conn.get_meters(**kwargs)]
 
@@ -1018,10 +1042,12 @@ class AlarmThresholdRule(_Base):
         if not threshold_rule.query:
             threshold_rule.query = []
 
-        timestamp_keys = ['timestamp', 'start', 'start_timestamp' 'end',
-                          'end_timestamp']
+        #Timestamp is not allowed for AlarmThresholdRule query, as the alarm
+        #evaluator will construct timestamp bounds for the sequence of
+        #statistics queries as the sliding evaluation window advances
+        #over time.
         _validate_query(threshold_rule.query, storage.SampleFilter.__init__,
-                        internal_keys=timestamp_keys)
+                        is_timestamp_valid=False)
         return threshold_rule
 
     @property
@@ -1496,8 +1522,10 @@ class AlarmsController(rest.RestController):
 
         :param q: Filter rules for the alarms to be returned.
         """
+        #Timestamp is not supported field for Simple Alarm queries
         kwargs = _query_to_kwargs(q,
-                                  pecan.request.storage_conn.get_alarms)
+                                  pecan.request.storage_conn.get_alarms,
+                                  is_timestamp_valid=False)
         return [Alarm.from_db_model(m)
                 for m in pecan.request.storage_conn.get_alarms(**kwargs)]
 
diff --git a/tests/api/v2/test_alarm_scenarios.py b/tests/api/v2/test_alarm_scenarios.py
index d006c54c..1c94edbe 100644
--- a/tests/api/v2/test_alarm_scenarios.py
+++ b/tests/api/v2/test_alarm_scenarios.py
@@ -167,6 +167,20 @@ class TestAlarms(FunctionalTest,
                              for r in data if 'combination_rule' in r),
                          set(['or']))
 
+    def test_alarms_query_with_timestamp(self):
+        date_time = datetime.datetime(2012, 7, 2, 10, 41)
+        isotime = date_time.isoformat()
+        resp = self.get_json('/alarms',
+                             q=[{'field': 'timestamp',
+                                 'op': 'gt',
+                                 'value': isotime}],
+                             expect_errors=True)
+        self.assertEqual(resp.status_code, 400)
+        self.assertEqual(jsonutils.loads(resp.body)['error_message']
+                         ['faultstring'],
+                         'Unknown argument: "timestamp": '
+                         'not valid for this resource')
+
     def test_get_not_existing_alarm(self):
         resp = self.get_json('/alarms/alarm-id-3', expect_errors=True)
         self.assertEqual(resp.status_code, 404)
@@ -360,6 +374,31 @@ class TestAlarms(FunctionalTest,
             'threshold_rule and combination_rule cannot '
             'be set at the same time')
 
+    def test_post_invalid_alarm_timestamp_in_threshold_rule(self):
+        date_time = datetime.datetime(2012, 7, 2, 10, 41)
+        isotime = date_time.isoformat()
+
+        json = {
+            'name': 'invalid_alarm',
+            'type': 'threshold',
+            'threshold_rule': {
+                'meter_name': 'ameter',
+                'query': [{'field': 'timestamp',
+                           'op': 'gt',
+                           'value': isotime}],
+                'comparison_operator': 'gt',
+                'threshold': 2.0,
+            }
+        }
+        resp = self.post_json('/alarms', params=json, expect_errors=True,
+                              status=400, headers=self.auth_headers)
+        alarms = list(self.conn.get_alarms())
+        self.assertEqual(4, len(alarms))
+        self.assertEqual(
+            'Unknown argument: "timestamp": '
+            'not valid for this resource',
+            resp.json['error_message']['faultstring'])
+
     def test_post_alarm_defaults(self):
         to_check = {
             'enabled': True,
diff --git a/tests/api/v2/test_list_meters_scenarios.py b/tests/api/v2/test_list_meters_scenarios.py
index fe2c5b78..0fbe77a2 100644
--- a/tests/api/v2/test_list_meters_scenarios.py
+++ b/tests/api/v2/test_list_meters_scenarios.py
@@ -21,6 +21,7 @@
 
 import base64
 import datetime
+import json as jsonutils
 import logging
 import testscenarios
 
@@ -150,6 +151,33 @@ class TestListMeters(FunctionalTest,
         self.assertEqual(set(r['source'] for r in data),
                          set(['test_source', 'test_source1']))
 
+    def test_list_meters_query_with_timestamp(self):
+        date_time = datetime.datetime(2012, 7, 2, 10, 41)
+        isotime = date_time.isoformat()
+        resp = self.get_json('/meters',
+                             q=[{'field': 'timestamp',
+                                 'op': 'gt',
+                                 'value': isotime}],
+                             expect_errors=True)
+        self.assertEqual(resp.status_code, 400)
+        self.assertEqual(jsonutils.loads(resp.body)['error_message']
+                         ['faultstring'],
+                         'Unknown argument: "timestamp": '
+                         'not valid for this resource')
+
+    def test_query_samples_with_search_offset(self):
+        resp = self.get_json('/meters/meter.mine',
+                             q=[{'field': 'search_offset',
+                                 'op': 'eq',
+                                 'value': 42}],
+                             expect_errors=True)
+        self.assertEqual(resp.status_code, 400)
+        self.assertEqual(jsonutils.loads(resp.body)['error_message']
+                         ['faultstring'],
+                         "Invalid input for field/attribute field. "
+                         "Value: 'search_offset'. "
+                         "search_offset cannot be used without timestamp")
+
     def test_list_meters_with_dict_metadata(self):
         data = self.get_json('/meters/meter.mine',
                              q=[{'field':