feat: Generic segment metadata

[khvn26]

In this PR, we improve the recently added Segment.metadata type to support generic metadata types.

A usage example is provided in form of two unit test demonstrating engine usage with and without a generic type argument.

[github-actions]

File	Coverage		Missing
All files	100%	✅

Minimum allowed coverage is 100%

Generated by 🐒 cobertura-action against b386058

[codspeed-hq]

CodSpeed Performance Report

Merging #265 will not alter performance

_{Comparing feat/generic-metadata (b386058) with main (fa183fb)}

Summary

✅ 1 untouched

[emyller]

Is the end goal of this PR to allow for specifying the metadata fields from the engine consumer side? e.g. to make SegmentMetadata(source: enum(api,identity_override), flagsmith_id: int) official?

I think we need to improve how this metadata specification is informed to SegmentContext. Right now it seems it's passed down the stack from EvaluationContext and it doesn't make sense if this metadata specification is only related to segments.

Or I may have failed to understand this PR correctly, sorry.

[khvn26]

Is the end goal of this PR to allow for specifying the metadata fields from the engine consumer side? e.g. to make SegmentMetadata(source: enum(api,identity_override), flagsmith_id: int) official?

It's, mainly, to avoid returning untyped/loosely typed data, and as a consequence, hard casts to/from SDK-side data (for examples, see here and here).

I think we need to improve how this metadata specification is informed to SegmentContext. Right now it seems it's passed down the stack from EvaluationContext and it doesn't make sense if this metadata specification is only related to segments.

This is how generics work in Python — any dependent class and interface should provide the generic type argument in order for it to work correctly. One way to improve clarity here would be to rename the generic type variable to SegmentMetadataT — done in b386058.

[emyller]

This is how generics work in Python — any dependent class and interface should provide the generic type argument in order for it to work correctly. One way to improve clarity here would be to rename the generic type variable to SegmentMetadataT — done in b386058.

Thanks, I think it does help.

I also think passing a generic type like this makes a lot of sense under certain scenarios, for example, serializers: ModelSerializer[ModelT] — it kind of reads like the model serializer exists for that type, which makes sense. We don't have this relation between EvaluationContext and SegmentMetadataT. It's currently difficult to me, for example, to envision how we could ever include specification to other metadata types, e.g. FeatureMetadataT, to this mix in the future. Can you please picture and share some example?

[khvn26]

We don't have this relation between EvaluationContext and SegmentMetadataT

We can observe it as we define specific evaluation contexts. In more robust typing system like TypeScript, SegmentMetadataT could be inferred automagically, e.g.:

let evalCtx = {
    environment: { key: "key", name: "Test" },
    identity: ...,
    segments: {
        "1": {
            key: "1",
            rules: [...],
            metadata: { hello: "world", id: 1 }
    }
}

result = getEvaluationResult(evalCtx);

result.segments[0].metadata  // { hello: str, id: Number } 

In Python, we still have to be explicit, but we can be fairly economical as well:

class SegmentMetadata(typing.TypedDict):
    flagsmith_id: NotRequired[int]
    """The ID of the segment used in Flagsmith API."""
    source: NotRequired[typing.Literal["api", "identity_overrides"]]
    """The source of the segment, e.g. 'api', 'identity_overrides'."""

SDKEvaluationContext = EvaluationContext[SegmentMetadata]
SDKEvaluationResult = EvaluationResult[SegmentMetadata]

(pulled from the actual usage https://github.com/Flagsmith/flagsmith-python-client/blob/ed21ef5c3aa1c3d2f4a7be4970e0aff8b3040c12/flagsmith/types.py#L40-L48)

how we could ever include specification to other metadata types, e.g. FeatureMetadataT

In Python only way would be to add a second type variable to the top interface, like so:

SDKEvaluationContext = EvaluationContext[SegmentMetadata, FeatureMetadata]
SDKEvaluationResult = EvaluationResult[SegmentMetadata, FeatureMetadata]


reveal_type(get_evaluation_result(SDKEvaluationContext(...))  # SDKEvaluationResult

The upside is that, thanks to PEP 696, these new type arguments will be optional and we'll be able to fall back to dict[str, object], like we already do for SegmentMetadataT — see here.

[emyller]

We don't have this relation between EvaluationContext and SegmentMetadataT

What I mean by this is:

• In one hand, we have the serializer existing for the model. The reading MySerializer[User] hints at that.
• In the other hand, EvaluationContext does not exist for SegmentMetadataT. It's somewhat the opposite.

In Python only way would be to add a second type variable to the top interface, like so:

SDKEvaluationContext = EvaluationContext[SegmentMetadata, FeatureMetadata]
SDKEvaluationResult = EvaluationResult[SegmentMetadata, FeatureMetadata]

The upside is that, thanks to PEP 696, these new type arguments will be optional and we'll be able to fall back to dict[str, object], like we already do for SegmentMetadataT — see here.

I think this PR solved a problem now, but it also creates a design problem for later. e.g. How to define a Context type that doesn't care about segment metadata, but does need FeatureMetadata? It looks like one will need to create a dummy generic type only to circumvent that. After some reflection, this actually feels very related to the ownership argument above.

Sorry I don't have a better idea that's friendly to static type checkers. My best suggestion right now is to not merge this, and keep things simpler, as metadata isn't actually critical.

I'm open to approve if you're adamant in keep going.

[khvn26]

How to define a Context type that doesn't care about segment metadata, but does need FeatureMetadata?

All of the following examples look ok to me:

# we don't care at all...
EvaluationContext[Any, MyFeatureMeta]
# or we don't care about its shape...
EvaluationContext[dict[str, Any], MyFeatureMeta]

# ...or, if we don't want to have Any in our codebase
EvaluationContext[object, MyFeatureMeta]

# ...we still can use this if we utterly completely do not care about any metadata
EvaluationContext 

It looks like one will need to create a dummy generic type only to circumvent that.

No, Any/object will suffice, if the engine consumer truly doesn't care.

Let me know if there's anything I'm missing here.

[emyller]

I think this adds more complexity than we need only to deal with static type checkers being incomplete IMO.

It's, mainly, to avoid returning untyped/loosely typed data, and as a consequence, hard casts to/from SDK-side data (for examples, see here and here).

I'd personally be happier with hard casts until Python improves — e.g. perhaps with named typed args, which could allow for EvaluationContext[segment_metadata=MySegmentMetadata] — or we find a better solution.

For now I'm not blocking this.