refactor: add GET /api/v1/devices/export endpoint - #1209
Conversation
- flat->nested mapping done in one place so SQL and Mongo responses match - me/os/platform/bmc render as null when a subsystem has no data - X-Total-Count header and server-side audit log per export attempt - fields with no writer yet are marked export-map and render null Add the Fuego/OpenAPI declaration and a Postman request asserting the envelope shape, X-Total-Count, and absence of credential fields. Signed-off-by: ShradhaGupta31 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1209 +/- ##
==========================================
+ Coverage 50.13% 50.19% +0.05%
==========================================
Files 147 148 +1
Lines 13574 13769 +195
==========================================
+ Hits 6805 6911 +106
- Misses 6176 6263 +87
- Partials 593 595 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| outcomeSuccess = "success" | ||
| outcomeError = "error" | ||
| tenantClaimKey = "tenantId" |
There was a problem hiding this comment.
This is not needed, the way tenantId is obtained is out of scope for this PR. In console we are planning to get the tenantid through x-tenant-id header. Refer this PR: #1220
| ctx, cancel := context.WithTimeout(c.Request.Context(), exportTimeout) | ||
| defer cancel() | ||
|
|
||
| devicesList, err := dr.t.Get(ctx, maxExportRecords, 0, tenantID) |
There was a problem hiding this comment.
For now pass tenantID as "" through PR: #1220 will update this correctly
|
|
||
| // exportIdentity reads the tenant and subject from the already-verified JWT. The | ||
| // export is tenant-scoped: only devices for the caller's tenant are returned | ||
| func exportIdentity(c *gin.Context) (tenantID, userID string) { |
There was a problem hiding this comment.
This function is not needed
| h := handler.Group("/devices") | ||
| { | ||
| h.GET("", r.get) | ||
| h.GET("export", r.export) |
There was a problem hiding this comment.
The GET /api/v1/devices/export approach could work as an initial implementation for relatively small datasets where the export can be generated within the lifetime of a single HTTP request. We could also start with an explicit limit on the number of devices that can be exported for ex: 500.
However, we should consider how this approach would evolve when the number of devices grows significantly or when filters result in a long-running query. In such cases, a synchronous request may lead to long request durations, timeouts, and increased resource consumption.
To avoid having to change the API contract later, it may be worth starting with the export modeled as a separate resource which you are already doing, while initially keeping the underlying implementation simple.
For future we need to plan implementing the export resource with API's for ex:
# Creates an export request with the required filters and returns an export ID. Initially, the export could be processed by an in-process/background worker.
POST /api/v1/device-exports
# Retrieves the status and metadata of the export job, such as queued, running, completed, or failed.
GET /api/v1/device-exports/{id}
# Downloads the generated export file once the export has completed.
GET /api/v1/device-exports/{id}/file
# Cancels an in-progress export and/or removes the export job and generated file, depending on the defined lifecycle.
DELETE /api/v1/device-exports/{id}
This would allow us to incrementally evolve the implementation without changing the external API. For example, we could initially support small exports using an in-process worker and local storage, and later add batch processing, persistent job state, cancellation, cleanup, distributed workers, and object storage as scalability requirements evolve.
This approach gives us something simple to implement initially while providing a clearer path for supporting larger datasets and container/Kubernetes deployments in the future.
|
|
||
| const ( | ||
| // maxExportRecords is the hard cap on records returned by a single export. | ||
| maxExportRecords = 10000 |
There was a problem hiding this comment.
10000 seems to be a big number we would reduce it to may be 500 or so. Please see my comment in the devices.go
| exportTimeout = 60 * time.Second | ||
|
|
||
| // exportCountHeader tells the client how many records are in the response. | ||
| exportCountHeader = "X-Total-Count" |
There was a problem hiding this comment.
Why do we need an header ? This should be captured in the response body
2 participants
Addresses: #1076
ADR: https://github.com/device-management-toolkit/console/wiki/ADR-devices-export-api