Fix QA review issues: route path, blocking I/O, API key leak, cache
- CRIT-1: Change weather route from /weather to / (was doubling prefix) - CRIT-2: Use run_in_executor for urllib calls + parallel fetch - WARN-1: Invalidate weather cache when city changes - WARN-2: Sanitize error messages to prevent API key leakage - SUG-2: Only enable weather query when city is configured - SUG-4: Remove duplicate Bell import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
ca8b654471
commit
374e07708f
@ -2,6 +2,7 @@ from fastapi import APIRouter, Depends, HTTPException
|
|||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
from datetime import datetime, timedelta
|
from datetime import datetime, timedelta
|
||||||
|
import asyncio
|
||||||
import urllib.request
|
import urllib.request
|
||||||
import urllib.parse
|
import urllib.parse
|
||||||
import urllib.error
|
import urllib.error
|
||||||
@ -17,16 +18,16 @@ router = APIRouter()
|
|||||||
_cache: dict = {}
|
_cache: dict = {}
|
||||||
|
|
||||||
|
|
||||||
@router.get("/weather")
|
def _fetch_json(url: str) -> dict:
|
||||||
|
with urllib.request.urlopen(url, timeout=10) as resp:
|
||||||
|
return json.loads(resp.read().decode())
|
||||||
|
|
||||||
|
|
||||||
|
@router.get("/")
|
||||||
async def get_weather(
|
async def get_weather(
|
||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
current_user: Settings = Depends(get_current_session)
|
current_user: Settings = Depends(get_current_session)
|
||||||
):
|
):
|
||||||
# Check cache
|
|
||||||
now = datetime.now()
|
|
||||||
if _cache.get("expires_at") and now < _cache["expires_at"]:
|
|
||||||
return _cache["data"]
|
|
||||||
|
|
||||||
# Get city from settings
|
# Get city from settings
|
||||||
result = await db.execute(select(Settings))
|
result = await db.execute(select(Settings))
|
||||||
settings_row = result.scalar_one_or_none()
|
settings_row = result.scalar_one_or_none()
|
||||||
@ -34,20 +35,26 @@ async def get_weather(
|
|||||||
if not city:
|
if not city:
|
||||||
raise HTTPException(status_code=400, detail="No weather city configured")
|
raise HTTPException(status_code=400, detail="No weather city configured")
|
||||||
|
|
||||||
|
# Check cache (also invalidate if city changed)
|
||||||
|
now = datetime.now()
|
||||||
|
if _cache.get("expires_at") and now < _cache["expires_at"] and _cache.get("city") == city:
|
||||||
|
return _cache["data"]
|
||||||
|
|
||||||
api_key = app_settings.OPENWEATHERMAP_API_KEY
|
api_key = app_settings.OPENWEATHERMAP_API_KEY
|
||||||
if not api_key:
|
if not api_key:
|
||||||
raise HTTPException(status_code=500, detail="Weather API key not configured")
|
raise HTTPException(status_code=500, detail="Weather API key not configured")
|
||||||
|
|
||||||
try:
|
try:
|
||||||
# Current weather
|
loop = asyncio.get_event_loop()
|
||||||
current_url = f"https://api.openweathermap.org/data/2.5/weather?q={urllib.parse.quote(city)}&units=metric&appid={api_key}"
|
|
||||||
with urllib.request.urlopen(current_url, timeout=10) as resp:
|
|
||||||
current_data = json.loads(resp.read().decode())
|
|
||||||
|
|
||||||
# Forecast for rain probability
|
# Current weather + forecast in parallel via thread pool
|
||||||
|
current_url = f"https://api.openweathermap.org/data/2.5/weather?q={urllib.parse.quote(city)}&units=metric&appid={api_key}"
|
||||||
forecast_url = f"https://api.openweathermap.org/data/2.5/forecast?q={urllib.parse.quote(city)}&units=metric&cnt=8&appid={api_key}"
|
forecast_url = f"https://api.openweathermap.org/data/2.5/forecast?q={urllib.parse.quote(city)}&units=metric&cnt=8&appid={api_key}"
|
||||||
with urllib.request.urlopen(forecast_url, timeout=10) as resp:
|
|
||||||
forecast_data = json.loads(resp.read().decode())
|
current_data, forecast_data = await asyncio.gather(
|
||||||
|
loop.run_in_executor(None, _fetch_json, current_url),
|
||||||
|
loop.run_in_executor(None, _fetch_json, forecast_url),
|
||||||
|
)
|
||||||
|
|
||||||
rain_chance = 0
|
rain_chance = 0
|
||||||
for item in forecast_data.get("list", []):
|
for item in forecast_data.get("list", []):
|
||||||
@ -69,10 +76,11 @@ async def get_weather(
|
|||||||
# Cache for 1 hour
|
# Cache for 1 hour
|
||||||
_cache["data"] = weather_result
|
_cache["data"] = weather_result
|
||||||
_cache["expires_at"] = now + timedelta(hours=1)
|
_cache["expires_at"] = now + timedelta(hours=1)
|
||||||
|
_cache["city"] = city
|
||||||
|
|
||||||
return weather_result
|
return weather_result
|
||||||
|
|
||||||
except urllib.error.URLError as e:
|
except urllib.error.URLError:
|
||||||
raise HTTPException(status_code=502, detail=f"Weather service unavailable: {str(e)}")
|
raise HTTPException(status_code=502, detail="Weather service unavailable")
|
||||||
except (KeyError, json.JSONDecodeError) as e:
|
except (KeyError, json.JSONDecodeError):
|
||||||
raise HTTPException(status_code=502, detail=f"Invalid weather data: {str(e)}")
|
raise HTTPException(status_code=502, detail="Invalid weather data")
|
||||||
|
|||||||
@ -1,7 +1,7 @@
|
|||||||
import { useState, useEffect, useRef } from 'react';
|
import { useState, useEffect, useRef } from 'react';
|
||||||
import { useQuery } from '@tanstack/react-query';
|
import { useQuery } from '@tanstack/react-query';
|
||||||
import { format } from 'date-fns';
|
import { format } from 'date-fns';
|
||||||
import { Bell, Plus, Calendar as CalIcon, ListTodo, Bell as BellIcon } from 'lucide-react';
|
import { Bell, Plus, Calendar as CalIcon, ListTodo } from 'lucide-react';
|
||||||
import api from '@/lib/api';
|
import api from '@/lib/api';
|
||||||
import type { DashboardData, UpcomingResponse, WeatherData } from '@/types';
|
import type { DashboardData, UpcomingResponse, WeatherData } from '@/types';
|
||||||
import { useSettings } from '@/hooks/useSettings';
|
import { useSettings } from '@/hooks/useSettings';
|
||||||
@ -72,6 +72,7 @@ export default function DashboardPage() {
|
|||||||
},
|
},
|
||||||
staleTime: 30 * 60 * 1000,
|
staleTime: 30 * 60 * 1000,
|
||||||
retry: false,
|
retry: false,
|
||||||
|
enabled: !!settings?.weather_city,
|
||||||
});
|
});
|
||||||
|
|
||||||
if (isLoading) {
|
if (isLoading) {
|
||||||
@ -139,7 +140,7 @@ export default function DashboardPage() {
|
|||||||
className="flex items-center gap-2.5 w-full px-3 py-2 text-sm hover:bg-card-elevated transition-colors"
|
className="flex items-center gap-2.5 w-full px-3 py-2 text-sm hover:bg-card-elevated transition-colors"
|
||||||
onClick={() => { setQuickAddType('reminder'); setDropdownOpen(false); }}
|
onClick={() => { setQuickAddType('reminder'); setDropdownOpen(false); }}
|
||||||
>
|
>
|
||||||
<BellIcon className="h-4 w-4 text-orange-400" />
|
<Bell className="h-4 w-4 text-orange-400" />
|
||||||
Reminder
|
Reminder
|
||||||
</button>
|
</button>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user