fix(records): validate aggregate and filter container shapes by andersfylling · Pull Request #2748 · cognitedata/cognite-sdk-python

andersfylling · 2026-08-06T23:03:39Z

aggregates is a mapping of the aggregate IDs you choose to aggregates, and a Filters aggregate takes a sequence of filters. Passing an aggregate directly (or a list of them) where a mapping is expected is silently serialized into a body the API cannot make sense of:

client.data_modeling.records.aggregate(Count(), stream_id=STREAM_ID)
# sends {"aggregates": {"count": {}}} - "count" read as a client-defined ID

UniqueValues(path, aggregates=[Count()])   # aggregates as a list, sent as-is

UniqueValues(path, aggregates={"buckets": {"uniqueValues": {"property": [...], "aggregates": [Count()]}}})
# a nested `aggregates` inside a raw dict aggregate value was shipped off unchecked

Both shapes are now validated where they are taken — the top-level aggregates argument, and the nested aggregates of every bucket aggregate, recursively through raw dict values too — with a TypeError that shows the expected shape.

Filters.filters is validated too, but since a single filter is common and unambiguous, it's normalized into a one-item list rather than rejected — matching how the rest of the SDK treats single-vs-sequence arguments:

Filters(filters=filters.MatchAll())   # now equivalent to Filters(filters=[filters.MatchAll()])

First commit is the failing tests, second is the fix.

🤖 Generated with Claude Code

The aggregates argument is a mapping of client-defined ID to aggregate, and a Filters aggregate takes a list. Passing an aggregate, or a list of them, builds a request body the API cannot make sense of, with nothing pointing at the shape as the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

An aggregates mapping must be keyed by the client-defined aggregate IDs, and a Filters aggregate takes a sequence of filters. Both now raise TypeError naming the expected shape - at the top-level aggregates argument and in every bucket aggregate that nests further aggregates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

codecov · 2026-08-07T13:31:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.04%. Comparing base (38cbb51) to head (316d750).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2748      +/-   ##
==========================================
- Coverage   93.06%   93.04%   -0.03%     
==========================================
  Files         514      514              
  Lines       52959    53057      +98     
==========================================
+ Hits        49286    49365      +79     
- Misses       3673     3692      +19

Files with missing lines	Coverage Δ
cognite/client/_api/data_modeling/records.py	`99.04% <100.00%> (ø)`
cognite/client/_sync_api/data_modeling/records.py	`100.00% <ø> (ø)`
...te/client/data_classes/data_modeling/aggregates.py	`99.64% <100.00%> (+0.03%)`	⬆️
...s_unit/test_api/test_data_modeling/test_records.py	`100.00% <100.00%> (ø)`
...t_data_classes/test_data_models/test_aggregates.py	`100.00% <100.00%> (ø)`

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gemini-code-assist

Code Review

This pull request introduces validation for the aggregates and filters arguments in the data modeling API and data classes. Specifically, it adds _validate_aggregates and _validate_filters helper functions to ensure that aggregates are passed as mappings and filters as sequences, raising appropriate TypeErrors with helpful hints otherwise. Unit tests have been added to verify these validations. There are no review comments, and I have no feedback to provide.

haakonvt

Great that you are adding user friendly validation! A few comments:

haakonvt · 2026-08-20T11:12:25Z

+
+
+def _validate_aggregates(
+    aggregates: Any, argument: str = "aggregates"


Suggested change

aggregates: Any, argument: str = "aggregates"

aggregates: Mapping[str, Aggregate | dict[str, Any]], argument: str = "aggregates"

haakonvt · 2026-08-20T11:16:05Z

+    for aggregate_id, aggregate in aggregates.items():
+        if not isinstance(aggregate_id, str):
+            raise TypeError(f"{argument!r} keys must be strings, not {type(aggregate_id).__name__}. {_AGGREGATES_HINT}")
+        if not isinstance(aggregate, (Aggregate, Mapping)):
+            raise TypeError(
+                f"{argument!r}[{aggregate_id!r}] must be an Aggregate or dict, "
+                f"not {type(aggregate).__name__}. {_AGGREGATES_HINT}"
+            )
+    return aggregates


Is the aggregate dict exactly one level deep? Just wondering if there are cases where we need to recurse.

Edit: I guess not - when we are inside the for aggregate_id, aggregate in aggregates.items(): and aggregate is a Mapping, then we just ship that off assuming it is valid anyway.

haakonvt · 2026-08-20T11:17:21Z

+    return aggregates
+
+
+def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter | dict[str, Any]]:


Suggested change

def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter | dict[str, Any]]:

def _validate_filters(filters: Sequence[Filter | dict[str, Any]], argument: str = "filters") -> Sequence[Filter | dict[str, Any]]:

haakonvt · 2026-08-20T11:18:50Z

+
+def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter | dict[str, Any]]:
+    """Validate the list of filter expressions one bucket is created per."""
+    hint = "Each filter creates one bucket, so a single filter must still be given as a list."


There is strong precedent in the SDK for allowing "singles" (and just wrapping in a list for the user)

haakonvt · 2026-08-20T11:19:13Z

+
+
+def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter | dict[str, Any]]:
+    """Validate the list of filter expressions one bucket is created per."""


Validate the list of filter expressions one bucket is created per.

