fix(server): reject null/undefined in numeric rule comparisons#2442
fix(server): reject null/undefined in numeric rule comparisons#2442enoch85 wants to merge 20 commits into
Conversation
The rule comparator guard `(firstVal !== undefined || null)` did not reject null values due to JS operator precedence. Since getters return null for missing data (e.g. missing IMDb rating), null reached BIGGER/SMALLER comparisons where JS coerced it to 0, causing false matches. - Fix the null guard to use `firstVal != null` (rejects both null and undefined) - Add explicit number type checks in BIGGER/SMALLER so non-numeric operands return false - Add table-driven regression tests for numeric comparisons including null/undefined cases Closes Maintainerr#2441
Keep null→0 coercion for BIGGER/SMALLER to match existing behaviour and avoid unexpected rule changes on upgrade. Add debug logging when non-number values are encountered in numeric comparisons.
PR #2438 - feat(rules): add rule Jellyfin favorited by including parents - Added Jellyfin favorited rule that checks parent items (series/seasons) - Includes unit tests for the new favorited-by-parents getter PR #2439 - fix: stop rule execution when rule group is deleted mid-run - Changed onRuleGroupDeleted to call stopProcessingRuleGroup instead of removeFromQueue - Fires abort signal for in-flight execution, preventing null reference crashes - Added unit test for delete-during-execution scenario PR #2442 - fix(server): reject null/undefined in numeric rule comparisons - Fixed null guard using proper nullish check (`firstVal != null`) - Preserves v2.27.0 null→0 coercion for BIGGER/SMALLER to avoid upgrade breakage - Logs warning when non-number values are encountered in numeric comparisons - Updated regression tests for numeric comparisons including null/undefined cases
Why this fix is betterThis fix is better for two reasons: it closes the real safety issue, and it leaves the codebase in a better shape to maintain. On the rule-engine side, it fixes the actual failure mode instead of preserving legacy coercion behavior. Missing operands now go through one comparison path, are treated as a non-match, and are logged for debugging. Numeric On the rating side, the refactor is better because the shared logic is now actually shared. Provider/source alias handling and lookup behavior live in Which issue it solvesThis solves two related problems. First, it fixes the rule-engine bug behind PR #2442: missing numeric values no longer match numeric rules by accident. That is the critical bug, because it can cause unintended cleanup actions. The regression is covered by Second, it addresses the Plex rating extraction gap that came up in review. Plex can expose external ratings through both It also improves Jellyfin parity where it makes sense. Jellyfin still has server-specific limitations, but it now uses the same shared lookup mechanics for provider-based rating selection, with Jellyfin-specific fallback behavior kept local to the Jellyfin getter. Why the refactor is betterThe refactor is better because it moves the right logic to the right layer. The media-server layer now owns the cross-server part: provider aliases, source matching, and rating lookup semantics. That is the reusable part, so it belongs in The server-specific code still owns the server-specific part: how Plex exposes ratings, how Jellyfin exposes ratings, and what fallback behavior is reasonable when a server does not provide an exact provider-specific value. That separation keeps the abstraction honest instead of forcing Plex-specific or Jellyfin-specific behavior into a fake generic layer. The result is:
ValidationFocused tests passed for the relevant areas:
Overall, this is better than the original PR direction because it fixes the unsafe behavior at the root, improves Plex rating extraction instead of only changing comparator semantics, and leaves the rating logic more coherent for future maintenance. |
…o fix/null-comparison-in-rule-evaluator
Tighten the follow-up review changes around rule comparisons and rating lookups.\n\n- keep PlexMapper.toMediaRatings private again because only metadataToMediaRatings is used externally\n- simplify Jellyfin rating rule handling by calling the shared media rating helper directly and splitting the show-rating branches for readability\n- keep IMDb-specific Jellyfin rules strict while allowing TMDB/community fallback only where intended\n- align comparator typing, defensive numeric logging, and parameterized numeric comparison tests with the current code standards
…, #2442, #2406, #2386, #2370 PR #2466 - fix: honor Jellyfin played threshold - Respect configured played percentage threshold for Jellyfin watch status PR #2461 - feat(rules): add ARR disk target path selection for disk space rules - Allow selecting specific disk target paths for Radarr/Sonarr disk space rules PR #2458 - feat: clean up empty ended shows in Sonarr after season actions - Automatically remove ended shows from Sonarr when all seasons are processed PR #2453 - fix: improve Plex viewCount reliability and add isWatched boolean - Use native Plex viewCount field with watch history fallback - Add new isWatched boolean rule property PR #2452 - build(deps): bump actions/download-artifact from 7 to 8 PR #2451 - build(deps): bump actions/upload-artifact from 6 to 7 PR #2442 - fix(server): reject null/undefined in numeric rule comparisons - Add getComparisonResult wrapper that fails closed on null/undefined operands - Strict type checking for BIGGER/SMALLER comparisons PR #2406 - Metadata provider abstraction layer with TVDB support - Add MetadataService as central metadata resolution layer - TVDB support as alternative metadata provider - Dynamic provider preference with fallback - Replace TmdbIdService with unified MetadataService PR #2386 - feat: missing_episode rules - Add missing episode count as a rule property for Sonarr PR #2370 - build(deps-dev): bump the eslint group with 2 updates
|
This is now included in the You can check the latest commits here: https://github.com/Maintainerr/Maintainerr/commits/jellyfin-dev
Thank you very much! 🚀 |
Allow null lastViewedAt/sw_lastWatched values to match BEFORE rules for Plex, Jellyfin, and Tautulli while keeping other null comparisons fail-closed.\n\nThis preserves the numeric-comparison hardening from the PR, avoids making IN_LAST match never-watched items, and adds focused regression coverage for the allowed and disallowed null-date cases.
Promote missing rule operand logging from debug to warn so unexpected null rule values are more visible during rule execution.\n\nUpdate the comparator execution tests to assert on warn-level logging for the fail-closed paths.
Normalize top-level Plex IMDb and TMDB rating slots through the Plex mapper so provider-specific rule lookups continue to use the shared media rating abstraction.\n\nAlso add focused regression coverage and narrow diagnostics when Plex still returns rating-related fields that cannot be resolved to a requested provider rating.
PR #2442 - fix(server): reject null/undefined in numeric rule comparisons - Treat missing last-view dates as stale - Raise missing-operand rule logs to warn - Normalize Plex provider rating slots
|
Latest changes added to jellyfin-dev. 👍 |
2c1f21e to
eeacbf9
Compare
|
The latest changes complements the null-comparison fix by addressing the upstream cause of some false null IMDb values from Plex. During rule evaluation, the Plex getter was fetching metadata without requesting Plex’s external media payload, so provider-specific rating data could be missing even when Plex’s XML view showed an IMDb rating. That meant the comparator fix would correctly fail closed on null, but we were still generating null in cases where Plex actually had enough metadata. This change updates the Plex metadata fetch path so rule evaluation explicitly requests external rating data for the item and its parent or grandparent lookups, while preserving the existing Issue solved: Plex IMDb rating rules should no longer receive false null values just because the richer Plex metadata payload was not requested. |
|
@ydkmlt84 I've been on this issue now for the past 6 hours. It became a bigger than I initally had planned for, but it also makes the code leaner, and pushes more code to the shared abstraction layer, which to me is a good thing. The more we can share between the media servers, the better. I've tested in my prod, and it behaves as expected. I have nothing to add here now. Consider this PR to be done. 👍 
|
- Improve missing operand logging with watch state context - Add isWatchDateRule helper to constants service
9921ecc to
634e897
Compare
|
The last commits was an attempt to improve logging, but reverted it and we are now back to 634e897 |
|
Replaced by #2528 |
Summary
Fixes #2441 — Plex IMDb rating (and other nullable values) incorrectly matching
SMALLER/BIGGERrules when the value is missing.Root Cause
Two issues in
rule.comparator.service.ts:Broken null guard — The expression
(firstVal !== undefined || null)does not rejectnulldue to JS operator precedence. Since getters returnnullfor missing data, null values pass through to comparisons.Unsafe numeric coercion —
BIGGER/SMALLERuse rawval1 > val2/val1 < val2. JS coercesnullto0, sonull < 6evaluates totrue, causing false matches.Changes
rule.comparator.service.ts: Fix the null guard tofirstVal != null(rejects both null and undefined). Addtypeofnumber checks in BIGGER/SMALLER (consistent with how BEFORE/AFTER already guard for Date types).rule.comparator.service.doRuleAction.spec.ts: Replace individual BIGGER/SMALLER tests with a single table-drivennumericComparisonDataarray (follows existinglistCountDatapattern), covering normal cases and null/undefined edge cases.