From 9cc5c15e32b60d673df57505553aa006b86fdf75 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 31 May 2026 12:43:56 -0600 Subject: [PATCH] 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. --- app/src/components/Download.js | 6 +++--- app/src/hooks/useForm.js | 8 ++++++-- app/src/hooks/useForm.test.js | 22 ++++++++++++++++++++++ modules/videos/downloader.py | 8 ++++++-- modules/videos/lib.py | 21 ++++++++++++++++++--- modules/videos/schema.py | 2 +- wrolpi/schema.py | 24 +++++++++++++++++++++++- wrolpi/test/test_downloader.py | 32 ++++++++++++++++++++++++++++++++ 8 files changed, 111 insertions(+), 12 deletions(-) diff --git a/app/src/components/Download.js b/app/src/components/Download.js index a0b6e4fb..04b74679 100644 --- a/app/src/components/Download.js +++ b/app/src/components/Download.js @@ -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: [], diff --git a/app/src/hooks/useForm.js b/app/src/hooks/useForm.js index 298ad896..eaf14fb8 100644 --- a/app/src/hooks/useForm.js +++ b/app/src/hooks/useForm.js @@ -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 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' } diff --git a/app/src/hooks/useForm.test.js b/app/src/hooks/useForm.test.js index 55f10aec..5ea0e167 100644 --- a/app/src/hooks/useForm.test.js +++ b/app/src/hooks/useForm.test.js @@ -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: ''}}, diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py index 7102ef81..7422a287 100755 --- a/modules/videos/downloader.py +++ b/modules/videos/downloader.py @@ -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) diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 06b4cad0..dbda2b26 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -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 {} diff --git a/modules/videos/schema.py b/modules/videos/schema.py index 85d47363..652ea27b 100644 --- a/modules/videos/schema.py +++ b/modules/videos/schema.py @@ -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)} diff --git a/wrolpi/schema.py b/wrolpi/schema.py index 64ea1708..033d0753 100644 --- a/wrolpi/schema.py +++ b/wrolpi/schema.py @@ -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)} diff --git a/wrolpi/test/test_downloader.py b/wrolpi/test/test_downloader.py index 0e9b020e..d4f38ca6 100644 --- a/wrolpi/test/test_downloader.py +++ b/wrolpi/test/test_downloader.py @@ -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):