[rest] add REST JSON:API for commissioner and diagnostics#2514
[rest] add REST JSON:API for commissioner and diagnostics#2514jwhui merged 1 commit intoopenthread:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2514 +/- ##
==========================================
- Coverage 55.77% 46.64% -9.13%
==========================================
Files 87 142 +55
Lines 6890 17175 +10285
Branches 0 1413 +1413
==========================================
+ Hits 3843 8012 +4169
- Misses 3047 8613 +5566
- Partials 0 550 +550 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Happy to discuss thoughts and improvements. Note: we will add a commit fixing the broken topology browser in otbr-web service asap. |
|
It looks #2515 is also adding the REST commissioner API. |
@martinzi Thanks for this contribution! But this PR is about 12K lines of change that it's too hard to review. Could you help split this PR by creating a separate smaller PR for each feature? For example, one PR for commissioner API and another PR for network diagnostic. |
Agree, it became quite large. See a first part split into PR2517. More to follow ... |
martinzi
force-pushed
the
rest-jsonapi
branch
5 times, most recently
from
5627bd7 to
1d140d7
Compare
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an extensive REST JSON:API for commissioner and diagnostics functionalities. It adds new C++ classes for managing actions, devices, and diagnostics collections, along with a commissioner manager and network diagnostic handler. The changes also include a comprehensive README for the new API, updates to the frontend web UI to utilize these new endpoints, and a new set of Bruno tests for validating the API. Key improvements include a more structured approach to handling asynchronous tasks via an actions queue, detailed device inventory, and enhanced diagnostic capabilities. The JSON key naming has been largely standardized to camelCase for better consistency. Overall, this is a significant feature addition that greatly expands the REST API's capabilities.
There was a problem hiding this comment.
Code Review
This pull request introduces a REST JSON:API for commissioner and diagnostics, guided by the JSON:API specification. It includes new REST resources for devices, diagnostics, and actions, along with corresponding data structures and handlers. The changes also involve adding new files for actions, diagnostic types, and JSON handling, as well as updating existing files to incorporate the new functionality. The code appears well-structured and follows the JSON:API specification. The review comments suggest minor improvements related to code readability, efficiency, and compatibility.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds JSON:API–compliant REST endpoints for commissioning and on-mesh diagnostics and integrates end-to-end tests and frontend updates.
- Enable mesh diagnostics in the OpenThread build
- Add Bruno CLI integration tests under
tests/restjsonapifor the new/apiroutes - Implement JSON:API handlers for actions, devices, and diagnostics in
src/rest, plus corresponding frontend and legacy-test updates
Reviewed Changes
Copilot reviewed 95 out of 96 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/openthread/CMakeLists.txt | Enable OT_MESH_DIAG option for mesh diagnostics |
| tests/restjsonapi/* | Add Bruno CLI–based JSON:API integration tests for devices, actions, and diagnostics |
| tests/rest/test_rest.py | Update legacy Python REST tests to expect JSON and lowercase hex values |
| src/web/web-service/frontend/res/js/app.js | Refactor UI to call new /api endpoints, add topology slider and TLV checkboxes |
| src/web/web-service/frontend/.../styles.css/html | Adjust styling and bindings to match new API data shapes |
| src/rest/types.hpp, services.hpp, rest_web_server.cpp, timestamp.cpp, uuid.* | Implement core JSON:API request parsing, routing, services, and timestamp formatting |
There was a problem hiding this comment.
Code Review
This pull request introduces a REST JSON:API for commissioning and diagnostics. I've focused on correctness and robustness, identifying areas where the implementation could be improved, particularly in handling joiner requests and parsing request URLs.
| @@ -0,0 +1,17 @@ | |||
| meta { | |||
There was a problem hiding this comment.
should a license header be added?
There was a problem hiding this comment.
oh, it seems bru files do not have a concept for such header text.
martinzi
force-pushed
the
rest-jsonapi
branch
2 times, most recently
from
122dc78 to
5ef6f4a
Compare
martinzi
force-pushed
the
rest-jsonapi
branch
2 times, most recently
from
e8b706d to
3abe24f
Compare
| @@ -0,0 +1,497 @@ | |||
| # Add REST JSON:API for commissioner and diagnostics | |||
There was a problem hiding this comment.
Yes I will get my review in by tomorrow if that's ok
Vyrastas
left a comment
Vyrastas
left a comment
There was a problem hiding this comment.
Sorry for the delay in review.
Good job on the README! Most of my comments are nits / copyedits.
martinzi
force-pushed
the
rest-jsonapi
branch
from
d5d7159 to
032ea79
Compare
martinzi
force-pushed
the
rest-jsonapi
branch
from
032ea79 to
6d935eb
Compare
…o generic off-mesh clients
This commit implements following parts:
- api/node
- api/devices
- api/actions
. addThreadDeviceTask
. getNetworkDiagnosticsTask
. resetNetworkDiagCounterTask
. getEnergyScanTask
. updateDeviceCollectionTask
- api/diagnostics
. networkDiagnostic
. energyScanReport
The commit also provides integration tests, see tests/restjsonapi.
For more details see README.md and openapi.yaml in src/rest.
squashed following former additional commits:
- fix(rest): fix carve-out from PR2514 and re-add diagnostic endpoint
- disable otBorderAgentState field causing build error in github actions
- update openapi.yaml
- fix(rest): delay test progress until node is attached
- fix(rest): return filtered node attributes
- feat(rest): add discerner joiner
- docs(rest): minor doc fixes
- Update src/rest/actions_list.cpp
- Update src/rest/actions/reset_diagnostic_counters.hpp, Fix typo in header guard
- fix(rest): use std::array to provide compile-time size information
- style(rest): change class name from UUID to Uuid
- fix(rest): byte order in SetServiceRoleFlags, set batteryLevel and supplyVoltage TLVs as omitable
- refact(rest): refactored for cpp-httplib
- fix: prevent incorrect timezone offset produced from offset in seconds rather than absolute POSIX time
- test(rest): let test script fail fast
- fix(rest): check for duplicated joiners
- fix(rest): action API by reference
- fix(rest): add sparsefields for api/actions
- refact(rest): use cpp-httplib path parser to get itemId
- fix(rest): cleanup extract queries
- test(rest): run cicd tests
- fix(rest): get border agent state
martinzi
force-pushed
the
rest-jsonapi
branch
from
6d935eb to
46d7e3a
Compare
jwhui
changed the title
In this PR, we propose an extended
RESTAPI functionality providing capabilities for commissioning and on-mesh diagnostics to generic off-mesh http clients. The implementation is guided by the JSON:API specification and provides following partsapi/node- pointing to api/devices/thisdeviceapi/devices- a collection of discovered devicesapi/actions- indirect processing of tasks, in particular for those that may need more timeaddThreadDeviceTask- starts the on-mesh commissioner and adds the joiner candidate into the joiner tablegetNetworkDiagnosticTask- sends diagnostic requests and diagnostic queries to the destinationresetNetworkDiagCounterTask- resets the network diagnostic mle and/or mac countergetEnergyScanTask- starts the commissioner and sends energy scan requests to the destinationupdateDeviceCollectionTask- (re-)discovers the devices in the Thread network and updates the collection in api/devicesapi/diagnosticsThe PR provides integration tests for local validation, see tests/restjsonapi.
The Readme provides more details and descriptions in src/rest/README.md, also see openapi specification.
Related to specification item SPEC-1349