feat: add sourcePolicy to History API values requests#2817
Conversation
Allow History API values requests to pass sourcePolicy to providers so clients can request source-aware data. Document sourceRef in the values metadata and validate sourcePolicy values before invoking the provider.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesHistory API sourcePolicy feature
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/api/history/index.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b6b0a00-dc98-4f56-8d41-33b8ef0d00a5
⛔ Files ignored due to path filters (1)
test/plugin-test-config/node_modules/testplugin/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (5)
packages/server-api/src/history.tspackages/server-api/src/typebox/history-schemas.tssrc/api/history/index.tssrc/api/history/openApi.tstest/history-api.ts
| let sourcePolicy: HistorySourcePolicy | undefined | ||
| try { | ||
| sourcePolicy = parseSourcePolicy(query.sourcePolicy) | ||
| } catch (error) { | ||
| errors.push(error instanceof Error ? error.message : 'Invalid sourcePolicy') | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 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.
Document that sourcePolicy=all only expands paths without an explicit sourceRef so callers know that path-level source filters take precedence.
|
Follow-up: the CodeRabbit note about I also tried to add the |
tkurki
left a comment
tkurki
left a comment
There was a problem hiding this comment.
In deltas we have $source whose type is SourceRef, so let's stick to that. Or am I missing something?
|
Updated in cf68e1c: history values metadata now uses I kept the request-side Does that address the concern, or would you prefer the request-side |
| * Controls how history providers should handle multiple sources for the | ||
| * requested paths. | ||
| * | ||
| * - `preferred`: provider default / priority-resolved source handling |
There was a problem hiding this comment.
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.
2 participants
Add a
sourcePolicyquery parameter to/history/valuesand pass it through to History API providers.This allows clients to request source-aware historical values with
sourcePolicy=all. Providers can then return separate value columns per source and includesourceRefin thevaluesmetadata.Also update the History API response schema to document optional
values[].sourceRef.Closes #2780.
Validation:
This PR adds a
sourcePolicyquery option to/history/valuesand threads it through the History API request types and handler logic. It validatessourcePolicyaspreferredorall, returns a 400 error (includingsourcePolicydetails) for invalid values, and documents the parameter in OpenAPI. WhensourcePolicy=all, it prompts History API providers to return values split by source and to include$sourcein thevaluesmetadata; path specs that already specify an explicitsourceRefremain an explicit filter.It also updates the History API TypeBox schemas to use
$sourceforvalues[].$sourcein the response, and keepssourceRefonPathSpecfor request-side source filtering. Tests verify thatsourcePolicy=allproduces per-sourcevaluesentries with the expected$sourcevalues and that invalid input is rejected.