-
Notifications
You must be signed in to change notification settings - Fork 5
feat(data): add getBatchForStudyDeployments endpoint with filtering #517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,13 @@ package dk.cachet.carp.data.application | |
|
|
||
|
|
||
| import dk.cachet.carp.common.application.UUID | ||
| import dk.cachet.carp.common.application.data.DataType | ||
| import dk.cachet.carp.common.application.services.ApiVersion | ||
| import dk.cachet.carp.common.application.services.ApplicationService | ||
| import dk.cachet.carp.common.application.services.IntegrationEvent | ||
| import kotlinx.serialization.* | ||
| import kotlinx.datetime.Instant | ||
| import kotlinx.serialization.Required | ||
| import kotlinx.serialization.Serializable | ||
|
|
||
|
|
||
| /** | ||
|
|
@@ -60,6 +63,33 @@ interface DataStreamService : ApplicationService<DataStreamService, DataStreamSe | |
| toSequenceIdInclusive: Long? = null | ||
| ): DataStreamBatch | ||
|
|
||
| /** | ||
| * Retrieve data across multiple study deployments, optionally filtered by device role names, | ||
| * data types, and time range. | ||
| * | ||
| * The response is a canonical [DataStreamBatch]: for each [DataStreamId], sequences are | ||
| * ordered by start time and non-overlapping (contract preserved). No derived/secondary | ||
| * indexing is applied in this API; analytics-specific projections are out of scope here. | ||
|
Comment on lines
+70
to
+72
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop this; all of this are implementation/design details. Not API documentation. The contract (API) of |
||
| * | ||
| * Time range semantics: if [from] or [to] are specified, sequences are clipped to the | ||
| * half-open interval [from, to) (inclusive start, exclusive end). | ||
|
Comment on lines
+74
to
+75
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That only works if from and to are specified. But, it looks like you can omit this and instead just document inclusive/exclusive nature in the corresponding from/to parameters. As is, this causes more confusion than it answers edge cases. Instead, I'm more surprised about how |
||
| * | ||
| * @param studyDeploymentIds Study deployments to query. Must not be empty. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? It seems like an overly strict contract. You can easily return nothing if you pass nothing, which would cause less additional handling for this edge case by the caller if they don't care about optimization/saving a roundtrip. |
||
| * @param deviceRoleNames Optional device role name filter (e.g., "phone"). If null or empty, all are included. | ||
| * @param dataTypes Optional data type filter. If null or empty, all are included. | ||
|
Comment on lines
+78
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's counter intuitive. If empty, just filter out everything. A good API doesn't give two ways to do the same thing. On why it matters: suppose a caller sets up a dynamic filter determining the set of device role names they are interested in, which ends up being empty. Now the caller will get all data, instead of no data, as expected. |
||
| * @param from Optional absolute start time (inclusive). If null, no lower bound. | ||
| * @param to Optional absolute end time (exclusive). If null, no upper bound. | ||
| * @return A [DataStreamBatch] containing matching data sequences, preserving per-stream invariants. | ||
| */ | ||
| suspend fun getBatchForStudyDeployments( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not certain about the naming of this. Maybe simply |
||
| studyDeploymentIds: Set<UUID>, | ||
| deviceRoleNames: Set<String>? = null, | ||
| dataTypes: Set<DataType>? = null, | ||
| from: Instant? = null, | ||
| to: Instant? = null | ||
| ): DataStreamBatch | ||
|
|
||
|
|
||
| /** | ||
| * Stop accepting incoming data for all data streams for each of the [studyDeploymentIds]. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's configured, but wildcard is used for serialization imports across the codebase.