fix: various custom operator conformance fixes#1778
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the flagd core evaluation logic, including enhanced type handling for context values, more robust SemVer parsing with support for partial versions and numeric coercion, and fixes for fractional evaluation edge cases. I have provided a suggestion to optimize the fractional evaluation logic by using List operations instead of manual array copying to improve performance.
| Object[] source = arguments.toArray(); | ||
| distributions = Arrays.copyOfRange(source, 1, source.length); | ||
| Object[] remaining = Arrays.copyOfRange(source, 1, source.length); | ||
|
|
||
| // json-logic pre-evaluation flattens a single-entry fractional | ||
| // e.g. [["single",1]] becomes ["single", 1]; detect and re-wrap | ||
| if (remaining.length > 0 && !(remaining[0] instanceof List)) { | ||
| List<Object> wrapped = new ArrayList<>(); | ||
| wrapped.add(arg1); | ||
| for (Object r : remaining) { | ||
| wrapped.add(r); | ||
| } | ||
| distributions = new ArrayList<>(); | ||
| distributions.add(wrapped); | ||
| } else { | ||
| distributions = Arrays.asList(remaining); | ||
| } |
There was a problem hiding this comment.
The current implementation performs multiple array copies and manual list building, which can be inefficient as this code is executed during every flag evaluation. Using List.subList and List.addAll is more efficient and idiomatic for this purpose.
List<Object> remaining = arguments.subList(1, arguments.size());
// json-logic pre-evaluation flattens a single-entry fractional
// e.g. [["single",1]] becomes ["single", 1]; detect and re-wrap
if (!remaining.isEmpty() && !(remaining.get(0) instanceof List)) {
List<Object> wrapped = new ArrayList<>(remaining.size() + 1);
wrapped.add(arg1);
wrapped.addAll(remaining);
distributions = Arrays.asList((Object) wrapped);
} else {
distributions = remaining;
}There was a problem hiding this comment.
Agreed. Fixed. Also made improvements around the $ref replace.
toddbaert
force-pushed
the
chore/update-test-harness
branch
from
7c5c7cd to
6977314
Compare
toddbaert
changed the title
toddbaert
force-pushed
the
chore/update-test-harness
branch
2 times, most recently
from
d4dbb57 to
436d041
Compare
* support v/V prefix in semver * support partial versions in semver * support numeric context values in semver * return null on errors * fix fractional single-entry flattening Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
chore/update-test-harness
branch
2 times, most recently
from
529b1d1 to
6f64b40
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
chore/update-test-harness
branch
from
6f64b40 to
d7c20b4
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
requested review from
aepfli,
alexandraoberaigner,
chrfwow,
guidobrei and
liran2000
and removed request for
Kavindu-Dodan
| @@ -51,7 +61,7 @@ public Object evaluate(List arguments, Object data, String jsonPath) throws Json | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Do we also need to check for flattening here for single-entry fractionals? Or does that not happen here because the pre-evaluation doesn't "get confused" by 2 different list entry types?
| // bucket key is a null var result (simulated by being a non-string, non-list) | ||
| List.of("one", 50), List.of("two", 50)); | ||
|
|
||
| // targeting key is null, so fractional falls back to flagKey + targetingKey |
There was a problem hiding this comment.
nit-pick, do you mean "bucketing key is null"?
7 participants
fix: various custom operator conformance fixes
Fixes: #1774
Related: