feat: add list_aggregates method for cross-address aggregate queries#285
feat: add list_aggregates method for cross-address aggregate queries#285
Conversation
|
Failed to retrieve llama text: POST 502: Bad Gateway |
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The PR adds a useful list_aggregates method for cross-address aggregate queries, but has several issues that need to be addressed. The main bug is an inconsistent API endpoint that's missing the .json extension used by all other list endpoints in the codebase. Additionally, the method is missing from the abstract base class, has no test coverage, and lacks input validation for pagination parameters.
src/aleph/sdk/client/http.py (line 251): BUG: Inconsistent API endpoint. The endpoint /api/v0/aggregates is missing the .json extension. All other list endpoints in this codebase use .json: /api/v0/posts.json, /api/v0/messages.json, and even the single aggregate endpoint uses /api/v0/aggregates/{address}.json. This will likely cause the API call to fail. Should be /api/v0/aggregates.json.
src/aleph/sdk/client/abstract.py (line 108): Missing abstract method declaration. The new list_aggregates method should be declared in the AlephClient abstract base class (similar to fetch_aggregate, fetch_aggregates, etc.) so that it's part of the public API contract and other client implementations know they need to implement it.
src/aleph/sdk/client/http.py (line 220): Inconsistent parameter type. The keys parameter is Optional[str] expecting a comma-separated string, while the existing fetch_aggregates method (line 214) uses Optional[Iterable[str]] and joins them internally. For API consistency, consider using Iterable[str] here too and joining internally like the existing methods do.
src/aleph/sdk/client/http.py (line 224): Missing input validation. The docstring mentions pagination should be 1-500, but there's no validation to enforce this. Consider adding bounds checking similar to how get_posts_cursor limits page_size to 200 (line 326).
tests/unit/test_asynchronous_get.py (line 67): Missing test coverage. There are no tests for the new list_aggregates method. Add tests following the pattern of existing aggregate tests (test_fetch_aggregate, test_fetch_aggregates, etc.) to verify the method works correctly with various filter combinations and pagination parameters.
src/aleph/sdk/client/http.py (line 226): Consider typed response. The method returns Dict[str, Any] instead of a typed response class like PostsResponse or MessagesResponse. Consider creating an AggregatesListResponse class in query/responses.py for better type safety.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR correctly implements a new list_aggregates method for cross-address aggregate queries. The implementation follows existing patterns in the codebase: the abstract method is properly defined in AlephClient, the HTTP implementation in AlephHttpClient uses proper pagination clamping and parameter handling, the AggregatesResponse type correctly extends PaginationResponse, and tests cover both basic usage and filtering scenarios. The code is clean, well-documented, and consistent with the project's style.
src/aleph/sdk/client/http.py (line 228): The pagination clamping max(1, min(pagination, 500)) is good. Consider adding a similar validation for page to ensure it's >= 1, for consistency with the docstring that says "Page number (starts at 1)".
tests/unit/test_asynchronous_get.py (line 69): The tests are good. Consider adding a test for pagination edge cases (e.g., pagination=0 or pagination=1000) to verify the clamping behavior works correctly.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR adds a new list_aggregates method that allows cross-address aggregate queries with filtering and pagination. The implementation follows existing code patterns consistently: the response model extends PaginationResponse correctly, the HTTP client implementation mirrors similar methods like get_credit_history, and the parameter handling (comma-joined lists) matches the existing _fetch_aggregates pattern. Pagination is properly validated to the 1-500 range. Tests cover basic functionality with keys and addresses filtering. The code is clean, well-documented, and ready to merge.
src/aleph/sdk/client/http.py (line 223): Minor: Consider adding validation for sort_by and sort_order parameters. The API likely accepts specific values (e.g., sort_order should probably be restricted to -1 or 1). Currently, invalid values would be sent to the API and fail there.
tests/unit/test_asynchronous_get.py (line 70): Minor: The tests could be enhanced to verify that parameters are actually passed to the API. Currently, the mock returns the same response regardless of parameters. Consider adding a test that checks the request params or a test for pagination boundaries (e.g., pagination=0 should be clamped to 1).
No description provided.