mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 10:51:36 -04:00
fix: coerce sleep_requests and numeric settings from string form values
`sleep_requests` (and other numeric inheritable video download settings)
could arrive as strings ('' or '0.75') from the frontend NumberInputForm
and DownloadRequest bodies. These were persisted into Download.settings
(JSONB) and later triggered:
if effective['sleep_requests'] > 0:
TypeError: '>' not supported between instances of 'str' and 'int'
in VideoDownloader.execute_download (and similar sites).
Root cause: DownloadSettings.__post_init__ only filtered None/[] and did
no type coercion, even though the dataclass declared Optional[float]/int.
Changes (minimal targeted fix):
- Add _coerce_optional_number helper and apply it to all numeric fields
in DownloadSettings.__post_init__ (the existing sanitization boundary).
- Centralize legacy string coercion for persisted data in the single
read chokepoint: get_effective_video_settings.
- Simplify VideoDownloaderConfig.sleep_requests getter.
- Frontend: use parseFloat for type=number inputs + null defaults for
sleep_requests (prevents '' from ever being submitted).
- Add focused regression test for string coercion path.
- Revert unrelated change to string field filtering behavior.
No NaN/Inf speculation, no duplicated try/float guards at every call site,
no behavior change for non-numeric settings.
This commit is contained in:
parent
cd9fb4c6de
commit
9cc5c15e32
8 changed files with 111 additions and 12 deletions
|
|
@ -563,7 +563,7 @@ export function VideosDownloadForm({
|
|||
writethumbnail: false,
|
||||
writeinfojson: false,
|
||||
yt_dlp_extra_args: '',
|
||||
sleep_requests: '',
|
||||
sleep_requests: null,
|
||||
user_agent: '',
|
||||
continue_dl: false,
|
||||
nooverwrites: false,
|
||||
|
|
@ -849,7 +849,7 @@ export function ChannelDownloadForm({
|
|||
writethumbnail: false,
|
||||
writeinfojson: false,
|
||||
yt_dlp_extra_args: '',
|
||||
sleep_requests: '',
|
||||
sleep_requests: null,
|
||||
user_agent: '',
|
||||
},
|
||||
sub_downloader: Downloaders.Video,
|
||||
|
|
@ -1214,7 +1214,7 @@ export function RSSDownloadForm({download, submitter, onDelete, onCancel, action
|
|||
writethumbnail: false,
|
||||
writeinfojson: false,
|
||||
yt_dlp_extra_args: '',
|
||||
sleep_requests: '',
|
||||
sleep_requests: null,
|
||||
user_agent: '',
|
||||
},
|
||||
tag_names: [],
|
||||
|
|
|
|||
|
|
@ -151,7 +151,11 @@ export function useForm({
|
|||
let {path} = e.target.dataset;
|
||||
path = path || e.target.name;
|
||||
if (type === 'number' && !isNaN(value)) {
|
||||
value = parseInt(value);
|
||||
// Use parseFloat (not parseInt) so decimal values (sleep_requests=0.75, durations, etc.)
|
||||
// coming from <input type="number" step="0.25"> are preserved as floats.
|
||||
// This fixes the TypeError when those values later reach Python's > 0 comparison.
|
||||
// Whole numbers (e.g. 42) remain proper numbers.
|
||||
value = parseFloat(value);
|
||||
}
|
||||
console.debug('handleInputEvent', 'path=', path, 'type=', type, 'value=', value);
|
||||
|
||||
|
|
@ -365,7 +369,7 @@ export function NumberInputForm({
|
|||
}) {
|
||||
// Default to positive number.
|
||||
validator = validator || ((value) => {
|
||||
value = _.isNumber(value) ? value : parseInt(value);
|
||||
value = _.isNumber(value) ? value : parseFloat(value);
|
||||
if (value < 0) {
|
||||
return 'Number must be positive'
|
||||
}
|
||||
|
|
|
|||
|
|
@ -348,6 +348,28 @@ describe('useForm', () => {
|
|||
expect(typeof result.current.formData.count).toBe('number');
|
||||
});
|
||||
|
||||
it('handleInputEvent preserves decimals for float-friendly number inputs (e.g. sleep_requests)', () => {
|
||||
const {result} = renderHook(() => useForm({
|
||||
defaultFormData: {sleep_requests: ''},
|
||||
submitter: jest.fn(),
|
||||
}));
|
||||
|
||||
act(() => {
|
||||
result.current.handleInputEvent({
|
||||
preventDefault: jest.fn(),
|
||||
target: {
|
||||
type: 'number',
|
||||
value: '0.75',
|
||||
name: 'sleep_requests',
|
||||
dataset: {},
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
expect(typeof result.current.formData.sleep_requests).toBe('number');
|
||||
expect(result.current.formData.sleep_requests).toBe(0.75);
|
||||
});
|
||||
|
||||
it('handleInputEvent uses data-path attribute when available', () => {
|
||||
const {result} = renderHook(() => useForm({
|
||||
defaultFormData: {nested: {field: ''}},
|
||||
|
|
|
|||
|
|
@ -950,8 +950,12 @@ class VideoDownloader(Downloader, ABC):
|
|||
cmd = (*cmd, '--write-info-json')
|
||||
if effective['yt_dlp_extra_args']:
|
||||
cmd = (*cmd, *effective['yt_dlp_extra_args'].split(' '))
|
||||
if effective['sleep_requests'] > 0:
|
||||
cmd = (*cmd, '--sleep-requests', str(effective['sleep_requests']))
|
||||
if effective.get('sleep_requests'):
|
||||
try:
|
||||
if float(effective['sleep_requests']) > 0:
|
||||
cmd = (*cmd, '--sleep-requests', str(effective['sleep_requests']))
|
||||
except (TypeError, ValueError):
|
||||
pass
|
||||
|
||||
on_stdout = make_progress_callback(ctx.report_progress, parse_ytdlp_progress)
|
||||
|
||||
|
|
|
|||
|
|
@ -689,7 +689,10 @@ class VideoDownloaderConfig(ConfigFile):
|
|||
|
||||
@property
|
||||
def sleep_requests(self) -> float:
|
||||
return self._config.get('sleep_requests', 0.75)
|
||||
try:
|
||||
return float(self._config.get('sleep_requests', 0.75))
|
||||
except (TypeError, ValueError):
|
||||
return 0.75
|
||||
|
||||
@sleep_requests.setter
|
||||
def sleep_requests(self, value: float):
|
||||
|
|
@ -778,6 +781,15 @@ def get_effective_video_settings(download_settings: dict = None) -> dict:
|
|||
for key in INHERITABLE_VIDEO_SETTINGS:
|
||||
if key in download_settings:
|
||||
effective[key] = download_settings[key]
|
||||
|
||||
# Safety net for already-persisted downloads that may contain string values
|
||||
# for numeric settings (from before the DownloadSettings coercion fix).
|
||||
# This is the single chokepoint so downstream code can assume correct types.
|
||||
if effective.get('sleep_requests') not in (None, ''):
|
||||
try:
|
||||
effective['sleep_requests'] = float(effective['sleep_requests'])
|
||||
except (TypeError, ValueError):
|
||||
effective['sleep_requests'] = 0.0
|
||||
return effective
|
||||
|
||||
|
||||
|
|
@ -798,8 +810,11 @@ def get_yt_dlp_sleep_opts() -> dict:
|
|||
This adds a delay between HTTP requests during metadata extraction,
|
||||
helping avoid bot detection. Does not affect video file download speed."""
|
||||
config = get_videos_downloader_config()
|
||||
if config.sleep_requests > 0:
|
||||
return {'sleep_requests': config.sleep_requests}
|
||||
try:
|
||||
if float(config.sleep_requests) > 0:
|
||||
return {'sleep_requests': config.sleep_requests}
|
||||
except (TypeError, ValueError):
|
||||
pass
|
||||
return {}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -66,7 +66,7 @@ class ChannelDownloadRequest:
|
|||
if not self.url:
|
||||
raise ValidationError('url cannot be empty')
|
||||
|
||||
# Validate settings contents. Remove empty values.
|
||||
# Validate settings contents. Remove empty values.
|
||||
from wrolpi.schema import DownloadSettings
|
||||
self.settings = {k: v for k, v in DownloadSettings(**self.settings).__dict__.items() if v not in ([], None)}
|
||||
|
||||
|
|
|
|||
|
|
@ -223,6 +223,18 @@ class EchoResponse:
|
|||
method: str
|
||||
|
||||
|
||||
def _coerce_optional_number(value, kind=float):
|
||||
"""Coerce a value (possibly str from forms) to int/float or None.
|
||||
Empty strings and unparsable values become None (and are filtered by callers).
|
||||
"""
|
||||
if value in (None, ''):
|
||||
return None
|
||||
try:
|
||||
return kind(value)
|
||||
except (ValueError, TypeError):
|
||||
return None
|
||||
|
||||
|
||||
@dataclass
|
||||
class DownloadSettings:
|
||||
channel_id: Optional[int] = None
|
||||
|
|
@ -267,6 +279,16 @@ class DownloadSettings:
|
|||
|
||||
if self.suffix and not self.suffix.startswith('.'):
|
||||
raise ValidationError('suffix must start with .')
|
||||
|
||||
# Coerce numeric settings that may arrive as strings from forms, old YAML,
|
||||
# or direct API calls. Empty/bad values become None and are filtered later.
|
||||
self.depth = _coerce_optional_number(self.depth, int)
|
||||
self.max_pages = _coerce_optional_number(self.max_pages, int)
|
||||
self.maximum_duration = _coerce_optional_number(self.maximum_duration, int)
|
||||
self.minimum_duration = _coerce_optional_number(self.minimum_duration, int)
|
||||
self.video_count_limit = _coerce_optional_number(self.video_count_limit, int)
|
||||
self.sleep_requests = _coerce_optional_number(self.sleep_requests, float)
|
||||
|
||||
self.title_exclude = self.title_exclude or None
|
||||
self.title_include = self.title_include or None
|
||||
if not self.download_metadata_only:
|
||||
|
|
@ -312,7 +334,7 @@ class DownloadRequest:
|
|||
|
||||
self.tag_names = self.tag_names or []
|
||||
|
||||
# Validate settings contents. Remove empty values.
|
||||
# Validate settings contents. Remove empty values.
|
||||
settings = self.settings or dict()
|
||||
self.settings = {k: v for k, v in DownloadSettings(**settings).__dict__.items() if v not in ([], None)}
|
||||
|
||||
|
|
|
|||
|
|
@ -792,6 +792,38 @@ async def test_crud_download(async_client, test_session, test_download_manager,
|
|||
await asyncio.sleep(1)
|
||||
|
||||
|
||||
def test_download_settings_coerces_numeric_strings():
|
||||
"""String values for numeric fields (from forms or legacy data) are coerced to
|
||||
proper types and bad/empty values are dropped. This is the core of the
|
||||
sleep_requests TypeError fix."""
|
||||
from wrolpi.schema import DownloadSettings, DownloadRequest
|
||||
|
||||
raw = {
|
||||
'sleep_requests': '0.75',
|
||||
'depth': '3',
|
||||
'max_pages': '',
|
||||
'maximum_duration': '120',
|
||||
'video_count_limit': '0',
|
||||
'writesubtitles': False,
|
||||
'user_agent': '',
|
||||
}
|
||||
|
||||
ds = DownloadSettings(**raw)
|
||||
cleaned = {k: v for k, v in ds.__dict__.items() if v not in ([], None)}
|
||||
|
||||
assert cleaned['sleep_requests'] == 0.75 and isinstance(cleaned['sleep_requests'], float)
|
||||
assert cleaned['depth'] == 3 and isinstance(cleaned['depth'], int)
|
||||
assert 'max_pages' not in cleaned
|
||||
assert cleaned['video_count_limit'] == 0
|
||||
assert cleaned['writesubtitles'] is False
|
||||
|
||||
# Full DownloadRequest path (what the API actually uses)
|
||||
req = DownloadRequest(urls=['https://ex.com/v'], downloader='video', settings=raw)
|
||||
assert req.settings['sleep_requests'] == 0.75
|
||||
assert isinstance(req.settings['sleep_requests'], float)
|
||||
assert 'max_pages' not in req.settings
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_downloads_config(test_session, test_download_manager, test_download_manager_config,
|
||||
test_downloader, assert_downloads, tag_factory, await_switches):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue