Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/server-api/src/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,21 @@ export type AggregateMethod =
export type ValueList = {
path: Path
method: AggregateMethod
sourceRef?: SourceRef
$source?: SourceRef
}[]

/**
* Controls how history providers should handle multiple sources for the
* requested paths.
*
* - `preferred`: provider default / priority-resolved source handling

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. What would 'preferred' mean in practise? That provider would be able to figure out what source's value was preferred at the time, for arbitrarily varying lengths of time slices? To me that sounds like getting into the too complex domain.

Is this really needed? I'd like to leave it out.

* - `all`: return values split by source, with `$source` set in `values`
*
* A `sourceRef` specified on an individual path remains an explicit filter;
* `all` only expands paths that do not already specify a source.
*/
export type HistorySourcePolicy = 'preferred' | 'all'

/**
* A row of historical data: first element is timestamp, followed by aggregated values.
* Values can be primitives, objects (like navigation.position), or null depending on the path.
Expand Down Expand Up @@ -104,6 +116,7 @@ export type TimeRangeQueryParams =
export type ValuesRequestQueryParams = TimeRangeQueryParams & {
context?: string
resolution?: number
sourcePolicy?: HistorySourcePolicy
}

export type PathsRequestQueryParams = TimeRangeQueryParams
Expand Down Expand Up @@ -258,6 +271,7 @@ export interface PathSpec {
export type ValuesRequest = TimeRangeParams & {
context?: Context
resolution?: number
sourcePolicy?: HistorySourcePolicy
pathSpecs: PathSpec[]
}

Expand Down
16 changes: 14 additions & 2 deletions packages/server-api/src/typebox/history-schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ export const ValuesResponseSchema = Type.Object(
values: Type.Array(
Type.Object({
path: Type.String({ description: 'Signal K path' }),
method: Type.String({ description: 'Aggregation method' })
method: Type.String({ description: 'Aggregation method' }),
$source: Type.Optional(
Type.String({
description:
'Source reference for this value column, present when the values are source-specific'
})
)
})
),
data: Type.Array(
Expand Down Expand Up @@ -80,7 +86,13 @@ export const PathSpecSchema = Type.Object(
parameter: Type.Array(Type.String(), {
description:
'Additional parameters for the aggregation method (e.g., sample count for sma, alpha for ema)'
})
}),
sourceRef: Type.Optional(
Type.String({
description:
'Optional source reference to filter this path to a single source'
})
)
},
{
$id: 'PathSpec',
Expand Down
15 changes: 15 additions & 0 deletions src/api/history/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
PathSpec,
PathsRequest,
PathsResponse,
HistorySourcePolicy,
TimeRangeParams,
ValuesRequest,
ValuesResponse,
Expand Down Expand Up @@ -281,6 +282,13 @@ const parseValuesQuery = (query: Record<string, unknown>): ValuesRequest => {
errors.push('paths parameter is required and must be a string')
}

let sourcePolicy: HistorySourcePolicy | undefined
try {
sourcePolicy = parseSourcePolicy(query.sourcePolicy)
} catch (error) {
errors.push(error instanceof Error ? error.message : 'Invalid sourcePolicy')
}

Comment on lines +285 to +291

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider validating/documenting sourcePolicy=all + per-path sourceRef interaction.

splitPathExpression (Line 392-394) lets a path already pin a single sourceRef via path|sourceRef. Combined with sourcePolicy=all, the semantics are ambiguous (should sourcePolicy be ignored, or override the pinned source?). Neither parseValuesQuery nor the OpenAPI docs address this combination.

Not blocking since provider behavior for this combination isn't defined in this PR's scope, but worth a validation error or explicit doc note to avoid confusing edge-case behavior downstream.

Also applies to: 296-299, 312-316, 368-396

🤖 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 `@src/api/history/index.ts` around lines 285 - 291, Add an explicit validation
or documentation note for the ambiguous `sourcePolicy=all` plus per-path
`sourceRef` case in `parseValuesQuery` and `splitPathExpression`. If this
combination is unsupported, reject it early with a clear error before building
the query; otherwise, document which setting wins so the behavior is unambiguous
for callers and reflected in the OpenAPI docs.

if (errors.length > 0) {
throw new Error(`Validation errors: ${errors.join(', ')}`)
}
Expand All @@ -294,12 +302,19 @@ const parseValuesQuery = (query: Record<string, unknown>): ValuesRequest => {
...timeRangeParams,
context,
resolution,
sourcePolicy,
pathSpecs
}
debug.enabled && debug(JSON.stringify(parsed, null, 2))
return parsed
}

const parseSourcePolicy = (value: unknown): HistorySourcePolicy | undefined => {
if (value === undefined || value === null || value === '') return undefined
if (value === 'preferred' || value === 'all') return value
throw new Error("sourcePolicy parameter must be 'preferred' or 'all'")
}

// Maps the single-letter unit suffix in a resolution time expression to seconds.
const RESOLUTION_UNIT_SECONDS: Record<string, number> = {
s: 1,
Expand Down
9 changes: 8 additions & 1 deletion src/api/history/openApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,13 @@ historyApiDoc.paths = {
"Length of data sample time window in seconds or as a time expression ('1s', '1m', '1h', '1d'). If resolution is not specified the server should provide data in a reasonable time resolution, depending on the time range in the request.",
schema: { type: 'number', format: 'integer' }
},
{
name: 'sourcePolicy',
in: 'query',
description:
"Controls how multiple historical sources are handled. With 'all', providers should return values split by source and include $source in the values metadata. A sourceRef specified on an individual path remains an explicit filter; 'all' only expands paths that do not already specify a source.",
schema: { type: 'string', enum: ['preferred', 'all'] }
},
{ $ref: '#/components/parameters/ProviderIdQuery' }
],
responses: {
Expand Down Expand Up @@ -198,7 +205,7 @@ historyApiDoc.paths = {
type: 'string',
description: 'Aggregation method'
},
sourceRef: {
$source: {
type: 'string',
description:
'Source reference, present when the query specified a source filter for this path'
Expand Down
31 changes: 31 additions & 0 deletions test/history-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,37 @@ describe('History API v2', () => {
body.data.length.should.be.greaterThan(0)
})

it('passes sourcePolicy=all to the provider', async function () {
const res = await fetch(
`${api}/history/values?paths=navigation.speedOverGround&from=${FROM}&to=${TO}&resolution=60&sourcePolicy=all`
)
res.status.should.equal(200)
const body = await res.json()
assertSchema(ValuesResponseSchema, body, 'ValuesResponse')
body.values.should.deep.equal([
{
path: 'navigation.speedOverGround',
method: 'average',
$source: 'source.a'
},
{
path: 'navigation.speedOverGround',
method: 'average',
$source: 'source.b'
}
])
body.data.should.deep.equal([['2025-01-01T12:00:00Z', 1.2, 2.4]])
})

it('returns 400 for invalid sourcePolicy', async function () {
const res = await fetch(
`${api}/history/values?paths=navigation.position&from=${FROM}&to=${TO}&sourcePolicy=unknown`
)
res.status.should.equal(400)
const body = await res.json()
body.error.should.contain('sourcePolicy')
})

it('returns paths from the provider', async function () {
const res = await fetch(`${api}/history/paths?from=${FROM}&to=${TO}`)
res.status.should.equal(200)
Expand Down
25 changes: 19 additions & 6 deletions test/plugin-test-config/node_modules/testplugin/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading