-
Notifications
You must be signed in to change notification settings - Fork 21
feat!: Context Values support, getEvaluationResult
#184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for context values and adds a new getEvaluationResult API while removing legacy engine APIs. The changes involve a breaking refactor from Java 8 to Java 11 and introduces JSONPath-enabled context value evaluation.
Key changes:
- Introduces
EvaluationResult getEvaluationResult(EvaluationContext context)API replacing old model-based APIs - Adds JSONPath support for context value evaluation and removes model-based segment evaluation
- Drops Java 8 support in favor of Java 11 minimum requirement
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updates Java version from 8 to 11, adds JsonPath dependency and code generation plugin |
| src/main/java/com/flagsmith/flagengine/Engine.java | Replaces model-based feature evaluation with context-based evaluation using new API |
| src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java | Refactors from model-based to context-based segment evaluation with JSONPath support |
| src/main/java/com/flagsmith/mappers/EngineMappers.java | New mapper utility for converting JSON documents to engine contexts |
| src/main/java/com/flagsmith/interfaces/IOfflineHandler.java | Breaking change: replaces EnvironmentModel with EvaluationContext |
| src/test/java/com/flagsmith/flagengine/unit/segments/SegmentModelTest.java | Updates test to use new context-based evaluation instead of model-based |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/test/java/com/flagsmith/flagengine/unit/segments/SegmentModelTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non blocking comments, since engine tests pass. These comments may or may not require attention.
P.S. Reviewing this on the iPad was frustratingly difficult, as is reading this Java codebase sometimes. I’m sorry for the somewhat poor review compared to my standards.
src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall. I share Evandro's confusion surrounding boolean management - some clarification could help, also to have other implementations ISO.
I added a comment on the modulo evaluator
ec51cfd to
7d1c948
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loooks goooood. Approving but have minor comments / NIT so feel free to discard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on @emyller's review, and my subsequent follow up on the comments that he had left. I have not done a comprehensive review of the code, only the parts relevant to those conversations.
Contributes to #179.
In this PR, we:
EvaluationResult getEvaluationResult(EvaluationContext context)and remove all of the old engine APIs.EngineMappersclass for engine-API data interop.EnvironmentModelclass, used byIOfflineHandler, is no longer part ofcom.flagsmith.flagenginepackage. Methods carrying the engine business logic and sub-models/fields not relevant to the SDK are removed.