Repository: google/timesketch

Issue:
Inconsistencies in the Aggregation section of the Timesketch API
Some inconsistencies in the TS Python API when interacting with aggregation. This was something that I found when calling [list_aggregations](https://github.com/google/timesketch/blob/1cef69b041c488878f062ce9e515313aa05f96cd/api_client/python/timesketch_api_client/sketch.py#L545), and was confused as to why the `.name` attribute of the returning objects was not the one returned by the API (`"sketches/{self.id}/aggregation/"`). It seems the `name` and `description` of the Aggregation object are extracted from `resource_data`, which is populated by [from_saved](https://github.com/google/timesketch/blob/1cef69b041c488878f062ce9e515313aa05f96cd/api_client/python/timesketch_api_client/aggregation.py#L136).

So this goes like this:

1. list_aggregations, query to `sketches/{self.id}/aggregation`
2. At this point, I'd expect the funciton to return these results, but it goes on
3. `from_saved` is called for each aggregation (which I expected loaded pre-cached results from a database), calling another API endpoint, presumably to get fresh aggreagtor data. This is the data containing the right name and description for the aggregation.
4. `from_saved` calls `_run_aggregator` (which runs the aggregation once more?) and stores the result in the resource_data attribute of the `self` object. (maybe erroneously, instead of doing this with the results of the previous API call?)
5. When `list_aggregations` returns, the objects it returns are a list of results of _run_aggregator, which don't contain the expected data.

Discussion:

Patch:
diff --git a/api_client/python/timesketch_api_client/aggregation.py b/api_client/python/timesketch_api_client/aggregation.py
index 4825b799..a64d6e7b 100644
--- a/api_client/python/timesketch_api_client/aggregation.py
+++ b/api_client/python/timesketch_api_client/aggregation.py
@@ -43,10 +43,11 @@ class Aggregation(resource.SketchResource):
 
     def __init__(self, sketch):
         self._created_at = ""
+        self._name = ""
         self._parameters = {}
         self._updated_at = ""
         self._group_id = None
-        self.aggregator_name = ""
+        self._aggregator_name = ""
         self.chart_color = ""
         self.chart_type = ""
         self.chart_title = ""
@@ -113,7 +114,7 @@ class Aggregation(resource.SketchResource):
         if search_id:
             self.search_id = search_id
 
-        self.aggregator_name = aggregator_name
+        self._aggregator_name = aggregator_name
         self.chart_color = parameters.get("chart_color", "")
         self._parameters = parameters
 
@@ -149,9 +150,10 @@ class Aggregation(resource.SketchResource):
         if not data:
             return
 
-        self.aggregator_name = data.get("agg_type")
+        self._aggregator_name = data.get("agg_type")
         self.type = "stored"
 
+        self._name = data.get("name", "")
         self._created_at = data.get("created_at", "")
         self._updated_at = data.get("updated_at", "")
         self._group_id = data.get("aggregationgroup_id")
@@ -173,7 +175,7 @@ class Aggregation(resource.SketchResource):
 
         self._username = data.get("user", {}).get("username", "System")
         self.resource_data = self._run_aggregator(
-            aggregator_name=self.aggregator_name,
+            aggregator_name=self._aggregator_name,
             parameters=parameters,
             chart_type=chart_type,
         )
@@ -190,7 +192,7 @@ class Aggregation(resource.SketchResource):
             self.api.api_root, self._sketch.id
         )
 
-        self.aggregator_name = "DSL"
+        self._aggregator_name = "DSL"
         self._username = getpass.getuser()
         self.type = "DSL"
 
@@ -274,6 +276,8 @@ class Aggregation(resource.SketchResource):
             return self.chart_title
 
         data = self.lazyload_data()
+        if not data:
+            return ""
         meta = data.get("meta", {})
         self.chart_title = meta.get("vega_chart_title", "")
 
@@ -292,7 +296,9 @@ class Aggregation(resource.SketchResource):
     @property
     def description(self):
         """Property that returns the description string."""
