Cascade explicit PUT monitored into tri-state fields so it actually persists - #87
Open
jbob06 wants to merge 2 commits into
Open
Cascade explicit PUT monitored into tri-state fields so it actually persists#87jbob06 wants to merge 2 commits into
jbob06 wants to merge 2 commits into
Conversation
…ersists
AuthorService.UpdateAuthor recomputes the legacy Author.Monitored flag
from the per-media-type tri-state settings on every save
(author.Monitored = author.IsMonitoredFromMediaSettings()), and
separately fills any still-null tri-state field from the root folder's
defaults before that recompute runs. So a PUT that only sets the
top-level "monitored" field - without touching any of the four
tri-state fields - has it silently discarded: whatever those fields
already were (or get defaulted to) wins, and the recompute overwrites
the requested value right back before it's ever persisted.
monitored:true "works" only when it happens to already match the
derived value; monitored:false never sticks once any tri-state field
for a configured media type is already on. Note that AuthorController's
own PutValidator/SharedValidator rules require Path and at least one
quality profile id on every PUT regardless of this fix, so the minimal
request that actually reaches this code still has to carry those - it
isn't as bare as "just send {id, monitored:false}".
Two changes to fix this for the native (non-facade) single-author PUT:
- AuthorResource.Monitored is now bool? instead of bool (matching the
existing bool? on AuthorEditorResource), since a non-nullable bool
can't distinguish "client omitted this field" from "client explicitly
sent false". Every other read of the field preserves its old
omitted-means-false behavior via `?? false` / `== true`, so this is
not a behavior change for any existing caller, including the
Readarr-facade write path, which is deliberately left exactly as it
was (now covered by two new tests in AuthorResourceMapperFixture).
- AuthorController.UpdateAuthor cascades an explicit Monitored value
into MonitorFuture (and, on turning off, MonitorExisting - zeroed,
since MonitorExisting > 0 alone keeps the derived flag true) for
whichever media types the author is configured for. A non-nullable
bool alone isn't enough to know intent, though: almost every real
client - Chaptarr's own UI monitor toggle included, and any script
that does GET -> flip one field -> PUT the whole object back - sends
a full object on every PUT, so a stale `monitored` AND stale
(unchanged) tri-state field values both ride along on saves that
have nothing to do with them. The cascade only treats a tri-state
field as "touched" by the client when its sent value actually
differs from what was stored - an earlier version of this fix
treated the field's mere presence as "touched," which broke the
GET-modify-PUT client shape specifically (its echoed, unchanged
tri-state fields read as edits, so every media type got skipped and
monitored:false silently failed to persist all over again). The
cascade itself only fires per media type when the requested
top-level value genuinely differs from what's already true for the
author as a whole, or when that media type is gaining a root folder
in this exact request (the one case where "nothing changed" isn't
safe to skip, since AuthorService's root-folder-defaults fill is
about to resolve that media type's tri-state fields for the first
time and could re-enable monitoring regardless of what was asked
for) - so a same-value echo of either kind is always a no-op, and
can't leak into a media type the request never meant to touch.
- Everything the cascade needs to know about the author's state BEFORE
this request - root folder paths, the four tri-state field values,
and the derived overall monitored status - is captured from the
stored author into one StoredAuthorMonitoringState value, in one
line, immediately after fetching it and BEFORE ToModel/ApplyChanges
mutate that same object in place (a PUT that doesn't include a root
folder path, for instance, would otherwise see that field already
nulled out by the time the cascade runs). The cascade method and
this new type are both internal (with the existing Chaptarr.Api.V1
-> Chaptarr.Core.Test InternalsVisibleTo) rather than private, so
tests call them directly instead of via reflection.
Known gap, left out of this PR: this doesn't guard a media type LOSING
its root folder in the same request - e.g. a PUT that clears
AudiobookRootFolderPath while also asking for monitored:true could
still cascade MonitorFuture=true onto a media type that no longer has
anywhere to look. That's downstream of a broader, pre-existing issue
in ApplyChanges: it copies Path/AudiobookRootFolderPath/
EbookRootFolderPath from the incoming resource unconditionally
(including to null, when a request simply omits them - not just when
it explicitly clears them), independent of this PR and already
reachable before it via the pre-existing unconditional Monitored
recompute. Fixing that asymmetry felt like its own change rather than
part of this one.
Only applies to the native, non-facade PUT path; the Readarr-facade
single-media-type request already gets an equivalent cascade inside
ToModel itself. The bulk author editor (AuthorEditorController) has
the identical underlying recompute bug (AuthorService.UpdateAuthors,
not UpdateAuthor) but is left out of this PR - it edits N authors at
once and needs its own per-author root-folder/prior-state snapshot
pass, which felt like its own change rather than a one-line extension
of this one. Happy to follow up on either gap if wanted.
Also updates openapi.json's AuthorResource.monitored schema to
nullable: true, matching the type change (AuthorEditorResource.monitored
was already documented that way).
Fixes the "PUT monitored:false does not persist" part of Chaptarr#17 (the
V5-matching / multi-pocket part of that issue is a metadata-server-side
data quality problem, not something fixable client-side, and is left
open).
jbob06
force-pushed
the
fix/author-monitored-false-not-persisting
branch
from
August 26, 2026 17:47
a85421a to
a1b7c23
Compare
AuthorService.UpdateAuthors has the identical recompute bug UpdateAuthor
had: it unconditionally sets author.Monitored = author.IsMonitoredFromMediaSettings()
on every save, discarding an explicit PUT /api/v1/author/editor
{"monitored": false} unless the tri-state fields also happen to end up
agreeing with it. AuthorEditorController.SaveAll already gates every
field write on the resource actually providing it (unlike the
single-author ApplyChanges path, it never overwrites a root folder path
with null just because a request omits it - it only ever sets one when
the resource sends a non-empty string), and already snapshots each
author's pre-mutation root folder paths for its own hydration-detection
purposes, so this fix is considerably more contained than the
single-author one was.
Refactors AuthorController.CascadeExplicitMonitoredIntoMediaTypeSettings
into a thin AuthorResource-shaped wrapper plus a primitive-parameter
core, so AuthorEditorController can call the same logic directly with
AuthorEditorResource's own fields (same four tri-state fields plus
Monitored, just not wrapped in an AuthorResource) without duplicating
it. Each author's pre-mutation state is captured via the same
StoredAuthorMonitoringState this PR already introduced, into a
per-author-id dictionary alongside the existing
previousRootFoldersByAuthorId/previousSyncByAuthorId snapshots, and the
cascade runs once per author inside the existing per-author loop, after
that author's own root-folder/tri-state field writes and before
AuthorService.UpdateAuthors is called. Skipped under a Readarr-facade
request, same as the single-author path, since this endpoint isn't
media-type-scoped by its own resource shape and a facade client could
otherwise leak a single-media-type intent into both.
While extracting the shared core, changed what "touched" means: from
"the client's request includes either of this media type's two
tri-state fields" to "the client's request includes a field, and its
value genuinely differs from what was stored" - per FIELD, not per
media type. The previous, coarser version protected a whole media type
from any cascading the moment ONE of its two fields was present in the
request, which is safe for the single-author PUT's echo scenarios (a
GET-modify-PUT client sends both fields together, unchanged) but turns
out to be a no-op for the actual bulk editor UI's own payload: it
always sends MonitorExisting when changing bulk monitoring but never
MonitorFuture, so the old logic saw MonitorExisting present, protected
the whole media type, and left MonitorFuture (and therefore
IsMonitoredFromMediaSettings()) exactly where it was - the reported bug,
reachable through Chaptarr's own UI.
Per-field protection on its own would still force MonitorFuture=true
whenever an untouched-and-turning-on media type's MonitorExisting was
being set, even when Existing alone already satisfies "monitored" - the
bulk editor's own "Monitored" (on) action sends exactly that shape
(MonitorExisting only, never MonitorFuture), and forcing Future there
would silently flip on "monitor new releases" for a preference the
request never touched. Turning OFF doesn't have a symmetric escape:
MonitorExisting > 0 alone keeps the author monitored even with Future
false, so both fields genuinely need clearing there. So the two
directions are asymmetric on purpose: turning off always cascades
Future (and Existing, when untouched); turning on only cascades Future
when Existing (as this request leaves it) doesn't already make the
media type monitored on its own.
This also changes single-author semantics in one narrow,
unlikely-in-practice case, unrelated to the asymmetry above: an
explicit edit to only one of a media type's two fields, in a direction
that genuinely contradicts the top-level flag (e.g. monitored:false
sent alongside an explicit MonitorFuture:true), now lets the untouched
field (MonitorExisting) cascade instead of protecting the whole media
type. I think that's the more defensible reading of "per field" as a
design principle, applied consistently, but it's a real, deliberate
behavior change from the four-review-round single-author fix this
builds on.
New tests in AuthorEditorControllerMonitoredCascadeFixture reuse the
AuthorServiceProxy/RootFolderServiceProxy/RecordingCommandQueue/
TestQualityProfileService/TestMetadataProfileService doubles already
established in AuthorEditorMissingRootHydrationFixture, rather than
hand-rolling weaker ones - notably, this fixture's AuthorServiceProxy
applies the real Monitored recompute in UpdateAuthors (the sibling
fixture's doesn't need to for its own purposes), since that recompute
is the actual bug being guarded against; a stub that skipped it could
only prove the cascade wrote plausible tri-state fields, not that
persisting monitored:false (or :true) through this endpoint actually
works. Covers: the exact bulk-editor-UI payload shape for both
directions (on and off) end to end, independent correctness across two
authors with different prior monitoring states in one bulk request, a
genuine per-field edit not being overridden, monitored omitted
entirely, a media type gaining a root folder in the same bulk request
(the one case whose correctness depends on the cascade running after
this author's field writes in the loop, not before), and the
Readarr-facade skip.
Known gaps, not addressed here:
- This covers the bulk editor's "All media types" mode. Its
media-type-scoped modes (Audiobook only / Ebook only) build a
different payload that never includes "monitored" at all, so
monitored:false still won't persist through those - a frontend gap,
not something this backend fix can reach.
- The cascade skips a media type with no root folder entirely, but
AuthorService's recompute (IsMonitoredFromMediaSettings) doesn't - it
reads all four tri-state fields regardless of whether either media
type actually has anywhere to look. So a stale MonitorFuture=true
left over on a never-configured media type can still keep an author
"monitored" after an explicit monitored:false, if that field was
written by some other path (e.g. the bulk editor's separate "Monitor
New Items" action, which isn't gated on root folder configuration
either). Pre-existing behavior, not introduced by this PR, and out of
scope for the same reason Chaptarr#88 is: fixing it means changing shared
monitoring-status logic well beyond what a single PUT endpoint owns.
- A Readarr-facade-scoped bulk request still can't persist
monitored:false - the facade guard above makes that a deliberate
no-op (there's no per-media-type ToModel equivalent for this endpoint
to fall back on the way the single-author PUT path has).
This was the second of the two gaps left open in the original PR
description; the first (a media type losing its root folder in the
same request that also asks to turn monitoring on) remains open and is
filed separately as Chaptarr#88, since it's entangled with a separate,
pre-existing ApplyChanges issue the bulk editor's endpoint doesn't
share (see the PR description for detail on why that one isn't safely
fixable as a small, contained change).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the "PUT monitored:false does not persist" sub-bug from #17. (The V5-matching / multi-pocket part of that issue is a metadata-server-side data quality problem, not something fixable client-side, and is left open.)
AuthorService.UpdateAuthor/UpdateAuthorsrecompute the legacyAuthor.Monitoredflag from the per-media-type tri-state settings on every save (author.Monitored = author.IsMonitoredFromMediaSettings()), and separately fill any still-null tri-state field from the root folder's defaults before that recompute runs. So a PUT that only sets the top-levelmonitoredfield - without touching any of the four tri-state fields - has it silently discarded: whatever those fields already were (or get defaulted to) wins, and the recompute overwrites the requested value right back before it's ever persisted.monitored:true"works" only when it happens to already match the derived value;monitored:falsenever sticks once any tri-state field for a configured media type is already on.(Note:
AuthorController's own validators requirePathand at least one quality profile id on every PUT regardless of this fix, so the minimal request that actually reaches this code still has to carry those - it isn't as bare as{id, monitored:false}.)What changed
AuthorResource.Monitoredis nowbool?instead ofbool(matching the existingbool?onAuthorEditorResource), since a non-nullable bool can't distinguish "client omitted this field" from "client explicitly sent false." Every other read of the field preserves its old omitted-means-false behavior, so this isn't a behavior change for any existing caller, including the Readarr-facade write path (covered by two new tests).AuthorController.UpdateAuthorcascades an explicitMonitoredvalue intoMonitorFuture/MonitorExistingfor whichever media types the author is configured for. A non-nullable bool alone isn't enough to know intent though: almost every real client - Chaptarr's own UI included, and any script that does GET → flip one field → PUT the whole object back - sends a full object on every PUT, so a stalemonitoredand stale (unchanged) tri-state field values both ride along on saves that have nothing to do with them. The cascade only treats a field as "touched" when its sent value actually differs from what was stored - by field, not by media type (see below) - so a same-value echo of either kind is always a no-op. The cascade fires per media type when the requested top-level value genuinely differs from what's already true for the author overall, or when that media type is gaining a root folder in this exact request (the one case where "nothing changed" isn't safe to skip).The bulk author editor (
PUT /api/v1/author/editor,AuthorEditorController.SaveAll) has the identical underlying recompute bug (AuthorService.UpdateAuthors) and now gets the same cascade, skipped under a Readarr-facade request the same way the single-author path is. The shared cascade logic was refactored into a thinAuthorResource-shaped wrapper over a primitive-parameter core so both controllers share it without duplicating it.Touched-detection is per FIELD, not per media type. Chaptarr's own bulk editor UI always sends
MonitorExistingwhen changing bulk monitoring but neverMonitorFuture- an earlier version of this fix protected a whole media type from cascading the moment either field was present, which made it a no-op for that exact payload. Turning off always needs both fields cleared (MonitorExisting > 0alone keeps an author monitored even withMonitorFuturefalse); turning on only forcesMonitorFuturewhenMonitorExisting(as the request leaves it) doesn't already satisfy "monitored" on its own - otherwise it would silently flip on "monitor new releases" for a preference nobody touched. This asymmetry was caught in review and is now covered by dedicated tests for both directions.Root folder paths, the four tri-state field values, and the author's overall derived-monitored status are all captured from the stored author - into one
StoredAuthorMonitoringStatevalue, in one line - beforeToModel/ApplyChanges(or the bulk editor's own field writes) mutate that object in place.Also updates
openapi.json'sAuthorResource.monitoredschema tonullable: true.Known gaps, left out of this PR
monitored:trueisn't guarded (single-author path only - the bulk editor has no mechanism to clear a root folder at all). This is downstream of a broader, pre-existing issue:Author.ApplyChangescopies root folder paths unconditionally, including tonullon simple omission, and that ambiguity is load-bearing elsewhere (BookController's facade normalization relies on it). Filed separately as PUT /api/v1/author/{id} silently clears root folder paths (and Path) when a request omits them #88, since it's a real, independently-reachable bug beyond this PR's scope.monitoredat all, somonitored:falsestill won't persist through those - a frontend gap, not something this backend fix can reach.IsMonitoredFromMediaSettings) doesn't - it reads all four tri-state fields regardless of whether either media type has a root folder. A staleMonitorFuture=trueleft on a never-configured media type (writable via the bulk editor's separate "Monitor New Items" action, which isn't root-folder-gated either) can still keep an author "monitored" after an explicitmonitored:false. Pre-existing, not introduced here, and out of scope for the same reason as PUT /api/v1/author/{id} silently clears root folder paths (and Path) when a request omits them #88.monitored:false- the facade guard makes that a deliberate no-op, since there's no per-media-typeToModelequivalent for this endpoint to fall back on.Test plan
AuthorControllerMonitoredCascadeFixture(15 cases): the reported bug, the GET-modify-PUT echo case, the UI-toggle stale-echo case, the newly-added-root-folder case, the turn-on-doesn't-force-future-when-existing-satisfies-it case, facade pass-through, and an end-to-end seam test through the realToModelcallAuthorEditorControllerMonitoredCascadeFixture(7 cases), built against the realAuthorEditorController.SaveAllend to end (reusing this project's existingAuthorServiceProxy/RootFolderServiceProxy/RecordingCommandQueuetest doubles, with a realMonitoredrecompute added so the tests assert on the actual outcome, not just the cascade's direct writes): the exact bulk-editor-UI payload shape for both monitored on and off, independent correctness across two authors with different prior states in one bulk request, a genuine per-field edit not being overridden,monitoredomitted, a media type gaining a root folder in the same bulk request, and the Readarr-facade skipAuthorResourceMapperFixtureconfirming the facade write path's omitted-Monitored-defaults-to-false behavior is unchanged