feat(constraints): add bulk edit mode for constraints#6017
Conversation
…faster evaluation, and add bulk edit mode for constraints Signed-off-by: Jon Gaul <[email protected]>
dosubot
Bot
added
the
size:L
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: request changes
The pre-parsed constraint optimization is well-structured and the bulk-edit UI is clean, but there's a correctness gap in matchesNumber: scalar number operators (eq, neq, lt, lte, gt, gte) use c.Number without a guard for whether PrepareForEvaluation was actually called. If a developer constructs an EvaluationConstraint directly and skips preparation, the constraint silently evaluates against 0.0 instead of erroring or non-matching, which contradicts the PR's stated intent.
internal/server/evaluation/evaluation.go
- major (L733):
matchesNumberfor scalar operators (eq, neq, lt, lte, gt, gte) assignsvalue := c.Numberafter only checkingc.Value == "". If a developer manually constructs anEvaluationConstraintwith a non-emptyValueand skipsPrepareForEvaluation(),c.Numberdefaults to0.0and the constraint silently evaluates against zero instead of degrading to an error/non-match as the PR description intends. Either use*float64forNumber(sonilis an unambiguous unprepared sentinel) or add a guard that falls back to parsing when the field looks unpopulated.
🤖 Automated review by the Flipt PR review agent.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #6017 +/- ##
==========================================
+ Coverage 61.39% 61.43% +0.04%
==========================================
Files 142 142
Lines 14261 14315 +54
==========================================
+ Hits 8755 8795 +40
- Misses 4771 4782 +11
- Partials 735 738 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- add Datetime field and parseDatetime helper for DATETIME_COMPARISON_TYPE - skip malformed segments during snapshot build with logged error instead of failing the entire namespace - thread logger through addDoc for error reporting - test error wrapping for all constraint parse failure paths (string, number, datetime) - apply consistent error border styling on bulk-mode textarea Signed-off-by: Roman Dmytrenko <[email protected]>
dosubot
Bot
added
size:XL
Signed-off-by: Roman Dmytrenko <[email protected]>
2 participants
Description
This PR raises the constraint value limit (
MAX_JSON_ARRAY_ITEMS) from 100 to 10,000, pre-parses the values in the JSON on the backend so evaluation stays performant at larger sizes, and adds a bulk-edit UI for theisoneofandisnotoneofoperators.Large rollouts are currently possible on Flipt by creating multiple sets of constraints in the UI or by manually editing the YAML files, but neither is ideal.
evaluation.gore-parses the JSON (and numbers) on each evaluation, so too many values may lead to performance issues. To address this,EvaluationConstraintnow stores pre-parsed values and uses them for matching.Behavior Change
isoneof/isnotoneofJSON and unparseable numbers for comparisons) now fail on snapshot load with an error naming the segment and property. Previously they were loaded fine and silently never matched at evaluation time (though they did produce a Debug-logged error). Hand-edited YAML with a bad constraint will block the whole snapshot.Considerations / Open questions
EvaluationConstraintrather than making a new parsed type. I don't love that a dev could construct anEvaluationConstraintand skipNewEvaluationConstraint(). That case now degrades to a logged error and non-match (same behavior as handling a malformed value prior to this PR). A new parsed type would make it unrepresentable. That would be a much bigger diff, though, and this is already pushing it...matchesDateTimestill parses the constraint value on every evaluation and could get a similar fix as a follow-up. I'm happy to do that if requested.UI Screenshot