Repository navigation
fix(subsonic): an unsaved play queue sends no element at all - #259
Conversation
The schema marks `username`, `changed` and `changedBy` all three required on `PlayQueue`, so the bare `<playQueue/>` this answered before anything was saved had no valid form. The reference documents the omission rather than a shape — "an empty <subsonic-response> if no play queue has been saved" — and filling the element in would mean inventing a `changed` and a `changedBy` for something that was never changed. Navidrome takes a third route and sends the element with `username` alone, which is equally invalid and only exchanges one deviation for another. A client that copes with that copes with omission too: omission is what Subsonic and Airsonic answer, so a client that could not read it would already fail there. `Node::nothing()` is the per-answer half of `empty_success_method`, which is per method. An element name can never be empty, so the empty name is a sentinel no real node collides with. The pinned assertion is rewritten rather than deleted, and asserts the envelope beside the absence: an error response carries no `playQueue` either, and absence alone cannot tell a conforming answer from a broken one. Both halves were proved by inversion, each on its own. Closes #229 Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
scope: server
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk | ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
La description explique le comportement, la justification, les tests ciblés et la documentation modifiée. Elle ne suit toutefois pas la structure requise et ne fournit pas le plan de test avec les cas… | Ajouter les sections # Summary, ## Changes, ## Test plan et ## Notes. Détailler les changements sous forme de liste. Indiquer l’état des commandes requises (bun run typecheck, bun run lint, bun run format:check, `bun run build… |
| Docstring Coverage | |
Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (3 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Title check Le titre décrit clairement le changement principal : getPlayQueuen’envoie plus d’élément pour une file non enregistrée.Linked Issues check L’issue active directement liée #229demande l’omission deplayQueueavant toute sauvegarde et la mise à jour du test contractuel.get_queuerenvoieNode::nothing()dans ce cas.handle_innerp…Out of Scope Changes check Les changements restent liés à #229. Le mécanismeNode::nothing()permet l’omission par réponse. Le helpersubsonic_xmlet les assertions JSON/XML valident le changement de format. RFC-002 et les …
Full details: Description check
-
Explanation
La description explique le comportement, la justification, les tests ciblés et la documentation modifiée. Elle ne suit toutefois pas la structure requise et ne fournit pas le plan de test avec les cases prévues.
Resolution
Ajouter les sections
# Summary,## Changes,## Test planet## Notes. Détailler les changements sous forme de liste. Indiquer l’état des commandes requises (bun run typecheck,bun run lint,bun run format:check,bun run build,bun run test) et le test manuel, ou supprimer la sectionNotessi elle est inutile. Conserver la confirmation AGPL-3.0-only et DCO du modèle de dépôt, si elle est requise par le processus.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (3 skipped: 3 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
getPlayQueueanswered a bare<playQueue/>before anything had been saved.The 1.16.1 schema marks
username,changedandchangedByall threeuse="required"onPlayQueue, so that element has no valid form for aqueue that was never written — and the populated answer, which carries all
three since #225, disagreed with it as well as with the schema.
What it answers now
Which is what the reference documents: "or an empty
<subsonic-response>ifno play queue has been saved".
<xs:choice minOccurs="0">onResponsemakesthe omission valid; no non-empty shape is.
Why not fill it in
That was the other way out, and it is the worse one:
changedandchangedBywould have to be invented for something that was never changed.
Navidrome takes a third route — it emits the element carrying
usernamealone — which is equally invalid against the schema and only exchanges one
deviation for another. It is worth naming because it sets the client risk: a
client that copes with Navidrome's shape copes with omission too, since
omission is what Subsonic and Airsonic answer. One that could not read an
absent
playQueuewould already fail against both.How it is done
Node::nothing()is the per-answer half ofempty_success_method, whichis per method: some methods have nothing to say only sometimes. An element name
can never be empty, so the empty name is a sentinel no real node collides with.
The pinned assertion
play_queue_and_now_playing_name_their_children_entrypinned the old shape onpurpose, so that it could not change unnoticed. It is rewritten rather than
deleted, and now asserts the envelope beside the absence — an error
response carries no
playQueueeither, and absence alone cannot tell aconforming answer from a broken one.
Both halves were proved by inversion, each on its own: with the fix
reverted, the JSON assertion fails on
{"playQueue":{}}and the XML assertion,run with the JSON one neutralised, fails on
<playQueue/>. The XML half is notdecoration — an element present but empty reads as absent to a JSON-only test.
subsonic_xmljoinssubsonic_jsonintests/support/, and the populatedhalf of the same test now goes through it instead of building its own request.
Documentation
Navidrome comparison. Like the other three, it requires the compatibility
matrix to be re-run before the next tag.
docs/subsonic-compatibility.md— a note saying the rows below were rightwhen written, and naming what to check: DSub and Juliet are the two clients
in the matrix that call the method.
docs/subsonic-api-guide.md— the queue row says what an unsaved queueanswers.
No
CHANGELOG.mdentry: that file is written at release time, last touched bythe
2.0.0-beta.1release PR.Closes #229
Summary by CodeRabbit
getPlayQueuerenvoie désormais une réponse réussie sans élémentplayQueue, en XML comme en JSON. Les réponses incluent toujours les informations de la file lorsqu’elle existe.getPlayQueuesans file enregistrée et signale que la compatibilité avec DSub et Juliet reste à vérifier.