Skip to content

fix(records): validate aggregate and filter container shapes - #2748

Open
andersfylling wants to merge 4 commits into
masterfrom
fix/records-aggregate-containers
Open

fix(records): validate aggregate and filter container shapes#2748
andersfylling wants to merge 4 commits into
masterfrom
fix/records-aggregate-containers

Conversation

@andersfylling

Copy link
Copy Markdown
Contributor

aggregates is a mapping of the aggregate IDs you choose to aggregates, and a Filters aggregate takes a sequence. Passing an aggregate directly, or a list of them, 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
Filters(filters=filters.MatchAll())        # TypeError: 'MatchAll' object is not iterable

Both shapes are now validated where they are taken — the top-level aggregates argument, the nested aggregates of every bucket aggregate, and Filters.filters — with a TypeError that shows the expected shape.

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

🤖 Generated with Claude Code

andersfylling and others added 3 commits August 7, 2026 01:00
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

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.76%. Comparing base (629c506) to head (43767b1).

Files with missing lines Patch % Lines
...te/client/data_classes/data_modeling/aggregates.py 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2748      +/-   ##
==========================================
- Coverage   93.79%   93.76%   -0.03%     
==========================================
  Files         513      513              
  Lines       52445    52481      +36     
==========================================
+ Hits        49189    49210      +21     
- Misses       3256     3271      +15     
Files with missing lines Coverage Δ
cognite/client/_api/data_modeling/records.py 97.53% <100.00%> (ø)
cognite/client/_sync_api/data_modeling/records.py 100.00% <ø> (ø)
...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%> (ø)
...te/client/data_classes/data_modeling/aggregates.py 98.89% <91.66%> (-0.72%) ⬇️

... and 4 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.

@andersfylling
andersfylling marked this pull request as ready for review August 11, 2026 15:22
@andersfylling
andersfylling requested review from a team as code owners August 11, 2026 15:22

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant