Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 31, 2025

Fix for issue #6051: Omit segments from evaluation context when get_environment_flags is called

This change addresses the issue where segments were unnecessarily included in the evaluation context when retrieving environment flags. Segments are only relevant for identity-specific evaluations, not for environment-wide flag evaluations.

Changes made:

  • Modified _get_environment_flags_from_document() to filter out segments from the evaluation context before passing it to the engine
  • Simplified implementation using copy() and pop() instead of dictionary comprehension
  • Uses consistent keyword argument style (context=...) for all engine.get_evaluation_result calls
  • Added comprehensive tests with proper fixtures and mocking

Testing & Quality:

  • ✅ All existing tests pass (73 tests)
  • ✅ All linters pass (black, isort, flake8, mypy)
  • ✅ Code review feedback addressed

Impact:

  • Improves performance by reducing unnecessary data in the evaluation context for environment flags
  • Maintains backward compatibility - no API changes
  • Segments continue to work correctly for identity-specific evaluations

Ready for review!

Original prompt

This section details on the original issue you should resolve

<issue_title>get_environment_flags includes segments in evaluation context</issue_title>
<issue_description>See Flagsmith/flagsmith#6051.

In the mapper layer, omit segments from evaluation context when get_environment_flags is called.</issue_description>

<agent_instructions>use conventional-commit for PR title:
fix:

add a new test — perhaps by adding test_mappers.py

verify with pytest and pre-commit run -a</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits October 31, 2025 19:42
Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>
Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>
Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix get_environment_flags to omit segments in evaluation context fix: omit segments from evaluation context in get_environment_flags Oct 31, 2025
Copilot AI requested a review from khvn26 October 31, 2025 19:57
Copilot finished work on behalf of khvn26 October 31, 2025 19:57
Comment on lines 235 to 236
flagsmith._evaluation_context = evaluation_context
flagsmith.enable_local_evaluation = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use local_eval_flagsmith fixture instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10242fb

# Given
flagsmith._evaluation_context = evaluation_context
flagsmith.enable_local_evaluation = True
mock_engine = mocker.patch("flagsmith.flagsmith.engine")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mock specific function and use autospec:

Suggested change
mock_engine = mocker.patch("flagsmith.flagsmith.engine")
mock_get_evaluation_result = mocker.patch(
"flagsmith.flagsmith.engine.get_evaluation_result",
autospec=True,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10242fb

Comment on lines 260 to 262
mock_engine.get_evaluation_result.assert_called_once()
call_args = mock_engine.get_evaluation_result.call_args
context = call_args[1]["context"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use assert_called_once_with method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10242fb

Comment on lines 125 to 127
mock_engine.get_evaluation_result.assert_called_once()
call_args = mock_engine.get_evaluation_result.call_args
context = call_args[1]["context"] # Keyword argument 'context'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use assert_called_once_with method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10242fb

Comment on lines 337 to 344
context_without_segments = typing.cast(
SDKEvaluationContext,
{
key: value
for key, value in self._evaluation_context.items()
if key != "segments"
},
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps to avoid both the cast and a bytecode loop:

Suggested change
context_without_segments = typing.cast(
SDKEvaluationContext,
{
key: value
for key, value in self._evaluation_context.items()
if key != "segments"
},
)
context_without_segments = self._evaluation_context.copy()
context_without_segments.pop("segments", None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10242fb

@khvn26
Copy link
Member

khvn26 commented Oct 31, 2025

@copilot Please address the review comments above.

- Use local_eval_flagsmith fixture in tests
- Mock specific function with autospec
- Use assert_called_once_with for cleaner assertions
- Simplify context filtering with copy() and pop()

Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 31, 2025

@copilot Please address the review comments above.

All review comments addressed in 10242fb. Changes include:

  • Using local_eval_flagsmith fixture
  • Mocking specific function with autospec=True
  • Using assert_called_once_with for cleaner assertions
  • Simplified context filtering with copy() and pop()

All tests pass (73/73) and linters are clean.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • test_urlanalytics
    • Triggering command: /home/REDACTED/.cache/pypoetry/virtualenvs/flagsmith-K6knkhUn-py3.12/bin/python /home/REDACTED/.cache/pypoetry/virtualenvs/flagsmith-K6knkhUn-py3.12/bin/pytest tests/ -x (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from khvn26 October 31, 2025 22:32
Copilot finished work on behalf of khvn26 October 31, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_environment_flags includes segments in evaluation context

2 participants