Can you rephrase this please?

…te-containers # Conflicts: # cognite/client/_sync_api/data_modeling/records.py # cognite/client/data_classes/data_modeling/aggregates.py # tests/tests_unit/test_api/test_data_modeling/test_records.py # tests/tests_unit/test_data_classes/test_data_models/test_aggregates.py

…te-containers # Conflicts: # cognite/client/_sync_api/data_modeling/records.py

haakonvt · 2026-08-21T10:34:53Z

            )
+        if isinstance(aggregate, Mapping):
+            for body in aggregate.values():
+                if isinstance(body, Mapping) and "aggregates" in body:


Someone is gonna use the key "aggregates", mark my words 😂

but this isn't checking an object level where they are allowed custom keys?

haakonvt · 2026-08-21T10:37:27Z

+    def test_sync_record_list_public_load(self) -> None:
+        items = [
+            {"space": "sp", "externalId": f"rec-{i}", "createdTime": 1, "lastUpdatedTime": 2, "status": "created"}
+            for i in range(2)
+        ]
+        page = SyncRecordList.load(items)
+        assert isinstance(page, SyncRecordList)
+        assert [record.external_id for record in page] == ["rec-0", "rec-1"]
+        assert page.cursor is None
+        assert page.has_next is False
+        assert page.typing is None


load is tested automatically, no need for this

haakonvt · 2026-08-21T10:44:17Z

+    @pytest.mark.parametrize("limit", [None, -1])
+    def test_filter_rejects_unlimited(self, cognite_client: CogniteClient, stream_id: str, limit: object) -> None:
+        with pytest.raises((TypeError, ValueError), match="'limit'"):


Ideally you should be strict in what exception type is expected per paramterized input test

haakonvt

lgtm, some nits:

haakonvt · 2026-08-21T11:44:50Z

+    def test_none_passes_through(self) -> None:
+        assert _validate_aggregates(None) is None
+
+    def test_mapping_of_aggregate_instances_passes_through_unchanged(self) -> None:
+        aggs = {"avg_temp": Average(["sp", "c", "temp"]), "n": Count()}
+        assert _validate_aggregates(aggs) is aggs
+
+    def test_mapping_of_raw_dicts_passes_through_unchanged(self) -> None:
+        aggs = {"avg_temp": {"avg": {"property": ["sp", "c", "temp"]}}}
+        assert _validate_aggregates(aggs) is aggs
+
+    def test_valid_nested_aggregates_inside_a_raw_dict_passes(self) -> None:
+        aggs = {
+            "buckets": {
+                "uniqueValues": {"property": ["sp", "c", "player"], "aggregates": {"n": Count()}},
+            }
+        }
+        assert _validate_aggregates(aggs) is aggs


Could be parametrize'd

haakonvt · 2026-08-21T11:45:39Z

+        assert _validate_filters(raw) == [raw]
+
+    def test_sequence_of_filters_and_dicts_passes_through_unchanged(self) -> None:
+        seq: list[Filter | dict[str, Any]] = [filters.MatchAll(), {"matchAll": {}}]


mypy really needs this?

Suggested change

seq: list[Filter | dict[str, Any]] = [filters.MatchAll(), {"matchAll": {}}]

seq = [filters.MatchAll(), {"matchAll": {}}]

andersfylling and others added 3 commits August 7, 2026 01:00

Merge branch 'master' into fix/records-aggregate-containers

16ba92c

andersfylling marked this pull request as ready for review August 11, 2026 15:22

andersfylling requested review from a team as code owners August 11, 2026 15:22

Merge branch 'master' into fix/records-aggregate-containers

43767b1

gemini-code-assist Bot reviewed Aug 11, 2026

View reviewed changes

haakonvt reviewed Aug 20, 2026

View reviewed changes

andersfylling added 4 commits August 21, 2026 10:24

address review comments

fa7d116

Merge remote-tracking branch 'origin/master' into fix/records-aggrega…

11dfd31

…te-containers # Conflicts: # cognite/client/_sync_api/data_modeling/records.py

more tests

316d750

haakonvt reviewed Aug 21, 2026

View reviewed changes

haakonvt approved these changes Aug 21, 2026

View reviewed changes



		def _validate_aggregates(
		aggregates: Any, argument: str = "aggregates"

	aggregates: Any, argument: str = "aggregates"
	aggregates: Mapping[str, Aggregate \| dict[str, Any]], argument: str = "aggregates"

		return aggregates


		def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter \| dict[str, Any]]:

	def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter \| dict[str, Any]]:
	def _validate_filters(filters: Sequence[Filter \| dict[str, Any]], argument: str = "filters") -> Sequence[Filter \| dict[str, Any]]:



		def _validate_filters(filters: Any, argument: str = "filters") -> Sequence[Filter \| dict[str, Any]]:
		"""Validate the list of filter expressions one bucket is created per."""

	seq: list[Filter \| dict[str, Any]] = [filters.MatchAll(), {"matchAll": {}}]
	seq = [filters.MatchAll(), {"matchAll": {}}]

Uh oh!

Conversation

andersfylling commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

haakonvt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haakonvt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andersfylling commented Aug 6, 2026 •

edited

Loading

codecov Bot commented Aug 7, 2026 •

edited

Loading