Added filtering to get experiments endpoints#6418
Added filtering to get experiments endpoints#6418Auz wants to merge 4 commits intomaingrowthbook/growthbook:mainfrom gm/experiment-api-filtersgrowthbook/growthbook:gm/experiment-api-filtersCopy head branch name to clipboard
Conversation
|
Deploy preview for docs ready! ✅ Preview Built with commit b83b746. |
Confidence Score: 4/5This is close, but the remaining filter validation issue should be fixed before merging.
packages/back-end/src/api/metrics/listMetricExperiments.ts and its query validator Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/back-end/src/api/metrics/listMetricExperiments.ts:42-43
**Invalid Types Widen Results**
This mapping still lets invalid `type` filter tokens reach the metric experiment filter path. The metric endpoint validator accepts `type` as any string, so a request like `GET /api/v1/metrics/<id>/experiments?type=banana` passes validation, then `normalizeExperimentFilters` drops the unknown token and applies no implementation-type filter. Callers receive a broader experiment list instead of a 400 for an unsupported constrained value. The metric endpoint needs the same constrained CSV validation as the main experiments endpoint before this mapping runs.
Reviews (3): Last reviewed commit: "Addressed Luke's comments" | Re-trigger Greptile |
|
@greptileai review |
| .optional(), | ||
| result: csvQueryField( | ||
| experimentResultsType, | ||
| "Filter by comma-separated results (won, lost, inconclusive, dnf). Only stopped experiments carry a result", |
There was a problem hiding this comment.
It looks like we don't actually validate that it is only stopped experiments. If you switch a status back to running manually a result can still exist on the model. So we either need to add that check or clean up this language here and in filterExperiments to make sure it's totally clear.
There was a problem hiding this comment.
This is feature compatible with the front end. I'm not sure we should have different experiments returned by the same query.
|
@greptileai review again |
| // these are implementation types (feature | visualChange | redirect) | ||
| implementationTypes: splitCsv(req.query.type), |
There was a problem hiding this comment.
Invalid Types Widen Results
This mapping still lets invalid type filter tokens reach the metric experiment filter path. The metric endpoint validator accepts type as any string, so a request like GET /api/v1/metrics/<id>/experiments?type=banana passes validation, then normalizeExperimentFilters drops the unknown token and applies no implementation-type filter. Callers receive a broader experiment list instead of a 400 for an unsupported constrained value. The metric endpoint needs the same constrained CSV validation as the main experiments endpoint before this mapping runs.
Knowledge Base Used: The shared package
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/back-end/src/api/metrics/listMetricExperiments.ts
Line: 42-43
Comment:
**Invalid Types Widen Results**
This mapping still lets invalid `type` filter tokens reach the metric experiment filter path. The metric endpoint validator accepts `type` as any string, so a request like `GET /api/v1/metrics/<id>/experiments?type=banana` passes validation, then `normalizeExperimentFilters` drops the unknown token and applies no implementation-type filter. Callers receive a broader experiment list instead of a 400 for an unsupported constrained value. The metric endpoint needs the same constrained CSV validation as the main experiments endpoint before this mapping runs.
**Knowledge Base Used:** [The `shared` package](https://app.greptile.com/growthbook/-/custom-context/knowledge-base/growthbook/growthbook/-/docs/shared-package.md)
How can I resolve this? If you propose a fix, please make it concise.
Also updated skills so that agents can use the new endpoint options.