feat: Add the FDv2 data system and expose it through configuration#310
feat: Add the FDv2 data system and expose it through configuration#310kinyoklion wants to merge 4 commits into
Conversation
kinyoklion
force-pushed
the
rlamb/sdk-2186/fdv2-data-system
branch
3 times, most recently
from
9e8ec33 to
fff23d4
Compare
kinyoklion
force-pushed
the
rlamb/sdk-2186/fdv2-data-system
branch
from
fff23d4 to
6225462
Compare
| })).buildFactories()[const FDv2Streaming()]!; | ||
|
|
||
| final source = factory(_context()); | ||
| expect(source, isA<DataSource>()); |
There was a problem hiding this comment.
Should this expect the polling source in order to confirm it isn't the streaming one?
There was a problem hiding this comment.
They are all the orchestrator type. So it isn't quite that straightforward. It is more an e2e test, but we can make sure it does a poll and make sure it doesn't use SSE.
There was a problem hiding this comment.
The type-to-source mapping (e.g. a polling definition producing a polling source) is covered in entry_factories_test. What this test was actually missing is the data system's own job -- confirming the override is selected over the built-in; isA<DataSource>() passed either way. Updated it to assert resolvedDefinition(mode) returns the override for the overridden mode and the built-in for the others.
| // The common client loads cached flags into the flag store before | ||
| // the data source starts (FlagManager.loadCached during identify), | ||
| // so the cache is already applied by the time this chain runs. | ||
| // Reporting a miss advances the chain without re-applying it. |
There was a problem hiding this comment.
This comment seems to "know too much".
|
|
||
| DataSourceFactory _factoryForMode(ModeDefinition modeDefinition) { | ||
| return (LDContext context) { | ||
| if (!identical(context, _lastContext)) { |
There was a problem hiding this comment.
If you go from context A -> B -> A and there is no decoration via auto env or anonymous context decoration, could this be the same instance? If this function isn't invoked during the transition (offline mode?), the selector may not get cleared but context B's flags may be loaded from cache.
I'm hand waving a bit and haven't fully investigated.
Android had a context generation bug recently so seeing this makes me wonder if it can happen here.
There was a problem hiding this comment.
This isn't just used for identify. But I can see if this really needs to even know this.
There was a problem hiding this comment.
Reworked this so the basis reset no longer keys on the context instance in the factory. It is now driven from the data manager's identify path, keyed on the context canonical key, so it fires on any context change independent of the active mode (including offline) and independent of the factory being invoked at all -- which closes the instance-reuse / offline-transition gap. Added a regression test for the identify-change-identify sequence. See 7f96be5.
…ata source Split the identify strategy into FDv1/FDv2 data managers so cache handling lives with each protocol. FDv2 no longer loads the cache at identify; the data system reads it through a real cachedFlagsReader and feeds it into the pipeline. A PayloadEvent basis flag, set by the orchestrator, gates both the valid status (moved out of handlePayload) and wait-for-network resolution, so cached flags apply without reporting a live connection. Offline is now a real pipeline mode (cache initializer, no synchronizer) that loads cache while the manager keeps the offline status. Adds ConnectionModeId.offline and FlagManager.readCached.
…ctory The held selector was discarded inside the data source factory by comparing context instance identity, which only held while the factory was invoked for every context change. Move the reset to the data manager's identify path, keyed on the context canonical key, so a context change clears the basis regardless of the active connection mode (including offline) and independent of factory invocation timing. The factory no longer tracks the context.
The override test asserted only that a DataSource was produced, which holds whether or not the override is honored. Assert that resolvedDefinition picks the override for the overridden mode and the built-in otherwise. Translating a definition's entries into concrete sources stays covered by the entry-factory tests. Adds a visibleForTesting accessor and the meta dependency it needs.
2 participants
What this adds
The piece that ties the FDv2 building blocks together and turns them on:
FDv2DataSystem, the manager routing that applies FDv2 payloads, and the configuration that opts in. This completes the FDv2 work incommon_client— everything below the public Flutter API.FDv2DataSystemcomposes the data source factories theDataSourceManagerconsumes. It owns the state that must outlive any single orchestrator: the current selector and the context it belongs to. A fresh orchestrator is built per connection-mode switch and per identify; the selector survives mode switches (initializers are skipped when a selector is already held) but resets when the context changes, since a selector is specific to one context.buildFactories()returns factories for streaming, polling, and background — offline has no data source and is handled by the manager directly. Custom connection modes from config replace the built-in definitions by name.DataSourceManagerpayload routing. ThePayloadEventcase — a no-op placeholder in the orchestrator PR — now applies the change set throughhandlePayloadand completes the pending identify, the same wayDataEventdoes for FDv1.DataSystemConfig(providing it, even empty, opts into FDv2);LDCommonConfig.dataSystem;LDCommonClientconstructs anFDv2DataSystemand installs its factories whendataSystemis configured, otherwise the FDv1 sources run unchanged; and the new public types are exported.When
dataSystemis absent the FDv1 path is byte-for-byte unchanged, so existing behavior is unaffected.Testing
DataSourceManagertest: aPayloadEventis applied throughhandlePayload, marks the source valid, and completes identify (a dropped/no-op payload would leave identify hanging — this distinguishes the real routing from the placeholder).FDv2DataSystemtests:buildFactoriesexposes streaming/polling/background and not offline; each factory builds a fresh data source per call; a custom connection mode replaces the built-in definition.DataSystemConfigdefault (no custom modes).common_clientsuite passes;flutter_client_sdk(the dependent package) analyzes and tests clean against these changes.SDK-2186
Note
Medium Risk
Changes core flag acquisition wiring when
dataSystemis set and alters identify completion for payload events; default behavior is unchanged when config is omitted, but the new path touches initialization and data-source orchestration.Overview
Adds an EAP opt-in for FDv2: setting
LDCommonConfig.dataSystem(evenconst DataSystemConfig()) switches startup toFDv2DataSystemfactories instead of the existing FDv1 streaming/polling/background wiring; leavingdataSystemnull keeps the prior path.DataSystemConfigexposesConnectionModeId(streaming/polling/background) and optionalconnectionModesoverrides that replace built-inModeDefinitionpipelines.FDv2DataSystembuilds per-mode factories (fresh orchestrator per connection), persists selector across mode switches but resets it on context change, and skips initializers when a selector is already held.DataSourceManagernow routesPayloadEventthroughhandlePayloadand shares_completeIdentifywith FDv1DataEventhandling so identify completes when FDv2 payloads land. Public exports add the new config and mode-definition types.Tests cover payload→identify completion and
FDv2DataSystemfactory/override behavior.Reviewed by Cursor Bugbot for commit 6225462. Bugbot is set up for automated code reviews on this repo. Configure here.