Skip to content

Comments

Add SendouQ match endpoint to sendou proxy#67

Merged
cesaregarza merged 1 commit intomainfrom
feature/sendouq-api-endpoint
Jan 6, 2026
Merged

Add SendouQ match endpoint to sendou proxy#67
cesaregarza merged 1 commit intomainfrom
feature/sendouq-api-endpoint

Conversation

@cesaregarza
Copy link
Owner

This commit introduces a new endpoint to fetch SendouQ match data from sendou.ink. The route proxies requests to the sendou.ink turbo-stream endpoint and returns the data in JSON format. Comprehensive error handling is included to manage potential HTTP and decoding issues, ensuring robust functionality and a seamless user experience.

This commit introduces a new endpoint to fetch SendouQ match data from sendou.ink. The route proxies requests to the sendou.ink turbo-stream endpoint and returns the data in JSON format. Comprehensive error handling is included to manage potential HTTP and decoding issues, ensuring robust functionality and a seamless user experience.
@claude
Copy link

claude bot commented Jan 6, 2026

Pull Request Review: Add SendouQ match endpoint

Summary

This PR adds a new endpoint to fetch SendouQ match data from sendou.ink, following the established pattern in the codebase. Overall, the implementation is clean and consistent with existing endpoints.

Code Quality & Best Practices ✅

Strengths:

  • Follows the existing pattern perfectly - the code is nearly identical to the other endpoints in the file (tournament match, tournament team)
  • Proper use of type hints (Dict[str, Any], int)
  • Clear docstring and FastAPI documentation (summary, description)
  • Consistent code style with the rest of the module
  • Proper use of logging at appropriate levels (warning for HTTP errors, error for system issues)

Consistency: The implementation at src/fast_api_app/routes/sendou_proxy.py:216-259 mirrors the existing endpoints exactly, which is good for maintainability.

Error Handling & Potential Issues ✅

Strengths:

  • Comprehensive error handling for both HTTPStatusError and RequestError
  • Proper error propagation with appropriate HTTP status codes (502 for upstream failures)
  • ValueError handling for decode failures

No issues found - error handling is robust and follows best practices.

Performance Considerations ⚠️

Minor Observation:

  • The endpoint uses httpx.AsyncClient correctly for async I/O
  • Uses the configured REQUEST_TIMEOUT of 10 seconds (line 23)
  • No caching is implemented, but this is consistent with other endpoints in the file

Recommendation: Consider implementing a caching layer for all sendou proxy endpoints in a future PR to reduce load on sendou.ink and improve response times. This could be a shared improvement across all endpoints.

Security Concerns ✅

Strengths:

  • Input validation via FastAPI's Path() parameter with integer type enforcement
  • No injection vulnerabilities - the match_id is an integer and is safely interpolated into the URL
  • Proper error message sanitization - doesn't leak sensitive information
  • Uses HTTPS for upstream requests (SENDOU_BASE_URL = "https://sendou.ink")

No security issues found.

Test Coverage ❌

Critical Gap:

  • No tests found for this new endpoint or any of the sendou proxy endpoints
  • I searched for test files but found no tests in tests/ directory for the sendou proxy routes

Recommendation:
Before merging, please add tests covering:

  1. Happy path: Successful fetch and decode of match data
  2. HTTP errors: 404 from sendou.ink (match not found), 500 errors
  3. Network errors: Request timeouts, connection failures
  4. Decode errors: Invalid turbo-stream data, missing route key
  5. Invalid input: Non-integer match_id (FastAPI should handle this, but good to verify)

Example test structure:

# tests/fast_api_app/routes/test_sendou_proxy.py
import pytest
from unittest.mock import AsyncMock, patch

async def test_get_q_match_success(client):
    """Test successful Q match retrieval"""
    # Mock _fetch_turbo_stream and _decode_and_extract
    # Assert response status and data structure

async def test_get_q_match_not_found(client):
    """Test 404 from sendou.ink"""
    # Mock HTTPStatusError with 404
    # Assert 404 response

async def test_get_q_match_decode_error(client):
    """Test decode failure"""
    # Mock ValueError from _decode_and_extract
    # Assert 502 response

Additional Notes

  1. Route Key: The new ROUTE_KEY_Q_MATCH (line 31) looks correct based on the pattern used for other routes
  2. API Documentation: The OpenAPI docs will automatically include this endpoint with good descriptions
  3. Logging: Consider adding a success log at debug level for observability: logger.debug("Successfully fetched q/match=%s", match_id)

Verdict

Conditional Approval: The code quality is excellent and follows all best practices. However, test coverage is missing entirely for this endpoint (and appears to be missing for all sendou proxy endpoints).

Before merging:

  • Add comprehensive tests for the new endpoint
  • Consider adding tests for existing endpoints as well (can be a separate PR)

Optional improvements:

  • Add debug-level success logging
  • Consider caching strategy for future optimization

Great work on maintaining consistency with the existing codebase! The implementation is solid - just needs tests to ensure reliability.

@cesaregarza cesaregarza merged commit c968088 into main Jan 6, 2026
7 checks passed
@cesaregarza cesaregarza deleted the feature/sendouq-api-endpoint branch January 6, 2026 06:28
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