Skip to content

fix(DatabaseSessionService): Align event filtering and ordering logic #915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ystory
Copy link

@ystory ystory commented May 24, 2025

This commit addresses inconsistencies in how DatabaseSessionService handles config.after_timestamp and config.num_recent_events parameters, aligning its behavior with InMemorySessionService and VertexAiSessionService.

Key changes:

  • Made after_timestamp filtering inclusive
  • Corrected num_recent_events behavior to fetch the N most recent events
  • Refined timezone handling for after_timestamp
  • Updated the unit test test_get_session_with_config to includeSessionServiceType.DATABASE, allowing verification of these fixes.

Fixes #911


Testing Plan

  1. The unit test test_get_session_with_config located in tests/unittests/sessions/test_session_service.py was updated to be parameterized with SessionServiceType.DATABASE. This ensures that the corrected logic in DatabaseSessionService is explicitly tested against the same criteria as other session services.
  2. All unit tests were executed by running pytest ./tests/unittests/sessions/test_session_service.py.
  3. As shown in the screenshot below, the tests for test_get_session_with_config, including all scenarios for SessionServiceType.DATABASE, have passed successfully.
Screenshot 2025-05-24 at 21 26 48

This commit addresses inconsistencies in how DatabaseSessionService
handles config.after_timestamp and config.num_recent_events
parameters, aligning its behavior with InMemorySessionService and
VertexAiSessionService.

Key changes:
- Made after_timestamp filtering inclusive
- Corrected num_recent_events behavior to fetch the N most recent events
- Refined timezone handling for after_timestamp
- Updated the unit test test_get_session_with_config to includeSessionServiceType.DATABASE, allowing verification of these fixes.

Fixes google#911
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.

Bug: DatabaseSessionService - Inconsistent after_timestamp and num_recent_events behavior
1 participant