refactor: accept expanded deviceInfo fields in v1 devices#1021
refactor: accept expanded deviceInfo fields in v1 devices#1021ShradhaGupta31 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
==========================================
+ Coverage 41.46% 41.67% +0.20%
==========================================
Files 134 135 +1
Lines 12332 12403 +71
==========================================
+ Hits 5114 5169 +55
- Misses 6672 6680 +8
- Partials 546 554 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Update the v1 devices POST/GET flow to accomodate the full deviceInfo payload Adresses - #1020 Signed-off-by: ShradhaGupta31 <shradha.gupta@intel.com>
There was a problem hiding this comment.
Pull request overview
Updates the v1 /api/v1/devices DTO/usecase flow to accept and persist an expanded deviceInfo payload (including unknown/extra fields), supporting the Device Discovery requirements in #1020.
Changes:
- Persist
deviceInfoby JSON-marshallingdto.DeviceInfointoentity.Device.DeviceInfo(string) and unmarshalling it back on reads. - Expand
dto.DeviceInfowith additional fields and custom JSON (un)marshal logic to round-trip unknown fields viaExtraFields. - Add/adjust unit tests and update the v1 devices Postman request example to include the expanded
deviceInfo.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/usecase/devices/usecase.go | Marshals/unmarshals deviceInfo between DTO and entity; allows partial-update setter for deviceInfo. |
| internal/usecase/devices/repo_test.go | Updates partial-update tests to include deviceInfo persistence/round-trip expectations. |
| internal/entity/dto/v1/device.go | Expands DeviceInfo and adds custom JSON round-tripping for unknown fields (ExtraFields). |
| internal/entity/dto/v1/device_test.go | Adds a JSON round-trip test for DeviceInfo, including ExtraFields. |
| internal/controller/httpapi/v1/devices_test.go | Adds controller-level test ensuring POST accepts full deviceInfo payload. |
| integration-test/collections/console_mps_apis.postman_collection.json | Updates Postman sample request body to include full deviceInfo. |
| var info dto.DeviceInfo | ||
| if err := json.Unmarshal([]byte(raw), &info); err != nil { | ||
| return nil | ||
| } |
| "lastconnected": func(dst, src *dto.Device) { dst.LastConnected = src.LastConnected }, | ||
| "lastseen": func(dst, src *dto.Device) { dst.LastSeen = src.LastSeen }, | ||
| "lastdisconnected": func(dst, src *dto.Device) { dst.LastDisconnected = src.LastDisconnected }, | ||
| "deviceinfo": func(dst, src *dto.Device) { dst.DeviceInfo = src.DeviceInfo }, | ||
| "username": func(dst, src *dto.Device) { dst.Username = src.Username }, |
| LastUpdated time.Time `json:"lastUpdated"` | ||
| TLSMode string `json:"tlsMode"` | ||
| UPID map[string]any `json:"upid,omitempty"` | ||
| AMTEnabledInBIOS *bool `json:"amtEnabledInBIOS,omitempty"` | ||
| MEInterfaceVersion string `json:"meInterfaceVersion"` | ||
| DHCPEnabled *bool `json:"dhcpEnabled,omitempty"` | ||
| CertHashes []string `json:"certHashes,omitempty"` | ||
| LMSInstalled *bool `json:"lmsInstalled,omitempty"` | ||
| LMSVersion string `json:"lmsVersion"` | ||
| OSName string `json:"osName"` |
| return err | ||
| } | ||
|
|
||
| delete(raw, "fwVersion") |
There was a problem hiding this comment.
Drop the catch-all entirely. Unknown fields aren't part of the contract — if a field isn't known, it doesn't belong on the wire.
That means this whole UnmarshalJSON (and the matching MarshalJSON below) can be deleted, along with the ExtraFields map. Default encoding/json behavior on the plain struct is exactly what's wanted: declared fields round-trip, undeclared fields are dropped.
Follow-ups in this PR:
- Remove
ExtraFieldsfrom theDeviceInfostruct. - Remove the custom
UnmarshalJSONandMarshalJSON. - Drop
TestDeviceInfoJSONRoundTrip'sExtraFieldsassertion. - Decide on
certProvisioningModeandamtPhasein the Postman sample: either add them as proper typed fields onDeviceInfo, or remove them from the request body.
Net result is roughly -50 lines and removes a class of silent-drift bugs.
| } | ||
|
|
||
| for key, value := range d.ExtraFields { | ||
| raw[key] = value |
There was a problem hiding this comment.
This concern goes away entirely if ExtraFields and the custom MarshalJSON are removed — see the top-level comment on the deletes. Leaving the thread for visibility only.
| IPAddress string `json:"ipAddress"` | ||
| LastUpdated time.Time `json:"lastUpdated"` | ||
| TLSMode string `json:"tlsMode"` | ||
| UPID map[string]any `json:"upid,omitempty"` |
There was a problem hiding this comment.
map[string]any coerces numeric JSON values to float64 on Unmarshal. If any UPID subfield is ever sent as an integer (e.g. "oemPlatformIdType": 0), the re-marshaled value loses type fidelity, and any 64-bit identifier larger than 2^53 loses precision outright.
The Postman sample shows UPID has a knowable shape (oemPlatformIdType, oemId, csmeId) — a typed struct would be safer than map[string]any. If a typed struct isn't practical yet, map[string]json.RawMessage preserves the original bytes for round-trip without coercion.
| type alias DeviceInfo | ||
|
|
||
| base := alias(d) | ||
| base.ExtraFields = nil |
There was a problem hiding this comment.
Moot — the entire MarshalJSON (and UnmarshalJSON) should be deleted along with ExtraFields. See the top-level comment on the deletes.
| GUID: "device-guid-123", | ||
| TenantID: "tenant-id-456", | ||
| Hostname: "new-hostname", | ||
| DeviceInfo: `{"fwVersion":"16.1.30","fwBuild":"","fwSku":"","currentMode":"","features":"","ipAddress":"10.0.0.55","lastUpdated":"0001-01-01T00:00:00Z","tlsMode":"","meInterfaceVersion":"","lmsInstalled":true,"lmsVersion":"","osName":"","osVersion":"","osDistro":"","cpuModel":"","osIpAddress":""}`, |
There was a problem hiding this comment.
This pins the expected entity's DeviceInfo to a hand-written JSON string whose key order matches Go's declaration order of DeviceInfo fields. Any future reorder (e.g. grouping OS-related fields together for readability) silently breaks this test even though the behavior is unchanged.
require.JSONEq(t, expectedJSON, actualEntity.DeviceInfo) is order-insensitive, or assert on the unmarshaled DTO instead of the encoded blob.
|
OpenAPI spec is stale. CLAUDE.md requires regenerating the spec in the same PR: go run ./cmd/openapi-genThen commit the regenerated |
rsdmike
left a comment
rsdmike
left a comment
There was a problem hiding this comment.
Great first attack at this, thanks for doing this! Please see my comments. Most important we def need to remove the complexity with the deletes. Thanks!
|
Hi, please take a look at #905 it contains the lmsinstalled part of deviceInfo |
Adresses - #1020