feat: add GET /sessions endpoint with pagination#238
feat: add GET /sessions endpoint with pagination#238sharma-sugurthi wants to merge 1 commit intojenkinsci:mainfrom
Conversation
|
@berviantoleo right now, no session endpoint (POST, DELETE, GET message history) filters by user either, this is consistent with the current state of the codebase. also included pagination as discussed on PR #196 , |
Fixes jenkinsci#218 Adds a GET /sessions endpoint that returns a paginated list of active session IDs with message counts. Query parameters: - offset (int, default 0): starting index - limit (int, default 20): max sessions per page Response includes total, offset, limit for pagination metadata. Note: user_id param is stubbed in get_all_sessions() for future auth filtering (jenkinsci#78). Currently returns all sessions. Changes: - api/models/schemas.py — SessionSummary, SessionListResponse - api/services/memory.py — get_all_sessions(offset, limit, user_id) - api/routes/chatbot.py — GET /sessions route - tests/integration/test_chatbot.py — 7 integration tests
0e173e1 to
60d10f0
Compare
|
Most likely, I'll hold this until we have a proper handling of the session. |
|
Yeah @berviantoleo . Holding this until and unless,good handling of the session, what i also thought of.. |
divyansh-cyber
left a comment
There was a problem hiding this comment.
Overall this is a good addition. The schema design is clean, pagination is reasonable, and the new tests cover the basic happy paths well. get_all_sessions is straightforward and correctly guarded by the lock.
some points that i want to address:
The GET /sessions/{session_id}/message route has been removed from chatbot.py. This breaks the existing message history functionality and needs to be restored before this can be merged.
There is no validation on offset and limit in the route handler. Negative values or very large limits are currently accepted. These should be constrained using Query parameters.
test_list_sessions_pagination_custom only checks the number of returned sessions, not which sessions are returned. It should assert specific session IDs to properly validate pagination behavior.
Ensure get_all_sessions exists in memory.py in the same PR. If not, the import will cause a startup failure.
coming to my final verdict ,the rest of the changes look well-structured and reasonable.
|
@divyansh-cyber thanks.. "GET /sessions/{session_id}/message route removed" This route is not touched by this PR - you can verify by checking the diff. The GET message history endpoint is part of PR #196 (separate branch). Our diff only adds the new GET /sessions list endpoint. The POST /sessions/{session_id}/message and POST .../upload routes are untouched. "No validation on offset and limit" ,"test_list_sessions_pagination_custom should assert specific session IDs", Good catch and i update that those two of the things.But many be it will be holding this updates too, as @berviantoleo holding this feature enhancement. As sooner, this feature gets attention to make this enhancement concrete. I shall fix those. "Ensure get_all_sessions exists in memory.py" It does - it's part of this PR's diff in . You can see it in the files changed @berviantoleo any comments?? |
Thanks for the clarification. I see now that the GET /sessions/{session_id}/message route is handled in PR #196 and not part of this change. |
Fixes #218
What it does
Adds a
GET /sessionsendpoint that returns a paginated list of active session IDs with message counts.Query parameters
offsetlimitResponse format
{ "sessions": [ {"session_id": "abc-123", "message_count": 4} ], "total": 1, "offset": 0, "limit": 20 }