-        data = self.resource_data
+        data = self.lazyload_data()
+        if not data:
+            return ""
         meta = data.get("meta", {})
         return meta.get("description", "")
 
@@ -307,12 +313,7 @@ class Aggregation(resource.SketchResource):
     @property
     def name(self):
         """Property that returns the name of the aggregation."""
-        data = self.resource_data
-        meta = data.get("meta", {})
-        name = meta.get("name")
-        if name:
-            return name
-        return self.aggregator_name
+        return self._name
 
     @name.setter
     def name(self, name):
@@ -322,6 +323,18 @@ class Aggregation(resource.SketchResource):
         meta = self.resource_data.get("meta")
         meta["name"] = name
 
+    @property
+    def aggregator_name(self):
+        """Property that returns the aggregator name."""
+        if self._aggregator_name:
+            return self._aggregator_name
+
+        data = self.resource_data
+        meta = data.get("meta", {})
+        self._aggregator_name = meta.get("name")
+
+        return self._aggregator_name
+
     def add_label(self, label):
         """Add a label to the aggregation.
 
@@ -381,7 +394,7 @@ class Aggregation(resource.SketchResource):
         data = {
             "name": self.name,
             "description": self.description,
-            "agg_type": self.aggregator_name,
+            "agg_type": self._aggregator_name,
             "parameters": self._parameters,
             "chart_type": self.chart_type,
         }
@@ -612,6 +625,8 @@ class AggregationGroup(resource.SketchResource):
             return altair.Chart()
 
         data = self.lazyload_data()
+        if not data:
+            return None
 
         meta = data.get("meta", {})
         vega_spec = meta.get("vega_spec")


Test patch:
diff --git a/api_client/python/timesketch_api_client/aggregation_test.py b/api_client/python/timesketch_api_client/aggregation_test.py
new file mode 100644
index 00000000..61f3d9c7
--- /dev/null
+++ b/api_client/python/timesketch_api_client/aggregation_test.py
@@ -0,0 +1,184 @@
+# Copyright 2023 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Tests for the Timesketch API aggregation object."""
+import unittest
+import mock
+
+import altair as alt
+
+from . import aggregation
+from . import client
+from . import test_lib
+
+
+class AggregationTest(unittest.TestCase):
+    """Test Graph object."""
+
+    @mock.patch("requests.Session", test_lib.mock_session)
+    def setUp(self):
+        """Setup test case."""
+        self.api_client = client.TimesketchApi("http://127.0.0.1", "test", "test")
+        self.sketch = self.api_client.get_sketch(1)
+
+    # pylint: disable=protected-access
+    def test_init(self):
+        """Tests the Aggregation init method."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+
+        # Aggregation attributes
+        self.assertEqual(aggregation_obj._created_at, "")
+        self.assertEqual(aggregation_obj._group_id, None)
+        self.assertEqual(aggregation_obj._parameters, {})
+        self.assertEqual(aggregation_obj._updated_at, "")
+        self.assertEqual(aggregation_obj._aggregator_name, "")
+        self.assertEqual(aggregation_obj.chart_color, "")
+        self.assertEqual(aggregation_obj.chart_title, "")
+        self.assertEqual(aggregation_obj.search_id, None)
+        self.assertEqual(aggregation_obj.type, None)
+
+        # SketchResource attributes
+        self.assertEqual(aggregation_obj._labels, [])
+        self.assertEqual(aggregation_obj._resource_id, 0)
+        self.assertEqual(aggregation_obj._sketch, self.sketch)
+        self.assertEqual(aggregation_obj._username, "")
+
+        # BaseResource attributes
+        self.assertEqual(aggregation_obj.api, self.api_client)
+        self.assertEqual(
+            aggregation_obj.resource_uri, "sketches/1/aggregation/explore/"
+        )
+        self.assertEqual(aggregation_obj.resource_data, None)
+
+    def test_created_at(self):
+        """Tests the created_at property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.created_at, "")
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(aggregation_obj.created_at, "2023-01-08T08:45:23.113454")
+
+    def test_parameters(self):
+        """Tests the parameters property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.parameters, {})
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(
+            aggregation_obj.parameters,
+            {
+                "supported_charts": "barchart",
+                "field": "ip",
+                "start_time": "",
+                "end_time": "",
+                "limit": "10",
+                "index": [1, 2],
+            },
+        )
+
+    def test_is_part_of_group(self):
+        """Tests the is_part_of_group property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.is_part_of_group, False)
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(aggregation_obj.is_part_of_group, False)
+
+    def test_title(self):
+        """Tests the title property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.title, "")
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(aggregation_obj.title, "Top results for an unknown field")
+
+    def test_title_setter(self):
+        """Tests the title setter."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        aggregation_obj.title = "new title"
+        self.assertEqual(aggregation_obj.title, "new title")
+
+    def test_chart(self):
+        """Tests the chart property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        with self.assertRaises(TypeError) as err:
+            _ = aggregation_obj.chart
+        self.assertEqual(
+            str(err.exception), "Unable to generate chart, missing a chart type."
+        )
+
+        aggregation_obj.from_saved(1)
+        chart = aggregation_obj.chart
+        self.assertIsInstance(chart, alt.Chart)
+
+    def test_description(self):
+        """Tests the description property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.description, "")
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(
+            aggregation_obj.description, "Aggregating values of a particular field"
+        )
+
+    def test_description_setter(self):
+        """Tests the description property setter."""
+
+    def test_name(self):
+        """Tests the name property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.name, "")
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(aggregation_obj.name, "ip barchart")
+
+    def test_aggregator_name(self):
+        """Tests the aggregator_name property."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        self.assertEqual(aggregation_obj.name, "")
+
+        aggregation_obj.from_saved(1)
+        self.assertEqual(aggregation_obj.name, "ip barchart")
+
+    def test_add_label(self):
+        """Test the add_label function."""
+
+    def test_from_saved(self):
+        """Tests the from_saved method."""
+        aggregation_obj = aggregation.Aggregation(self.sketch)
+        aggregation_obj.from_saved(2)
+        self.assertEqual(aggregation_obj._aggregator_name, "field_bucket")
+        self.assertEqual(aggregation_obj.chart_color, "")
+        self.assertEqual(aggregation_obj.chart_type, "table")
+        self.assertEqual(aggregation_obj.chart_title, "")
+        self.assertEqual(aggregation_obj.search_id, None)
+        self.assertEqual(aggregation_obj.type, "stored")
+
+        self.assertEqual(aggregation_obj.created_at, "2023-01-08T08:46:24.871292")
+        self.assertEqual(
+            aggregation_obj.parameters,
+            {
+                "supported_charts": "table",
+                "field": "domain",
+                "start_time": "",
+                "end_time": "",
+                "limit": "10",
+                "index": [1, 2],
+            },
+        )
+        self.assertEqual(aggregation_obj.is_part_of_group, False)
+        self.assertEqual(aggregation_obj.title, "Top results for an unknown field")
+        self.assertEqual(
+            aggregation_obj.description, "Aggregating values of a particular field"
+        )
+        self.assertEqual(aggregation_obj.name, "domain table")
diff --git a/api_client/python/timesketch_api_client/sketch_test.py b/api_client/python/timesketch_api_client/sketch_test.py
index 09e00271..ef41f149 100644
--- a/api_client/python/timesketch_api_client/sketch_test.py
+++ b/api_client/python/timesketch_api_client/sketch_test.py
@@ -82,3 +82,16 @@ class SketchTest(unittest.TestCase):
         events = {"_id": "1", "_type": "_doc", "index": "1", "attributes": []}
         with self.assertRaisesRegex(ValueError, "Events need to be a list."):
             self.sketch.add_event_attributes(events)
