fix: Data API table compatibility — ratings, views, activity, warning suppression#19
Merged
fix: Data API table compatibility — ratings, views, activity, warning suppression#19
Conversation
Add GitHub Actions workflow to build and publish container images to ghcr.io/killrvideo/kv-be-python-fastapi-dataapi-table on push to main and version tags. PRs get build-only validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move `record_user_activity` import to top of file instead of inside function body - Add module-level `logger = logging.getLogger(__name__)` and change except block from `logging.getLogger(__name__).debug(...)` to `logger.warning(...)` - Remove inline `import logging` from inside the except block - Patch `app.services.comment_service.record_user_activity` in `test_add_comment_success` to prevent real AstraDB calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove unnecessary camelCase aliases from UserActivity (DB cols are snake_case); keep validation_alias on UserActivityResponse for accepting snake_case input - Add ACTIVITY_TYPES = Literal["view","comment","rate"] and apply it to record_user_activity parameter and UserActivity.activity_type field - Add __all__ export list to user_activity.py consistent with comment.py - Serialize activity_timestamp as isoformat string before DB insert - Replace serial 30-day loop with asyncio.gather() for concurrent partition queries - Add MAX_ACTIVITY_ROWS = 1000 hard cap to prevent unbounded memory usage - Add per-partition try/except in _fetch_day_rows so one bad partition does not abort the entire read - Remove dead/overwritten mock setup code from test_list_user_activity_returns_paginated_results - Add test_list_user_activity_fetches_table (read-path get_table coverage) - Add test_list_user_activity_error_in_partition_is_skipped (error isolation) - Add test_record_user_activity_timestamp_is_isoformat_string Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ix tests (closes #14) - Wrap video_activity insert in its own try/except so a DB failure there never kills the 204 response after the views counter already succeeded - Move import of record_user_activity/ANONYMOUS_USER_ID outside the try block so ImportError is not silently swallowed - Upgrade logger.debug to logger.warning for both non-critical insert failures for production visibility - Update user_activity tests to patch record_user_activity directly instead of patching get_table — tests the contract, not implementation - Fix test_list_latest_videos: assert page_size=10 (was 3) to match call - Fix test_search_videos_by_keyword: patch search_videos_by_semantic (keyword search now delegates there) instead of list_videos_with_query Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ches in rating_service (closes #16) - Move deferred `from app.services.user_activity_service import record_user_activity` to module-level import in rating_service.py - Move the record_user_activity try/except call inside both the if (update) and else (insert) branches so a future early return cannot silently skip activity tracking - Add `test_rate_video_user_activity_failure_does_not_break` test that patches record_user_activity to raise and asserts the rating call still succeeds and returns a valid Rating object Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename test_record_user_activity_db_failure_propagates to test_record_user_activity_db_failure_raises with updated docstring - Simplify test_list_user_activity_fetches_table to only assert get_table is awaited, removing extra data assertions - Add test_list_user_activity_page_beyond_data to verify empty list returned with correct total on out-of-range page - Add test_get_user_activity_invalid_uuid to verify 422 on invalid UUID path parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use validation_alias instead of alias in UserActivityResponse so OpenAPI
spec emits camelCase field names (userId, activityType, activityId,
activityTimestamp) while model_validate() from snake_case DB objects still works
- Change route path from {user_id_path:uuid} to {user_id_path} with UUID
type annotation so invalid UUIDs produce 422 instead of 404
- Replace activity_type query param type str with Literal["view","comment","rate"]
for enum validation and OpenAPI documentation
- Replace max(1, math.ceil(...)) totalPages formula with integer division
so empty result sets return totalPages=0 consistently
- Fix test_get_user_activity_with_type_filter to assert item["activityType"]
directly instead of hedging with .get() fallback
- Regenerate docs/killrvideo_openapi.yaml to reflect all model/route changes
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously per_day_limit was set to MAX_ACTIVITY_ROWS (1000), allowing up to 30 x 1000 = 30,000 rows to be fetched across all 30 partitions before the post-gather trim. Changed to max(1, MAX_ACTIVITY_ROWS // 30) (~33) so the total rows fetched stays bounded at ~MAX_ACTIVITY_ROWS. Added test_list_user_activity_per_day_limit_is_bounded to assert that find() is called with the smaller per-partition limit for all 30 partitions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o_id (closes #14) - Remove early `return` from UNKNOWN_TABLE_COLUMNS/UNSUPPORTED_UPDATE_OPERATIONS branch so video_activity and user_activity inserts still run even when the views counter update is unavailable. - Pass `activity_id=video_id` to `record_user_activity` so each user_activity row links back to the video that was viewed. - Add `record_user_activity` patch + `assert_awaited_once` to `test_record_video_view_success` as a regression guard. - Update `test_record_video_view_authenticated_user_activity` and `test_record_video_view_anonymous_user_activity` assertions to include the new `activity_id=vid` kwarg. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
) - Bound per-partition fetch limit to MAX_ACTIVITY_ROWS // 30 (#13) - Pass viewer_user_id from endpoint to record_video_view (#14) - Change early-return to pass so activity tracking isn't skipped (#14) - Pass video_id as activity_id in view tracking (#14) - Patch record_user_activity contract in comment tests, not DB internals (#15) - Add totalPages==0 assertion and invalid activity_type 422 test (#17) - Graceful error handling in record_user_activity with logger.warning (#18) - Add test verifying warning is logged on DB failure (#18) 172 tests pass, 0 new failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mary table Replace full-scan recompute (scan all ratings → update videos table) with $inc-based counter updates on video_ratings summary table. New ratings increment rating_counter and rating_total; updates adjust only rating_total by the delta. get_video_ratings_summary now reads directly from the summary table rather than pulling pre-computed fields from the video record. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
video_id is a v4 UUID but user_activity.activity_id is a TimeUUID (v1) column in Cassandra. Passing a v4 UUID caused INVALID_DATABASE_QUERY errors on every video view. Fix by dropping the explicit activity_id argument so record_user_activity auto-generates a uuid1() as intended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
countDocuments is unsupported on CQL tables, and astrapy logs a WARNING before raising DataAPIResponseException. safe_count already catches and handles this case correctly (returning fallback_len), but the warning appeared in logs as a false alarm. Fix: attach a targeted logging.Filter to the astrapy.utils.api_commander logger during the count_documents call. The filter drops only records containing UNSUPPORTED_TABLE_COMMAND; all other errors still surface. Also fix conftest.py: was installing astrapy stubs even when astrapy is installed, because the sys.modules check fired before any app code imported astrapy. Now uses importlib.util.find_spec() to check installability, not presence in sys.modules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Astra DB Table API does not support $inc (only $set, $unset, $push, $pullAll are supported). The previous code attempted $inc, failed with UNSUPPORTED_UPDATE_OPERATIONS_FOR_TABLE, logged a misleading "view tracking disabled" warning, then fell through to a $set fallback — a wasted round trip and a noisy false alarm on every view. Fix: go directly to read-modify-write with $set. Removes the $inc attempt, the complex two-if exception handler, and the _logged_views_disabled global. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ideo listing Adds `suppress_astrapy_warnings` context manager to `db_helpers.py` and applies it around the `find()` call in `list_videos_with_query()` to silence pre-existing performance-advisory warnings that astrapy emits when listing all videos without a partition key filter. Documented in GitHub issue #21. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pmcfadin
changed the title
aar0np
approved these changes
Mar 3, 2026
Contributor
aar0np
left a comment
aar0np
left a comment
There was a problem hiding this comment.
Damn. That's a lot.
APPROVED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
$inc-based counters onvideo_ratingssummary table for_update_video_aggregate_rating()$inc(Table API only supports$set) with read-modify-write pattern —find_one→ increment in Python →update_onewith$setactivity_id=video_idfromrecord_user_activity()call; the column isTimeUUID(v1) butvideo_idis a v4 UUID — let the service auto-generate auuid1()countDocumentswarning suppression: Table API doesn't supportcountDocuments;safe_count()indb_helpers.pyalready had a fallback, but astrapy logs a WARNING before raising — suppress it with a targetedlogging.Filteronastrapy.utils.api_commanderIN_MEMORY_SORTING/ZERO_FILTERwarning suppression: Addsuppress_astrapy_warningscontext manager and apply it around thefind()call inlist_videos_with_query(); documented in issue perf: video listing query causes IN_MEMORY_SORTING and ZERO_FILTER warnings on Astra DB #21conftest.pystub timing fix: Was using"astrapy" not in sys.moduleswhich fires before any app code imports astrapy, causing stubs to shadow real classes in tests; fixed toimportlib.util.find_spec("astrapy") is NoneTest Plan
pytest— expect 190 passed, 6 pre-existing failures (semantic search, vector ingest, otlp endpoints not covered by unit tests)UNSUPPORTED_TABLE_COMMANDwarning in logsIN_MEMORY_SORTINGorZERO_FILTERwarnings in logsInvalid version for TimeUUID typeerror in logs🤖 Generated with Claude Code