fix: resolve pylint and type-checking issues
- Fix return type annotation in SetupRedirectMiddleware.dispatch() to use Response instead of RedirectResponse - Replace broad 'except Exception' with specific exception types (FileNotFoundError, ValueError, OSError, etc.) - Rename AppConfig.validate() to validate_config() to avoid shadowing BaseModel.validate() - Fix ValidationResult.errors field to use List[str] with default_factory - Add pylint disable comments for intentional broad exception catches during shutdown - Rename lifespan parameter to _application to indicate unused variable - Update all callers to use new validate_config() method name
This commit is contained in:
parent
63742bb369
commit
3cb644add4
@ -43,8 +43,13 @@ from src.server.services.websocket_service import get_websocket_service
|
||||
|
||||
|
||||
@asynccontextmanager
|
||||
async def lifespan(app: FastAPI):
|
||||
"""Manage application lifespan (startup and shutdown)."""
|
||||
async def lifespan(_application: FastAPI):
|
||||
"""Manage application lifespan (startup and shutdown).
|
||||
|
||||
Args:
|
||||
_application: The FastAPI application instance (unused but required
|
||||
by the lifespan protocol).
|
||||
"""
|
||||
# Setup logging first with DEBUG level
|
||||
logger = setup_logging(log_level="DEBUG")
|
||||
|
||||
@ -72,8 +77,11 @@ async def lifespan(app: FastAPI):
|
||||
)
|
||||
|
||||
# Sync anime_directory from config.json to settings
|
||||
if config.other and config.other.get("anime_directory"):
|
||||
settings.anime_directory = str(config.other["anime_directory"])
|
||||
# config.other is Dict[str, object] - pylint doesn't infer this
|
||||
other_settings = dict(config.other) if config.other else {}
|
||||
if other_settings.get("anime_directory"):
|
||||
anime_dir = other_settings["anime_directory"]
|
||||
settings.anime_directory = str(anime_dir)
|
||||
logger.info(
|
||||
"Loaded anime_directory from config: %s",
|
||||
settings.anime_directory
|
||||
@ -82,7 +90,7 @@ async def lifespan(app: FastAPI):
|
||||
logger.debug(
|
||||
"anime_directory not found in config.other"
|
||||
)
|
||||
except Exception as e:
|
||||
except (OSError, ValueError, KeyError) as e:
|
||||
logger.warning("Failed to load config from config.json: %s", e)
|
||||
|
||||
# Initialize progress service with event subscription
|
||||
@ -131,7 +139,7 @@ async def lifespan(app: FastAPI):
|
||||
"Download service initialization skipped - "
|
||||
"anime directory not configured"
|
||||
)
|
||||
except Exception as e:
|
||||
except (OSError, RuntimeError, ValueError) as e:
|
||||
logger.warning("Failed to initialize download service: %s", e)
|
||||
# Continue startup - download service can be initialized later
|
||||
|
||||
@ -152,12 +160,14 @@ async def lifespan(app: FastAPI):
|
||||
|
||||
# Shutdown download service and its thread pool
|
||||
try:
|
||||
from src.server.services.download_service import _download_service_instance
|
||||
from src.server.services.download_service import ( # noqa: E501
|
||||
_download_service_instance,
|
||||
)
|
||||
if _download_service_instance is not None:
|
||||
logger.info("Stopping download service...")
|
||||
await _download_service_instance.stop()
|
||||
logger.info("Download service stopped successfully")
|
||||
except Exception as e:
|
||||
except Exception as e: # pylint: disable=broad-exception-caught
|
||||
logger.error("Error stopping download service: %s", e, exc_info=True)
|
||||
|
||||
# Close database connections
|
||||
@ -165,7 +175,7 @@ async def lifespan(app: FastAPI):
|
||||
from src.server.database.connection import close_db
|
||||
await close_db()
|
||||
logger.info("Database connections closed")
|
||||
except Exception as e:
|
||||
except Exception as e: # pylint: disable=broad-exception-caught
|
||||
logger.error("Error closing database: %s", e, exc_info=True)
|
||||
|
||||
logger.info("FastAPI application shutdown complete")
|
||||
|
||||
@ -11,7 +11,7 @@ from typing import Callable
|
||||
|
||||
from fastapi import Request
|
||||
from starlette.middleware.base import BaseHTTPMiddleware
|
||||
from starlette.responses import RedirectResponse
|
||||
from starlette.responses import RedirectResponse, Response
|
||||
from starlette.types import ASGIApp
|
||||
|
||||
from src.server.services.auth_service import auth_service
|
||||
@ -91,11 +91,11 @@ class SetupRedirectMiddleware(BaseHTTPMiddleware):
|
||||
config = config_service.load_config()
|
||||
|
||||
# Validate the loaded config
|
||||
validation = config.validate()
|
||||
validation = config.validate_config()
|
||||
if not validation.valid:
|
||||
return True
|
||||
|
||||
except Exception:
|
||||
except (FileNotFoundError, ValueError, OSError, AttributeError):
|
||||
# If we can't load or validate config, setup is needed
|
||||
return True
|
||||
|
||||
@ -103,7 +103,7 @@ class SetupRedirectMiddleware(BaseHTTPMiddleware):
|
||||
|
||||
async def dispatch(
|
||||
self, request: Request, call_next: Callable
|
||||
) -> RedirectResponse:
|
||||
) -> Response:
|
||||
"""Process the request and redirect to setup if needed.
|
||||
|
||||
Args:
|
||||
|
||||
@ -58,8 +58,9 @@ class ValidationResult(BaseModel):
|
||||
"""Result of a configuration validation attempt."""
|
||||
|
||||
valid: bool = Field(..., description="Whether the configuration is valid")
|
||||
errors: Optional[List[str]] = Field(
|
||||
default_factory=list, description="List of validation error messages"
|
||||
errors: List[str] = Field(
|
||||
default_factory=lambda: [],
|
||||
description="List of validation error messages"
|
||||
)
|
||||
|
||||
|
||||
@ -71,14 +72,16 @@ class AppConfig(BaseModel):
|
||||
|
||||
name: str = Field(default="Aniworld", description="Application name")
|
||||
data_dir: str = Field(default="data", description="Base data directory")
|
||||
scheduler: SchedulerConfig = Field(default_factory=SchedulerConfig)
|
||||
scheduler: SchedulerConfig = Field(
|
||||
default_factory=SchedulerConfig
|
||||
)
|
||||
logging: LoggingConfig = Field(default_factory=LoggingConfig)
|
||||
backup: BackupConfig = Field(default_factory=BackupConfig)
|
||||
other: Dict[str, object] = Field(
|
||||
default_factory=dict, description="Arbitrary other settings"
|
||||
)
|
||||
|
||||
def validate(self) -> ValidationResult:
|
||||
def validate_config(self) -> ValidationResult:
|
||||
"""Perform light-weight validation and return a ValidationResult.
|
||||
|
||||
This method intentionally avoids performing IO (no filesystem checks)
|
||||
@ -98,7 +101,8 @@ class AppConfig(BaseModel):
|
||||
errors.append(msg)
|
||||
|
||||
# backup.path must be set when backups are enabled
|
||||
if self.backup.enabled and (not self.backup.path):
|
||||
backup_data = self.model_dump().get("backup", {})
|
||||
if backup_data.get("enabled") and not backup_data.get("path"):
|
||||
errors.append(
|
||||
"backup.path must be set when backups.enabled is true"
|
||||
)
|
||||
|
||||
@ -90,7 +90,7 @@ class ConfigService:
|
||||
config = AppConfig(**data)
|
||||
|
||||
# Validate configuration
|
||||
validation = config.validate()
|
||||
validation = config.validate_config()
|
||||
if not validation.valid:
|
||||
errors = ', '.join(validation.errors or [])
|
||||
raise ConfigValidationError(
|
||||
@ -123,7 +123,7 @@ class ConfigService:
|
||||
ConfigValidationError: If config validation fails
|
||||
"""
|
||||
# Validate before saving
|
||||
validation = config.validate()
|
||||
validation = config.validate_config()
|
||||
if not validation.valid:
|
||||
errors = ', '.join(validation.errors or [])
|
||||
raise ConfigValidationError(
|
||||
@ -180,7 +180,7 @@ class ConfigService:
|
||||
Returns:
|
||||
ValidationResult: Validation result with errors if any
|
||||
"""
|
||||
return config.validate()
|
||||
return config.validate_config()
|
||||
|
||||
def create_backup(self, name: Optional[str] = None) -> Path:
|
||||
"""Create backup of current configuration.
|
||||
|
||||
@ -44,12 +44,12 @@ def test_appconfig_and_config_update_apply_to():
|
||||
def test_backup_and_validation():
|
||||
cfg = AppConfig()
|
||||
# default backups disabled -> valid
|
||||
res: ValidationResult = cfg.validate()
|
||||
res: ValidationResult = cfg.validate_config()
|
||||
assert res.valid is True
|
||||
|
||||
# enable backups but leave path empty -> invalid
|
||||
cfg.backup.enabled = True
|
||||
cfg.backup.path = ""
|
||||
res2 = cfg.validate()
|
||||
res2 = cfg.validate_config()
|
||||
assert res2.valid is False
|
||||
assert any("backup.path" in e for e in res2.errors)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user