Skip to content

Comments

Add tournament teams endpoint to sendou proxy#64

Merged
cesaregarza merged 1 commit intomainfrom
feature/sendou-data-workaround-3
Jan 3, 2026
Merged

Add tournament teams endpoint to sendou proxy#64
cesaregarza merged 1 commit intomainfrom
feature/sendou-data-workaround-3

Conversation

@cesaregarza
Copy link
Owner

This commit introduces a new endpoint to fetch all teams in a tournament from sendou.ink. The route proxies requests to the sendou.ink turbo-stream endpoint and returns the team data in a structured 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 all teams in a tournament from sendou.ink. The route proxies requests to the sendou.ink turbo-stream endpoint and returns the team data in a structured 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 3, 2026

Pull Request Review: Add tournament teams endpoint to sendou proxy

Summary

This PR adds a new endpoint to fetch all teams in a tournament from sendou.ink. Overall, the implementation is solid and follows the existing patterns well. Below are my findings across different areas:


✅ Code Quality and Best Practices

Strengths:

  • Excellent consistency with existing code patterns in the file
  • Proper use of type hints (List[Dict[str, Any]])
  • Clear docstring and OpenAPI documentation
  • Follows the same error handling structure as sibling endpoints
  • Appropriate logging levels (warning for HTTP errors, error for decoding issues)

Minor Observations:

  • The code follows the DRY principle well by reusing _fetch_turbo_stream() and _decode_and_extract()
  • The endpoint naming and URL structure is consistent with the existing /to/{tournament_id}/teams/{team_id} endpoint

⚠️ Potential Issues

Data Structure Navigation (lines 204-210):
The nested dictionary access data["data"]["tournament"]["ctx"]["teams"] is fragile and makes assumptions about the response structure. While you do catch KeyError and TypeError, consider:

# Current approach catches errors but loses context
try:
    teams = data["data"]["tournament"]["ctx"]["teams"]
except (KeyError, TypeError):
    raise HTTPException(
        status_code=502,
        detail="Unexpected response structure from sendou.ink",
    )

Suggestions:

  1. Add validation that teams is actually a list before returning
  2. Log what keys were found when the structure doesn't match expectations (helps with debugging)
  3. Consider adding defensive checks:
try:
    teams = data["data"]["tournament"]["ctx"]["teams"]
    if not isinstance(teams, list):
        raise TypeError("teams is not a list")
except (KeyError, TypeError) as e:
    logger.error(
        "Unexpected response structure for tournament=%s: %s. Keys: %s",
        tournament_id,
        e,
        list(data.get("data", {}).keys()) if isinstance(data.get("data"), dict) else "N/A"
    )
    raise HTTPException(
        status_code=502,
        detail="Unexpected response structure from sendou.ink",
    )

🚀 Performance Considerations

Good:

  • Async/await pattern properly used throughout
  • Reuses existing httpx.AsyncClient with timeout
  • No blocking operations

Potential Concern:

  • For tournaments with many teams, the response could be large. Consider if pagination or response size limits should be implemented in the future (not a blocker for this PR, but worth noting)

🔒 Security Considerations

Status: GOOD

  • ✅ No user input is directly interpolated into URLs unsafely (tournament_id is validated by FastAPI's Path type)
  • ✅ HTTP errors are properly propagated without exposing internal details
  • ✅ Timeout is configured (REQUEST_TIMEOUT = 10.0)
  • ✅ No sensitive data exposure in error messages

🧪 Test Coverage

Status: MISSING

This is the main gap in this PR. There are no tests for this new endpoint. Based on the test structure in the repository, tests should be added.

Recommended test cases:

  1. Happy path: Successfully fetch teams list
  2. HTTP errors: sendou.ink returns 404, 500, etc.
  3. Network errors: Connection timeout, DNS failure
  4. Malformed responses: Invalid JSON, unexpected structure
  5. Empty teams list: Tournament exists but has no teams
  6. Type validation: Ensure returned data is a list

Example test structure (to be added in a follow-up or before merge):

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

@pytest.mark.asyncio
async def test_get_tournament_teams_success():
    # Mock successful response
    pass

@pytest.mark.asyncio
async def test_get_tournament_teams_http_error():
    # Mock 404 from sendou.ink
    pass

@pytest.mark.asyncio  
async def test_get_tournament_teams_invalid_structure():
    # Mock response with unexpected structure
    pass

📋 Summary & Recommendations

Approval Status: Approve with minor suggestions

This is well-written code that follows the project's patterns. The main recommendations are:

  1. Before merge (recommended):

    • Add basic test coverage for the new endpoint
    • Add type validation for the teams list before returning
  2. Can be follow-up work:

    • Enhanced logging when response structure doesn't match
    • Consider pagination strategy for large team lists

Overall: Strong implementation that fits well into the existing codebase. With tests added, this is ready to merge.

@cesaregarza cesaregarza merged commit 411cff0 into main Jan 3, 2026
7 checks passed
@cesaregarza cesaregarza deleted the feature/sendou-data-workaround-3 branch January 3, 2026 05:29
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