+
+    def test_list_aggregations(self):
+        """Test the Sketch list_aggregations method."""
+        aggregations = self.sketch.list_aggregations()
+        self.assertEqual(len(aggregations), 2)
+        self.assertEqual(aggregations[0].name, "ip barchart")
+        self.assertEqual(
+            aggregations[0].description, "Aggregating values of a particular field"
+        )
+        self.assertEqual(aggregations[1].name, "domain table")
+        self.assertEqual(
+            aggregations[1].description, "Aggregating values of a particular field"
+        )
diff --git a/api_client/python/timesketch_api_client/test_lib.py b/api_client/python/timesketch_api_client/test_lib.py
index be9dd48a..6cbd2c82 100644
--- a/api_client/python/timesketch_api_client/test_lib.py
+++ b/api_client/python/timesketch_api_client/test_lib.py
@@ -458,6 +458,188 @@ def mock_response(*args, **kwargs):
         ],
     }
 
+    aggregation_data = {
+        "meta": {},
+        "objects": [
+            [
+                {
+                    "agg_type": "field_bucket",
+                    "aggregationgroup_id": 0,
+                    "chart_type": "barchart",
+                    "created_at": "2023-01-08T08:45:23.113454",
+                    "description": "Aggregating values of a particular field",
+                    "id": 1,
+                    "label_string": "",
+                    "name": "ip barchart",
+                    "parameters": (
+                        '{"supported_charts": "barchart", '
+                        '"field": "ip", "start_time": "", "end_time": "", '
+                        '"limit": "10", "index": [1, 2]}'
+                    ),
+                    "updated_at": "2023-01-08T08:45:23.113454",
+                    "user": {
+                        "active": True,
+                        "admin": False,
+                        "groups": [],
+                        "username": "dev",
+                    },
+                },
+                {
+                    "agg_type": "field_bucket",
+                    "aggregationgroup_id": 0,
+                    "chart_type": "table",
+                    "created_at": "2023-01-08T08:46:24.871292",
+                    "description": "Aggregating values of a particular field",
+                    "id": 2,
+                    "label_string": "",
+                    "name": "domain table",
+                    "parameters": (
+                        '{"supported_charts": "table", "field": "domain", '
+                        '"start_time": "", "end_time": "", "limit": "10", '
+                        '"index": [1, 2]}'
+                    ),
+                    "updated_at": "2023-01-08T08:46:24.871292",
+                    "user": {
+                        "active": True,
+                        "admin": False,
+                        "groups": [],
+                        "username": "dev",
+                    },
+                },
+            ]
+        ],
+    }
+
+    aggregation_1_data = {
+        "meta": {},
+        "objects": [
+            {
+                "agg_type": "field_bucket",
+                "aggregationgroup_id": 0,
+                "chart_type": "barchart",
+                "created_at": "2023-01-08T08:45:23.113454",
+                "description": "Aggregating values of a particular field",
+                "id": 1,
+                "label_string": "",
+                "name": "ip barchart",
+                "parameters": (
+                    '{"supported_charts": "barchart", "field": "ip", '
+                    '"start_time": "", "end_time": "", "limit": "10", '
+                    '"index": [1, 2]}'
+                ),
+                "updated_at": "2023-01-08T08:45:23.113454",
+                "user": {
+                    "active": True,
+                    "admin": False,
+                    "groups": [],
+                    "username": "dev",
+                },
+            }
+        ],
+    }
+
+    aggregation_2_data = {
+        "meta": {},
+        "objects": [
+            {
+                "agg_type": "field_bucket",
+                "aggregationgroup_id": 0,
+                "chart_type": "table",
+                "created_at": "2023-01-08T08:46:24.871292",
+                "description": "Aggregating values of a particular field",
+                "id": 2,
+                "label_string": "",
+                "name": "domain table",
+                "parameters": (
+                    '{"supported_charts": "table", "field": "domain", '
+                    '"start_time": "", "end_time": "", "limit": "10", '
+                    '"index": [1, 2]}'
+                ),
+                "updated_at": "2023-01-08T08:46:24.871292",
+                "user": {
+                    "active": True,
+                    "admin": False,
+                    "groups": [],
+                    "username": "dev",
+                },
+            }
+        ],
+    }
+
+    aggregation_chart_data = {
+        "meta": {
+            "chart_type": "barchart",
+            "description": "Aggregating values of a particular field",
+            "es_time": 0.01930856704711914,
+            "method": "aggregator_run",
+            "name": "field_bucket",
+            "vega_chart_title": "Top results for an unknown field",
+            "vega_spec": {
+                "$schema": "https://vega.github.io/schema/vega-lite/v4.8.1.json",
+                "config": {"view": {"continuousHeight": 300, "continuousWidth": 400}},
+                "data": {"name": "data-4e004a0d2e426361c7096c1d456fe9f0"},
+                "datasets": {
+                    "data-4e004a0d2e426361c7096c1d456fe9f0": [
+                        {"count": 125, "ip": "1.1.1.1"},
+                        {"count": 108, "ip": "1.1.1.2"},
+                        {"count": 97, "ip": "1.1.1.3"},
+                        {"count": 95, "ip": "1.1.1.4"},
+                        {"count": 87, "ip": "1.1.1.5"},
+                        {"count": 84, "ip": "1.1.1.6"},
+                        {"count": 82, "ip": "1.1.1.7"},
+                        {"count": 82, "ip": "1.1.1.8"},
+                        {"count": 51, "ip": "1.1.1.9"},
+                        {"count": 51, "ip": "1.1.1.10"},
+                    ]
+                },
+                "encoding": {
+                    "href": {"field": "url", "type": "nominal"},
+                    "tooltip": [
+                        {"field": "ip", "type": "nominal"},
+                        {"field": "count", "type": "quantitative"},
+                    ],
+                    "x": {
+                        "field": "ip",
+                        "sort": {"field": "count", "op": "sum", "order": "descending"},
+                        "type": "nominal",
+                    },
+                    "y": {"field": "count", "type": "quantitative"},
+                },
+                "mark": {"strokeWidth": 0.3, "type": "bar"},
+                "title": "Top results for an unknown field",
+                "transform": [
+                    {
+                        "as": "url",
+                        "calculate": (
+                            "((('/sketch/1/explore?q=ip:\"' + datum.ip) + "
+                            "'\" ') + '')"
+                        ),
+                    }
+                ],
+            },
+        },
+        "objects": [
+            {
+                "field_bucket": {
+                    "buckets": [
+                        {"count": 125, "ip": "1.1.1.1"},
+                        {"count": 108, "ip": "1.1.1.2"},
+                        {"count": 97, "ip": "1.1.1.3"},
+                        {"count": 95, "ip": "1.1.1.4"},
+                        {"count": 87, "ip": "1.1.1.5"},
+                        {"count": 84, "ip": "1.1.1.6"},
+                        {"count": 82, "ip": "1.1.1.7"},
+                        {"count": 82, "ip": "1.1.1.8"},
+                        {"count": 51, "ip": "1.1.1.9"},
+                        {"count": 51, "ip": "1.1.1.10"},
+                    ]
+                }
+            }
+        ],
+    }
+
+    aggregation_group = {"meta": {"command": "list_groups"}, "objects": []}
+
     # Register API endpoints to the correct mock response data.
     url_router = {
         "http://127.0.0.1": MockResponse(text_data=auth_text_data),
@@ -513,6 +695,21 @@ def mock_response(*args, **kwargs):
         "http://127.0.0.1/api/v1/sigmarules/text/": MockResponse(
             json_data=sigmarule_text
         ),
+        "http://127.0.0.1/api/v1/sketches/1/aggregation/": MockResponse(
+            json_data=aggregation_data
+        ),
+        "http://127.0.0.1/api/v1/sketches/1/aggregation/1/": MockResponse(
+            json_data=aggregation_1_data
+        ),
+        "http://127.0.0.1/api/v1/sketches/1/aggregation/2/": MockResponse(
+            json_data=aggregation_2_data
+        ),
+        "http://127.0.0.1/api/v1/sketches/1/aggregation/group/": MockResponse(
+            json_data=aggregation_group
+        ),
+        "http://127.0.0.1/api/v1/sketches/1/aggregation/explore/": MockResponse(
+            json_data=aggregation_chart_data
+        ),
     }
 
     if kwargs.get("empty", False):