fix(firmware): decode manifests independent of content type - #6400
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughApiService now uses shared lenient JSON configuration for firmware payloads, decodes release manifests from raw response text, and applies the configuration to nightly firmware responses. Common tests cover response content types and decoder edge cases. ChangesFirmware JSON decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ApiServiceImpl
participant FirmwareEndpoint
participant firmwareJson
ApiServiceImpl->>FirmwareEndpoint: Request firmware manifest
FirmwareEndpoint-->>ApiServiceImpl: Return response body as text
ApiServiceImpl->>firmwareJson: Decode FirmwareReleaseManifest
firmwareJson-->>ApiServiceImpl: Return decoded manifest
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Nice fix — reading One non-blocking note on the JSON config. The new isLenient = true
ignoreUnknownKeys = true
coerceInputValues = trueSince every field in Suggestion (low real-world likelihood, but a free robustness win): either inject the DI |
|
@jamesarich Good catch. You’re right that the explicit decoder currently narrows the tolerance provided by the shared I’ll align |
dcd689a to
72cce7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/network/src/commonTest/kotlin/org/meshtastic/core/network/service/ApiServiceTest.kt (1)
26-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the HTTP content-type regression path.
These tests call
decodeFirmwareReleaseManifestdirectly, so they would still pass ifgetFirmwareReleaseManifest()regressed to content-negotiated decoding. Add a service-level test returning valid JSON asapplication/octet-streamand exercise the release endpoint; also cover the nightly endpoint if it is part of this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/network/src/commonTest/kotlin/org/meshtastic/core/network/service/ApiServiceTest.kt` around lines 26 - 44, Add service-level tests alongside `ApiServiceTest` that configure the release endpoint to return valid firmware manifest JSON with `application/octet-stream`, then invoke `getFirmwareReleaseManifest()` and assert the decoded result. If the nightly endpoint is included in the implementation, add the equivalent test for `getNightlyFirmwareReleaseManifest()`; retain the direct decoder test for unknown-field handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/network/src/commonTest/kotlin/org/meshtastic/core/network/service/ApiServiceTest.kt`:
- Around line 26-44: Add service-level tests alongside `ApiServiceTest` that
configure the release endpoint to return valid firmware manifest JSON with
`application/octet-stream`, then invoke `getFirmwareReleaseManifest()` and
assert the decoded result. If the nightly endpoint is included in the
implementation, add the equivalent test for
`getNightlyFirmwareReleaseManifest()`; retain the direct decoder test for
unknown-field handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8de756f5-a577-4baa-854b-a374f42d15ca
📒 Files selected for processing (2)
core/network/src/commonMain/kotlin/org/meshtastic/core/network/service/ApiService.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/service/ApiServiceTest.kt
Head branch was pushed to by a user without write access
72cce7a to
3bdcfb5
Compare
|
I just added that last coderabbit nit test too. |
Pull request was converted to draft
GitHub release assets can serve valid firmware manifest JSON as application/octet-stream. Passing those responses through Ktor content negotiation raises NoTransformationFoundException even though the payload and HTTP status are valid. Read release manifests as text and decode them explicitly. Preserve the former shared JSON tolerance for lenient syntax, unknown fields, and explicit nulls on defaulted properties while continuing to propagate transport, HTTP, and malformed-payload failures. Cover forward-compatible fields, explicit-null coercion against the actual manifest model, malformed input, and the complete ApiService path with an octet-stream MockEngine response so content-negotiated decoding cannot regress unnoticed.
3bdcfb5 to
1ce27ef
Compare
2 participants
Overview
This makes firmware release manifest decoding independent of the HTTP response content type.
GitHub release assets can return valid JSON as
application/octet-stream. Passing those responses through Ktor content negotiation can produceNoTransformationFoundExceptioneven though the request succeeded and the response body contains a valid firmware manifest.The repository now reads the response body as text and decodes it explicitly with the firmware JSON configuration.
Key Changes
Testing
Migration Notes
Summary by CodeRabbit
nullvalues to safe defaults.