Skip to content
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

fix: enriched context lost on some events #1226

Merged
merged 3 commits into from
Feb 18, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -184,19 +184,6 @@ public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultVa
return flagResolver.objectEvaluation(key, defaultValue, ctx);
}

/**
* An unmodifiable view of a Structure representing the latest result of the
* SyncMetadata.
* Set on initial connection and updated with every reconnection.
* see:
* https://buf.build/open-feature/flagd/docs/main:flagd.sync.v1#flagd.sync.v1.FlagSyncService.GetMetadata
*
* @return Object map representing sync metadata
*/
protected Structure getSyncMetadata() {
return new ImmutableStructure(eventsLock.syncMetadata.asMap());
}

/**
* The updated context mixed into all evaluations based on the sync-metadata.
*
Expand All @@ -211,10 +198,6 @@ private void onProviderEvent(FlagdProviderEvent flagdProviderEvent) {

synchronized (eventsLock) {
log.info("FlagdProviderEvent: {}", flagdProviderEvent.getEvent());
eventsLock.syncMetadata = flagdProviderEvent.getSyncMetadata();
if (flagdProviderEvent.getSyncMetadata() != null) {
eventsLock.enrichedContext = contextEnricher.apply(flagdProviderEvent.getSyncMetadata());
}

/*
* We only use Error and Ready as previous states.
Expand All @@ -233,6 +216,10 @@ private void onProviderEvent(FlagdProviderEvent flagdProviderEvent) {
}
// intentional fall through, a not-ready change will trigger a ready.
case PROVIDER_READY:
// sync metadata is used to enrich the context, and is immutable in flagd, so only need to be fetched onces at READY
if (flagdProviderEvent.getSyncMetadata() != null) {
eventsLock.enrichedContext = contextEnricher.apply(flagdProviderEvent.getSyncMetadata());
}
onReady();
eventsLock.previousEvent = ProviderEvent.PROVIDER_READY;
break;
Expand Down Expand Up @@ -304,7 +291,6 @@ private void onError() {
*/
static class EventsLock {
volatile ProviderEvent previousEvent = null;
volatile Structure syncMetadata = new ImmutableStructure();
volatile boolean initialized = false;
volatile EvaluationContext enrichedContext = new ImmutableContext();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,6 @@ void updatesSyncMetadataWithCallback() throws Exception {
provider.initialize(ctx);

// the onConnectionEvent should have updated the sync metadata and the
assertEquals(val, provider.getSyncMetadata().getValue(key).asString());
assertEquals(val, provider.getEnrichedContext().getValue(key).asString());

// call the hook manually and make sure the enriched context is returned
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
@ConfigurationParameter(key = GLUE_PROPERTY_NAME, value = "dev.openfeature.contrib.providers.flagd.e2e.steps")
@ConfigurationParameter(key = OBJECT_FACTORY_PROPERTY_NAME, value = "io.cucumber.picocontainer.PicoFactory")
@IncludeTags("file")
@ExcludeTags({"unixsocket", "targetURI", "reconnect", "customCert", "events", "flagdcontext"})
@ExcludeTags({"unixsocket", "targetURI", "reconnect", "customCert", "events", "contextEnrichment"})
@Testcontainers
public class RunFileTest {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
@Slf4j
public class EventSteps extends AbstractSteps {

private static final int EVENT_TIMEOUT_MS = 12_000;

public EventSteps(State state) {
super(state);
state.events = new LinkedList<>();
Expand Down Expand Up @@ -47,12 +49,12 @@ public void a_stale_event_handler(String eventType) {

@When("a {} event was fired")
public void eventWasFired(String eventType) throws InterruptedException {
eventHandlerShouldBeExecutedWithin(eventType, 8000);
eventHandlerShouldBeExecutedWithin(eventType, EVENT_TIMEOUT_MS);
}

@Then("the {} event handler should have been executed")
public void eventHandlerShouldBeExecuted(String eventType) {
eventHandlerShouldBeExecutedWithin(eventType, 10000);
eventHandlerShouldBeExecutedWithin(eventType, EVENT_TIMEOUT_MS);
}

@Then("the {} event handler should have been executed within {int}ms")
Expand Down
2 changes: 1 addition & 1 deletion providers/flagd/test-harness
Loading