Uh oh!
There was an error while loading. Please reload this page.
feat: Add segment types, segment metadata, identity override segments support - #198
feat: Add segment types, segment metadata, identity override segments support#198khvn26 wants to merge 4 commits into
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against de8a1ba |
matthewelwell
left a comment
There was a problem hiding this comment.
A few minor comments but looks pretty clean on the whole.
| IS_NOT_SET: ConditionOperator = "IS_NOT_SET" | ||
| IN: ConditionOperator = "IN" | ||
| SEGMENT_IDENTIFIER_PROPERTY_NAME: str = "_$identity.identifier" |
There was a problem hiding this comment.
I kind of feel like we could generalise this to just the prefix? So e.g. _$identity.identity_uuid would also work? Maybe that's a bit YAGNI?
There was a problem hiding this comment.
_$identity.identity_uuid would also work?
In the future, maybe we'll want it to. Maybe we'll have other entities participating in the matching, e.g. $_contexts.org.location.
| name: str | ||
| rules: typing.List[SegmentRuleModel] = Field(default_factory=list) | ||
| feature_states: typing.List[FeatureStateModel] = Field(default_factory=list) | ||
| meta: typing.Optional[typing.Dict[str, str]] = None |
There was a problem hiding this comment.
I wonder if this should be at least a typed dict? Maybe one for a little further down the line when we know exactly what we need in the meta dict?
There was a problem hiding this comment.
There's two reasons I don't want to impose a typed dict here yet:
- The metadata schema is not settled yet.
- The engine does not need this metadata, APIs do. It's on Core/Edge to maintain the schema (could be a common library, just not the engine.)
| ) | ||
| segment_identity_override = SegmentModel( | ||
| id=7, | ||
| name="segment_identity_override", |
There was a problem hiding this comment.
We talked about this being unique in someway, right? Should we illustrate that even in this test data?
| meta={ | ||
| "identity_identifier": identifier, | ||
| "type": "IDENTITY_OVERRIDE", | ||
| }, |
There was a problem hiding this comment.
Do we want to add identity_uuid here too?
matthewelwell
left a comment
There was a problem hiding this comment.
@khvn26 I think this PR is still valuable but probably needs some reworking to fit in with the changes we made to the data structures?
khvn26
commented
Aug 18, 2025
Superseded by #220. |
- Bump the engine-test-data submodule to pick up Flagsmith/engine-test-data#51 (a realistic 262-feature / 26-multivariate / segment-override test case mirroring the #198 scenario) and consume it as the large-context benchmark, dropping the synthetic local fixture. - Exercise the `metadata` branch of the `get_flag_result_from_context` wrapper by adding metadata to the existing parametrized test. - Add a test for the `value == 100` recursive fallback on the new two-key fast-path hasher. Restores 100% coverage and keeps the realistic benchmark portable across every SDK consuming engine-test-data, rather than living only in this repo's Python tests. beep boop
- Bump the engine-test-data submodule to pick up Flagsmith/engine-test-data#51 (a realistic 262-feature / 26-multivariate / segment-override test case mirroring the #198 scenario) and consume it as the large-context benchmark, dropping the synthetic local fixture. - Exercise the `metadata` branch of the `get_flag_result_from_context` wrapper by adding metadata to the existing parametrized test. - Add a test for the `value == 100` recursive fallback on the new two-key fast-path hasher. Restores 100% coverage and keeps the realistic benchmark portable across every SDK consuming engine-test-data, rather than living only in this repo's Python tests. beep boop
Closes#197.
This PR adds the following:
meta: dict[str, str] | Nonefield to the Segment model. It's intended to be used by Core and Edge API to store data specific for their Flagsmith implementations, unrelated to the engine and SDKs."_$identity.identifier"string literal can be now assigned toSegmentCondition.property_to make the engine match segment condition rules against the identity identifier.