fix!: correct endpoints, auth and paths that the live APIs reject - #213
Open
marksie1988 wants to merge 9 commits into
Open
fix!: correct endpoints, auth and paths that the live APIs reject#213marksie1988 wants to merge 9 commits into
marksie1988 wants to merge 9 commits into
Conversation
…paths
Appending "/{item_id}" to a path that already ended in a slash produced a double
slash, so every Dispatcharr detail route was requested as
"channels/channels//1". Dispatcharr answers any unmatched path with its single
page application's HTML at HTTP 200 rather than a 404, so these calls returned a
web page instead of data and never raised.
_detail_path now preserves whichever convention the collection path uses:
Servarr keeps "series/1", Dispatcharr's Django routes get
"channels/channels/1/".
Verified against a live Dispatcharr 0.29.0 container.
Config was attached as .config on the Sonarr, Radarr, Lidarr and Readarr
clients, but each class held nothing but a constructor, so the whole config/*
family was unreachable. Prowlarr had no config component at all. The web
interface loads config/ui and config/host on every single page, and every
settings page reads and writes this family.
The tiering follows what each application actually serves, taken from the live
routing tables:
Config host, ui, downloadclient all six Servarr clients
MediaConfig indexer, mediamanagement, naming the five media clients
naming/examples
Sections only some applications serve stay on the per-application subclasses:
importlist for Sonarr and Radarr, metadata for Radarr, metadataProvider for
Lidarr and Readarr, development for Readarr and Prowlarr.
Every getter and setter was round-tripped against live containers, reading a
section, writing it back unchanged and confirming the value was untouched.
…lients
Two client construction concerns, both driven by what the live servers accept.
RequestHandler gained an auth_scheme argument. It still defaults to "apikey",
sending X-Api-Key as before, but "bearer" now sends Authorization: Bearer.
Dispatcharr issues JWT access tokens and rejects them in X-Api-Key with
{"detail":"Invalid API key"}, which left 62 of its 67 read methods unusable.
System is split so each client only advertises what it serves. Prowlarr manages
no media and has no diskspace route, so prowlarr.system.get_diskspace() raised
PyarrResourceNotFound on every call. Everything else on System is served by all
six Servarr applications and stays shared. diskspace moves to MediaSystem,
attached by MediaArrClient.
BREAKING CHANGE: Prowlarr.system.get_diskspace() is removed. Prowlarr never
served /api/v1/diskspace, so the call could only ever fail. The other System
methods are unchanged for every client.
The client was assembled from the Radarr components, but Whisparr V2 ("Eros")
forks Sonarr v3, not Radarr. Its library primitives are series, shown as Sites
in the web interface, and episode, shown as Scenes. There is no movie concept
anywhere in the application: /api/v3/movie, /movie/lookup, /movie/editor and
/moviefile all return an unrouted 404, confirmed against Whisparr 2.2.0.108 and
absent from its routing table in every verb.
That left the entire media management half of the client aimed at endpoints the
application does not serve, and whisparr.config was an empty class.
Series, Episode, EpisodeFile, Release and ManualImport already existed under
_async/sonarr and match this build's paths and parameter names, so the fix is
largely a swap. Every path those components use was checked against Whisparr's
live routing table first, languageprofile included.
BREAKING CHANGE: whisparr.movie and whisparr.movie_file are removed, as neither
resource exists on Whisparr. Use whisparr.series, whisparr.episode and
whisparr.episode_file instead. whisparr.release and whisparr.manual_import now
take seriesId and episodeId rather than movieId, which the server ignored.
Bazarr is a Flask application, not a Servarr one, and answers any unmatched
/api/ path with its single page application's HTML at HTTP 200 rather than a
404. Several inherited methods were therefore returning a web page wrapped in
{"message": "<!doctype html>..."} without ever raising. The same trap was
already noted in a comment about subtitles/wanted.
system.get_health() GET health -> now system/health
system.get_task() GET system/task -> now system/tasks
system.request_restart() POST system/restart -> now POST system?action=
system.request_shutdown() POST system/shutdown
Restart and shutdown also close the connection instead of answering, so a
dropped connection is expected and is swallowed. A ConnectError still raises,
so an unreachable instance is not hidden.
Subtitles are downloaded and deleted through the media they belong to, so those
actions move to the episodes and movies components, matching both the server's
URL layout and the existing component structure.
Wanted paged with the Servarr page and pageSize parameters. Bazarr declares only
start, length and the id list, and Flask-RestX silently drops anything else, so
paging had no effect at all.
BREAKING CHANGE: bazarr.subtitles.download() and bazarr.subtitles.delete() are
removed; there is no subtitles/{id} route. Use
bazarr.episodes.download_subtitle(), bazarr.episodes.delete_subtitle(),
bazarr.movies.download_subtitle() and bazarr.movies.delete_subtitle().
bazarr.system.get_diskspace(), get_routes(), get_routes_duplicate() and
get_task() are removed; Bazarr serves none of them, and get_task is replaced by
get_tasks(). bazarr.wanted_episodes.get() and wanted_movies.get() now take
start and length rather than page, page_size, sort_key and sort_dir.
…ng urls
Every fix here is a path or header the running server rejected. Dispatcharr
answers unmatched paths with its single page application's HTML at HTTP 200, so
none of these failures raised.
Auth: the client now defaults to bearer, because Dispatcharr issues JWT access
tokens from accounts/token/ and rejects them in X-Api-Key. 62 of 67 read methods
returned {"detail":"Invalid API key"}; 53 now succeed and the remainder are
genuine record-not-found responses against an empty instance.
Proxy: Dispatcharr serves these views from the server root, not under /api.
GET /api/proxy/ts/status returned HTML, GET /proxy/ts/status returns JSON. The
component now escapes the api prefix with ../ the way live.py already did, and
matches the schema's trailing slashes exactly, which differ per route.
HDHR: device.xml, discover.json, lineup.json and lineup_status.json are not
router routes and reject a trailing slash. hdhr/devices/ is one and keeps it.
m3u.refresh_account_info the profile id belongs in the path
channel_profiles the channel id segment was missing
plugins.delete needs the /delete/ action route
BREAKING CHANGE: dispatcharr defaults to auth_scheme="bearer". Pass
auth_scheme="apikey" when authenticating with a generated API key rather than a
JWT. m3u.refresh_account_info() now takes profile_id as its first argument.
channel_profiles.partial_update_channels() is renamed to
partial_update_channel() and takes a channel_id, since the route acts on one
channel rather than all of them.
test_request_restart genuinely restarts the live application, which then goes down for several seconds and sometimes fails to rebind its port entirely, dying with "Failed to bind to address: address already in use". Every test running afterwards against that application then fails. Radarr failed deterministically because it is the only directory with a test file after test_system.py alphabetically. Sonarr and Readarr were doing the same damage with nothing behind them to notice. The --reruns 3 --reruns-delay 5 in the nox session masked it whenever the application happened to recover inside five seconds. These are now marked destructive and deselected by default. Run them deliberately with `pytest -m destructive`, where nothing follows them. Restarting the application under test cannot be made safe inside a shared fixture run: ordering it last still leaves the application down, and the rebind failure is a race no delay reliably covers. The request shape is covered instead by mocked tests that assert both calls POST to their own endpoints.
Reviewer's GuideAligns pyarr’s async/sync clients and shared components with what the live Servarr, Bazarr, Whisparr, and Dispatcharr APIs actually serve: fixes path construction and auth handling, introduces tiered Config/System abstractions, adds Bazarr- and Dispatcharr-specific components, switches Whisparr over to Sonarr-based primitives, and extends the test suite (including a Sequence diagram for Dispatcharr authentication and corrected routingsequenceDiagram
participant Client
participant RequestHandler
participant Dispatcharr
Client->>RequestHandler: request(endpoint)
RequestHandler->>Dispatcharr: Authorization: Bearer api_key
Dispatcharr-->>RequestHandler: JSON response
RequestHandler-->>Client: parsed response
Client->>RequestHandler: get_ts_status(channel_id)
RequestHandler->>Dispatcharr: GET ../proxy/ts/status/channel_id
Dispatcharr-->>RequestHandler: proxy status JSON
RequestHandler-->>Client: parsed response
Sequence diagram for Bazarr system actionssequenceDiagram
participant Client
participant BazarrSystem
participant Bazarr
Client->>BazarrSystem: get_health()
BazarrSystem->>Bazarr: GET system/health
Bazarr-->>BazarrSystem: health JSON
BazarrSystem-->>Client: health JSON
Client->>BazarrSystem: request_restart()
BazarrSystem->>Bazarr: POST system action=restart
Bazarr-->>BazarrSystem: connection dropped while restarting
BazarrSystem-->>Client: return cleanly
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pyarr/_async/common/base.py" line_range="34" />
<code_context>
+ Returns:
+ str: The endpoint to request.
+ """
+ if not item_id:
+ return path
+ if path.endswith("/"):
+ return f"{path}{item_id}/"
+ return f"{path}/{item_id}"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `_detail_path` treats an item ID of `0` as absent and returns the collection path. `_delete` therefore sends `DELETE` to the collection instead of the requested detail resource when `item_id=0`, whereas the old `_delete` always appended the ID.
**Triggers:** When a valid resource has ID 0.
**Suggested fix:** Check `item_id is None` rather than its truthiness, while preserving the trailing-slash handling.
```suggestion
if item_id is None:
```
</issue_to_address>
### Comment 2
<location path="src/pyarr/_async/whisparr/__init__.py" line_range="57-61" />
<code_context>
headers=headers,
)
- self.config = Config(self.http_utils)
- self.movie = Movie(self.http_utils)
- self.movie_file = MovieFile(self.http_utils)
+ self.config = MediaConfig(self.http_utils)
+ self.series = Series(self.http_utils)
+ self.episode = Episode(self.http_utils)
+ self.episode_file = EpisodeFile(self.http_utils)
</code_context>
<issue_to_address>
**issue (broader_impact):** The Whisparr client removes `movie` and `movie_file`, but the existing `tests/integration/test_whisparr.py` still calls `whisparr_client.movie.get()`. Running that integration test now raises `AttributeError` before making a request.
**Triggers:** When the existing Whisparr integration tests are included in the test run.
**Suggested fix:** Update the existing Whisparr integration test to exercise `series`/`episode` resources, or explicitly migrate/remove the obsolete test as part of the breaking change.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and a wrong default authentication scheme can deny every Dispatcharr request or send credentials through the wrong trust boundary, and the new action and deletion endpoints can restart or shut down an instance and remove subtitle or plugin files when called. Reverting restores the prior client behavior, but it cannot undo an outage or deletion that already occurred, and the authentication default is itself a policy decision affecting all users immediately.
Blocking findings: src/pyarr/_async/common/base.py:34, src/pyarr/_async/whisparr/__init__.py:61
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 47.56% 56.40% +8.83%
==========================================
Files 164 182 +18
Lines 4457 5062 +605
==========================================
+ Hits 2120 2855 +735
+ Misses 2337 2207 -130 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
_detail_path tested the item id for truthiness, so an id of 0 was read as "no id" and returned the collection path. The previous _delete always appended the id, so this was a regression: _delete(path, 0) sent DELETE to the collection instead of to item 0, which is a very different request. Only None means "no item", which matches every caller's signature. Reported by Sourcery on #213.
The integration test still called whisparr_client.movie.get(), which no longer exists now the client is built from the Sonarr components. It kept passing because the body was wrapped in a bare `except Exception: pass`, so the AttributeError was swallowed along with everything else. That is exactly how the call survived the change unnoticed, so the replacement asserts directly. Reported by Sourcery on #213.
Collaborator
Author
|
Both findings were valid and are fixed in 6dac575 and 11d719b. 1. 2. Obsolete Whisparr integration test - correct that it needed migrating, though the stated failure mode does not occur: the body is wrapped in Worth flagging that I had been running the suite with |
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.
What this fixes
Eight *arr applications were run in Docker, driven through their own web interfaces, and their real API traffic recorded. Every endpoint pyarr targets was then checked against the running server. This corrects the 25 implementations proven wrong, plus one defect class found while fixing them.
Nothing here is inferred from documentation or upstream specs. Every claim below was settled by a live observation; anything that could not be was left alone rather than guessed.
Why these were easy to miss
Three of the eight applications answer an unmatched
/api/path with their single page application's HTML at HTTP 200 rather than a 404. A wrong path therefore returned a web page that pyarr handed back as data, and nothing raised.bazarr.system.get_health()was returning{"message": "<!doctype html>..."}.The fixes
common/base.py_get/_deleteappended/{item_id}to paths already ending in/, so every Dispatcharr detail route was requested aschannels/channels//1GET .../channels//1returnedtext/htmlat 200common/config.pyConfigwas attached as.configon four clients but held only a constructor, so all ofconfig/*was unreachable. Prowlarr had none at allconfig/uiandconfig/hostload on every page of every appcommon/system.pyprowlarr.system.get_diskspace()raisedPyarrResourceNotFoundon every callwhisparr//api/v3/movie,/movie/lookup,/movie/editor,/moviefileall return an unrouted 404bazarr/get_health,get_task, restart and shutdown all targeted Servarr paths Bazarr does not serve; subtitle actions targeted a nonexistentsubtitles/{id};wantedpaged with parameters Bazarr silently dropsdispatcharr/X-Api-Key; wholeproxycomponent mounted under/apiwhere it is served from the root; four HDHR routes broken by a trailing slash; three malformed URLs{"detail":"Invalid API key"}Shared components kept shared
The tiering follows what each application actually serves, taken from the live routing tables rather than assumed:
Config(host, ui, downloadclient) for all six Servarr clients,MediaConfig(indexer, mediamanagement, naming, naming/examples) for the five media clients. App-specific sections stay on the per-app subclasses.Systemstays shared by all six; onlydiskspacemoves down toMediaSystem.common/wanted.pyis untouched for Servarr; Bazarr gets its own paging._async/sonarr/*rather than duplicating it.Verification
All 62 changed or added methods were executed against live containers:
PUTit back, read again, confirm unchanged. 17 section/app combinations, all HTTP 202.BazarrSystem.request_restart()proven by effect: the container log shows the restart, and it returns cleanly rather than raising.Coverage by client afterwards: Prowlarr 27/27 read methods, Dispatcharr 53/67 (up from 5/67; the remainder are genuine record-not-found on an empty database).
Versions tested: Sonarr 4.0.19, Radarr 6.3.0, Lidarr 3.1.0, Readarr 0.4.18, Prowlarr 2.5.2, Whisparr 2.2.0, Bazarr 1.6.0, Dispatcharr 0.29.0.
Test suite
532 passing, up from 446. The suite also had a pre-existing failure that is now fixed:
test_request_restartgenuinely restarted the live application, which then went down for several seconds and sometimes failed to rebind its port entirely, breaking everything after it. Radarr failed deterministically because it is the only directory with a test file aftertest_system.py; Sonarr and Readarr were doing the same damage unnoticed. Those tests are now markeddestructiveand deselected by default, runnable withpytest -m destructive, with the request shape covered by mocked tests instead.Breaking changes
Every removal below is a method that could only ever fail against the real server.
whisparr.movieandwhisparr.movie_fileremoved. Useseries,episode,episode_file.releaseandmanual_importnow takeseriesId/episodeId.prowlarr.system.get_diskspace()removed.bazarr.subtitles.download()/.delete()removed. Usebazarr.episodes.download_subtitle(),bazarr.movies.delete_subtitle(), and so on.bazarr.system.get_diskspace(),get_routes(),get_routes_duplicate(),get_task()removed.get_taskbecomesget_tasks().bazarr.wanted_episodes.get()/wanted_movies.get()takestart/lengthinstead ofpage/page_size/sort_key/sort_dir.Dispatcharrdefaults toauth_scheme="bearer". Passauth_scheme="apikey"for a generated API key.dispatcharr.m3u.refresh_account_info()takesprofile_idfirst.dispatcharr.channel_profiles.partial_update_channels()renamed topartial_update_channel()and takes achannel_id.Follow-ups not in this PR
--reruns 3 --reruns-delay 5in the nox session was masking the restart flake and no longer serves that purpose.addoptsapplies to the nox run. A finalpytest -m destructivestep would restore it safely.Summary by Sourcery
Align synchronous and asynchronous clients with the endpoints, authentication schemes, and resource models served by the supported applications.
New Features:
Bug Fixes:
Enhancements:
Tests: