feat: get evaluation get result #88

Zaimwa9 · 2025-10-27T14:36:06Z

No description provided.

Zaimwa9 · 2025-10-28T15:12:08Z

lib/flagsmith/engine/segments/models.rb

+            'Integer' => ->(v) {
+              i = v.to_i;
+              i.to_s == v.to_s ? i : v
+            },
+            'Float' => ->(v) {
+              f = v.to_f;
+              f.to_s == v.to_s ? f : v
+            }


This implementation is problematic and was fixed in incoming PR.
This would end up coerciting 123abc to 123abc 🔴

Zaimwa9 · 2025-10-28T15:12:40Z

lib/flagsmith/engine/segments/evaluator.rb

+          # Simple JSONPath implementation - handle basic cases
+          path_parts = json_path.sub('$.', '').split('.')
+          current = context
+
+          path_parts.each do |part|
+            return nil unless current.is_a?(Hash)
+
+            current = current[part.to_sym]


Proper JSON path implementation done in following PR

khvn26

Leaving some comments as a first pass.

I'd really like to see which engine tests fail for this PR — if it's something to better pursue locally rather than in CI, let me know, @Zaimwa9.

khvn26 · 2025-10-30T16:47:37Z

lib/flagsmith/engine/evaluation/core.rb

+
+        # Returns { segments: EvaluationResultSegments; segmentOverrides: Record<string, SegmentOverride>; }
+        def evaluate_segments(evaluation_context)
+          return [], {} if evaluation_context[:identity].nil? || evaluation_context[:segments].nil?


I'm insisting that if evaluation_context[:identity].nil? part should happen in the mapper layer. We're not supposed to prevent identity-less evaluation in the engine, we're maintaining compatibility with /v1/flags for the get_environment_flags SDK interface.

khvn26 · 2025-10-30T16:49:49Z

lib/flagsmith/engine/segments/evaluator.rb

+        #
+        # @param context [Hash] Evaluation context containing identity and segment definitions
+        # @return [Array<Hash>] Array of segments that the identity matches
+        def get_identity_segments_from_context(context)


nit: For complete correctness:

Suggested change

def get_identity_segments_from_context(context)

def get_segments_from_context(context)

As stated previously, we're not shutting the door for identity-less contexts.

khvn26 · 2025-10-30T17:04:28Z

lib/flagsmith/engine/evaluation/core.rb

+              name: segment[:name]
+            }
+
+            result[:metadata] = segment[:metadata].dup if segment[:metadata]


Are we forced to use dup here? Does a simple result[:metadata] = segment[:metadata] imply clone?

From what I’ve read, assignment does not imply a clone — it just copies the reference to the same object. Since this struct is created fresh for every request, I think we can remove the dup here.

khvn26 · 2025-10-30T17:10:26Z

lib/flagsmith/engine/evaluation/core.rb

+            evaluated = if has_override
+                          { value: final_feature[:value], reason: nil }
+                        else
+                          evaluate_feature_value(final_feature, get_identity_key(evaluation_context))
+                        end


This probably reproduces the bug existing in the reference implementation, covered by test_multivariate__segment_override__expected_allocation.

Can we add tests to CI so we don't have to guess?

That's not part of this sprint?

I think you're confusing with Flagsmith/flagsmith-python-client#172 (and other similar issues in Flagsmith/flagsmith#5977)

Is it this: https://flagsmith.slack.com/archives/CTF0THS2D/p1761723486272129?

Yes it is — I don't think we should ship with that bug. The test covering it is part of engine-test-data anyway.

As far as I remember, I raised this during the standup to fix it in Rust, but we decided not to include it in this sprint. Is this bug present in all the SDKs?

We’re not releasing without the fix, but it can be included in a separate pull request. As far as I remember, that was the decision — please correct me if I’m mistaken.

As discussed offline, there's no reason not to inlclude the fix in this PR.

khvn26 · 2025-10-30T17:17:25Z

lib/flagsmith/engine/evaluation/core.rb

+
+            # Set reason
+            flag_result[:reason] = evaluated[:reason] ||
+                                   get_targeting_match_reason({ type: 'SEGMENT', override: segment_override })


I'm not sure why we need this additional level of misdirection with match_object and get_targeting_match_reason utility, when evaluate_feature_value and get_multivariate_feature_value could've simply set the reason then and there? Am I missing something?

khvn26 · 2025-10-30T17:23:24Z

lib/flagsmith/engine/evaluation/core.rb

+
+        # returns boolean
+        def higher_priority?(priority_a, priority_b)
+          (priority_a || Float::INFINITY) < (priority_b || Float::INFINITY)


nit: Maybe have a WEAKEST_PRIORITY constant set to Float::INFINITY.

I can see it addressed in #89; feel free to resolve or integrate the changes here.

khvn26 · 2025-10-30T17:23:59Z

lib/flagsmith/engine/evaluation/core.rb

+        end
+
+        # returns boolean
+        def higher_priority?(priority_a, priority_b)


Suggested change

def higher_priority?(priority_a, priority_b)

def stronger_priority?(priority_a, priority_b)

Or maybe even

Suggested change

def higher_priority?(priority_a, priority_b)

def is_stronger_priority?(priority_a, priority_b)

…h/flagsmith-ruby-client into feat/evaluation-get-result

khvn26 · 2025-11-03T16:26:34Z

lib/flagsmith/engine/segments/evaluator.rb

+        def non_primitive?(value)
+          return false if value.nil?
+
+          value.is_a?(Hash) || value.is_a?(Array)


Seems this is used in a negative boolean expression. For readability's sake, I'd prefer to have a is_primitive? method instead.

khvn26 · 2025-11-03T16:29:48Z

lib/flagsmith/engine/segments/evaluator.rb

+        # @return [Boolean] True if the condition matches
+        def traits_match_segment_condition_from_context(condition, segment_key, context)
+          if condition[:operator] == PERCENTAGE_SPLIT
+            context_value_key = get_context_value(condition[:property], context) || get_identity_key_from_context(context)


Maybe optimise this by memorising the key in the context itself, like Python engine does.

Zaimwa9 added 7 commits October 24, 2025 16:51

feat: added-engine-function-signatures

2aeb65a

feat: moved-engine-to-core

ba057cb

feat: implemented-process-segment-overrides

bb96eb1

feat: implemented-evalute-segments-partially

a2fd5a8

feat: implemented-should-apply-override

0f08fb1

feat: implemented-get-identity-segments

da08e8e

feat: implemented-new-in-and-fixed-remaining-tests

af51bf7

Zaimwa9 requested a review from a team as a code owner October 27, 2025 14:36

Zaimwa9 requested review from khvn26 and removed request for a team October 27, 2025 14:36

Zaimwa9 changed the base branch from main to feat/evaluation-context-mappers October 27, 2025 14:36

Zaimwa9 marked this pull request as draft October 27, 2025 14:36

Zaimwa9 commented Oct 28, 2025

View reviewed changes

Zaimwa9 added 3 commits October 28, 2025 17:15

feat: run-lint

0fb7587

feat: rebased

741ceeb

feat: misc

6a6a129

Zaimwa9 marked this pull request as ready for review October 29, 2025 09:41

Zaimwa9 changed the title ~~feat: get evaluation get result [draft]~~ feat: get evaluation get result Oct 29, 2025

khvn26 requested changes Oct 30, 2025

View reviewed changes

Zaimwa9 added 2 commits October 30, 2025 20:36

Merge branch 'feat/evaluation-context-mappers' of github.com:Flagsmit…

3bad95b

…h/flagsmith-ruby-client into feat/evaluation-get-result

feat: json-path-lib-implementation

f0a53b7

khvn26 reviewed Nov 3, 2025

View reviewed changes

khvn26 mentioned this pull request Nov 3, 2025

feat: sdk consumes context engine #89

Open

remove dup

ef1274a

	def get_identity_segments_from_context(context)
	def get_segments_from_context(context)

	def higher_priority?(priority_a, priority_b)
	def stronger_priority?(priority_a, priority_b)

	def higher_priority?(priority_a, priority_b)
	def is_stronger_priority?(priority_a, priority_b)

feat: get evaluation get result #88

Are you sure you want to change the base?

feat: get evaluation get result #88

Uh oh!

Conversation

Zaimwa9 commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

khvn26 Oct 30, 2025 •

edited

Loading

khvn26 Nov 5, 2025 •

edited

Loading

khvn26 Nov 3, 2025 •

edited

Loading