From c78c612c5cd3fab001ff0716cb4a68fc5a93f28f Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 13 Aug 2026 13:31:23 -0700 Subject: [PATCH] feat: add HookDecorator so a hook wrapper forwards the stages it does not override A hook's stages each default to doing nothing, so a wrapper that omits one swallows it rather than passing it on: wrapping a hook meant reimplementing beforeIdentify, afterIdentify, and afterTrack as pure forwarding just to keep them working, and a wrapper that forgot dropped them silently. HookDecorator forwards every stage, so a subclass overrides only the stages it changes. DedupingHook extends it and is down to the three it changes. The wrapped hook is private to the decorator, reachable only through super, so nothing can unwrap a decorator to decide what to do with what it finds. The identify and track stages are @CallSuper, so lint reports an override that stops forwarding one: a decorator wrapping a DedupingHook could otherwise keep it from being told to forget what it has reported, and it would go on suppressing across an identify. The evaluation stages are not, because suppressing an evaluation series is what a decorator is for. Also drops the remaining suggestions to subclass from the deduper docs, which the iOS SDK had already removed. Co-authored-by: Cursor --- .../android/integrations/DedupingHook.java | 46 +---- .../EvaluationExposureDeduper.java | 6 +- .../sdk/android/integrations/Hook.java | 6 +- .../android/integrations/HookDecorator.java | 172 ++++++++++++++++++ .../integrations/DedupingHookTest.java | 34 ++-- 5 files changed, 207 insertions(+), 57 deletions(-) create mode 100644 launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/HookDecorator.java diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/DedupingHook.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/DedupingHook.java index 0307bc21..a00562ae 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/DedupingHook.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/DedupingHook.java @@ -28,12 +28,11 @@ * .addHook(new MetricsHook()) // observes every evaluation * .addHook(new DedupingHook(new ObservabilityHook())) // default window * .addHook(new DedupingHook(new TelemetryHook(), 60_000)) - * .addHook(new DedupingHook(new ExperimentHook(), myCustomDeduper)) + * .addHook(new DedupingHook(new ExperimentHook(), sharedDeduper)) * *

* Two evaluations resolve to the same result when they agree on everything - * {@link EvaluationExposureKey} describes. Pass your own {@link EvaluationExposureDeduper} subclass to - * decide that differently. + * {@link EvaluationExposureKey} describes. *

* An evaluation the SDK has no flag data for resolves to the default value, and is the same result as * another that does. Evaluations made before the client has flags are of that kind, so the wrapped @@ -58,7 +57,7 @@ * it stored in its own before stage. A decorator inside this one is unaffected, since a suppressed * evaluation never reaches it. */ -public final class DedupingHook extends Hook { +public final class DedupingHook extends HookDecorator { /** * Reads the clock a window is measured against. Exists so that tests can control it; the SDK has @@ -80,7 +79,6 @@ interface Clock { // Namespaced because it travels in series data that the wrapped hook may also write to. private static final String SUPPRESSED = "com.launchdarkly.sdk.android.DedupingHook.suppressed"; - private final Hook delegate; private final EvaluationExposureDeduper deduper; private final Clock clock; @@ -117,20 +115,11 @@ public DedupingHook(Hook delegate, EvaluationExposureDeduper deduper) { @VisibleForTesting DedupingHook(Hook delegate, EvaluationExposureDeduper deduper, Clock clock) { - super(nameOf(delegate)); - this.delegate = Objects.requireNonNull(delegate, "a deduping hook must wrap a hook"); + super(delegate); this.deduper = Objects.requireNonNull(deduper, "a deduping hook must have a deduper"); this.clock = clock; } - /** - * @return the wrapped hook's metadata, so that the SDK names the hook a stage belongs to - */ - @Override - public HookMetadata getMetadata() { - return delegate.getMetadata(); - } - /** * Forwards the evaluation unless the wrapped hook has just been told about the same result. *

@@ -148,7 +137,7 @@ public Map beforeEvaluation(EvaluationSeriesContext seriesContex if (key != null && !deduper.shouldRecord(key, clock.elapsedMillis())) { return suppressedSeriesData; } - return delegate.beforeEvaluation(seriesContext, seriesData); + return super.beforeEvaluation(seriesContext, seriesData); } /** @@ -165,7 +154,7 @@ public Map afterEvaluation(EvaluationSeriesContext seriesContext if (seriesData != null && seriesData.get(SUPPRESSED) == this) { return seriesData; } - return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail); + return super.afterEvaluation(seriesContext, seriesData, evaluationDetail); } /** @@ -183,27 +172,6 @@ public Map afterEvaluation(EvaluationSeriesContext seriesContext @Override public Map beforeIdentify(IdentifySeriesContext seriesContext, Map seriesData) { deduper.reset(); - return delegate.beforeIdentify(seriesContext, seriesData); - } - - @Override - public Map afterIdentify(IdentifySeriesContext seriesContext, Map seriesData, - IdentifySeriesResult result) { - return delegate.afterIdentify(seriesContext, seriesData, result); - } - - @Override - public void afterTrack(TrackSeriesContext seriesContext) { - delegate.afterTrack(seriesContext); - } - - // Static because it runs in the super() call, before this instance exists. Tolerates a hook whose - // metadata throws, which the SDK reports rather than propagates. - private static String nameOf(Hook delegate) { - try { - return delegate == null ? null : delegate.getMetadata().getName(); - } catch (Exception e) { - return null; - } + return super.beforeIdentify(seriesContext, seriesData); } } diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/EvaluationExposureDeduper.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/EvaluationExposureDeduper.java index 44b0ec50..879488b2 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/EvaluationExposureDeduper.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/EvaluationExposureDeduper.java @@ -16,16 +16,14 @@ * .addHook(new MetricsHook()) // told about every evaluation * .addHook(new DedupingHook(new ObservabilityHook())) // default window * .addHook(new DedupingHook(new TelemetryHook(), 30_000)) - * .addHook(new DedupingHook(new ExperimentHook(), myCustomDeduper)) + * .addHook(new DedupingHook(new ExperimentHook(), sharedDeduper)) * *

* This class is the SDK's implementation: it remembers the result each flag last reported, and tells * the hook about the flag again as soon as that result changes, or once the window elapses while it * stays the same. Tracking one result per flag rather than every result seen keeps a flag that flips * back and forth from hiding the flips, and holds one record per flag the application evaluates, so - * the window is the only thing there is to configure. Subclass this to implement a different policy; - * only {@link #shouldRecord(EvaluationExposureKey, long)} and {@link #reset()} are called by - * {@link DedupingHook}. + * the window is the only thing there is to configure. *

* A deduper is consulted once per evaluation, before the series opens, so a suppressed evaluation * invokes neither {@code beforeEvaluation} nor {@code afterEvaluation}. Implementations must be diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/Hook.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/Hook.java index 841ab606..13e51862 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/Hook.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/Hook.java @@ -13,8 +13,10 @@ * stages in the order they were configured, and each hook's after stages in reverse order. (i.e. * myHook1.beforeEvaluation, myHook2.beforeEvaluation, myHook2.afterEvaluation, myHook1.afterEvaluation) *

- * To deduplicate the repeated evaluations observed by one hook, wrap it in a - * {@link DedupingHook} and register the wrapper. + * Note that each stage below has a default implementation that does nothing, so a hook implements only + * the stages it cares about. A {@link HookDecorator} wraps a hook and forwards every stage to it, which + * is what a hook that adds behavior to another hook is built on. To deduplicate the repeated + * evaluations observed by one hook, wrap it in a {@link DedupingHook} and register the wrapper. */ public abstract class Hook { diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/HookDecorator.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/HookDecorator.java new file mode 100644 index 00000000..7a6cbc6b --- /dev/null +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/HookDecorator.java @@ -0,0 +1,172 @@ +package com.launchdarkly.sdk.android.integrations; + +import androidx.annotation.CallSuper; + +import com.launchdarkly.sdk.EvaluationDetail; +import com.launchdarkly.sdk.LDValue; + +import java.util.Map; +import java.util.Objects; + +/** + * A hook that wraps another hook, forwarding every stage to it. A subclass adds behavior to a hook + * without changing it, and is registered in place of the hook it wraps. + *

+ * Every stage forwards to the wrapped hook, so a subclass overrides only the stages it changes, and + * calls {@code super} to forward the ones it does. The stages it leaves alone still reach the wrapped + * hook. + *

+ * An override that never calls {@code super} stops forwarding that stage, which for the identify and + * track stages is a decorator swallowing something it has no reason to: a {@link DedupingHook} inside + * such an override would stop being told to forget what it has reported, and would go on suppressing + * across an identify. Those three stages are {@link CallSuper}, so Android Lint reports an override of + * one that does not forward. The evaluation stages are not, because suppressing an evaluation series is + * what a decorator is for. + * + * 


+ *     public final class FlagFilteringHook extends HookDecorator {
+ *         private final Set<String> flagKeys;
+ *
+ *         public FlagFilteringHook(Hook delegate, Set<String> flagKeys) {
+ *             super(delegate);
+ *             this.flagKeys = flagKeys;
+ *         }
+ *
+ *         @Override
+ *         public Map<String, Object> beforeEvaluation(EvaluationSeriesContext seriesContext,
+ *                                                     Map<String, Object> seriesData) {
+ *             return flagKeys.contains(seriesContext.flagKey)
+ *                     ? super.beforeEvaluation(seriesContext, seriesData)
+ *                     : seriesData;
+ *         }
+ *     }
+ *
+ *

+ * That hook filters evaluations and still forwards identify and track, which it never mentions. + *

+ * {@link DedupingHook} is the decorator the SDK ships: it forwards an evaluation series only when the + * flag's result is one its hook has not just been told about. + *

+ * Decorators stack, so a hook may be wrapped in as many as it needs, each wrapping the one inside it: + * + * 


+ *     Components.hooks()
+ *         .addHook(new DedupingHook(new FlagFilteringHook(new ObservabilityHook(), myFlagKeys)))
+ *
+ *

+ * A decorator reports the wrapped hook's metadata as its own, so the SDK names the hook that a stage + * belongs to rather than the wrappers around it. + *

+ * A decorator that suppresses a stage must suppress the whole evaluation series, because hooks pair + * their stages: an observability hook opens a span in + * {@link Hook#beforeEvaluation(EvaluationSeriesContext, Map)} and closes it in + * {@link Hook#afterEvaluation(EvaluationSeriesContext, Map, EvaluationDetail)}, so suppressing only + * the after stage leaves that span open. To carry the decision from one stage to the other, return + * series data the after stage recognizes, the way {@link DedupingHook} does. + *

+ * A decorator that does that belongs outermost, because the series data it returns replaces what it + * was given: a decorator outside it does not get back what it stored in its own before stage. + *

+ * This class is not stable, and not subject to any backwards compatibility guarantees or semantic versioning. + * It is experimental. + */ +public abstract class HookDecorator extends Hook { + + // Reachable only through super calls. Handing the wrapped hook back out invites code to unwrap a + // decorator and decide what to do with what it finds, when a decorator is meant to stand in for the + // hook it wraps. + private final Hook delegate; + + /** + * Wraps a hook, taking its name as this decorator's own. + * + * @param delegate the hook to forward each stage to + */ + protected HookDecorator(Hook delegate) { + super(nameOf(delegate)); + this.delegate = Objects.requireNonNull(delegate, "a decorator must wrap a hook"); + } + + /** + * @return the wrapped hook's metadata, so that the SDK names the hook a stage belongs to + */ + @Override + public HookMetadata getMetadata() { + return delegate.getMetadata(); + } + + /** + * Forwards the stage to the wrapped hook. + * + * @param seriesContext container of parameters associated with this evaluation + * @param seriesData immutable data from the previous stage in the evaluation series + * @return the wrapped hook's series data + */ + @Override + public Map beforeEvaluation(EvaluationSeriesContext seriesContext, Map seriesData) { + return delegate.beforeEvaluation(seriesContext, seriesData); + } + + /** + * Forwards the stage to the wrapped hook. + * + * @param seriesContext container of parameters associated with this evaluation + * @param seriesData data from the previous stage in the evaluation series + * @param evaluationDetail the result of the evaluation + * @return the wrapped hook's series data + */ + @Override + public Map afterEvaluation(EvaluationSeriesContext seriesContext, Map seriesData, + EvaluationDetail evaluationDetail) { + return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail); + } + + /** + * Forwards the stage to the wrapped hook. + * + * @param seriesContext container of parameters associated with this identify + * @param seriesData immutable data from the previous stage in the identify series + * @return the wrapped hook's series data + */ + @CallSuper + @Override + public Map beforeIdentify(IdentifySeriesContext seriesContext, Map seriesData) { + return delegate.beforeIdentify(seriesContext, seriesData); + } + + /** + * Forwards the stage to the wrapped hook. + * + * @param seriesContext container of parameters associated with this identify + * @param seriesData data from the previous stage in the identify series + * @param result the result of the identify operation + * @return the wrapped hook's series data + */ + @CallSuper + @Override + public Map afterIdentify(IdentifySeriesContext seriesContext, Map seriesData, + IdentifySeriesResult result) { + return delegate.afterIdentify(seriesContext, seriesData, result); + } + + /** + * Forwards the stage to the wrapped hook. + * + * @param seriesContext container of parameters associated with this track + */ + @CallSuper + @Override + public void afterTrack(TrackSeriesContext seriesContext) { + delegate.afterTrack(seriesContext); + } + + // Static because it runs in the super() call, before this instance exists. Tolerates a hook whose + // metadata throws, which the SDK reports rather than propagates. + private static String nameOf(Hook delegate) { + try { + return delegate == null ? null : delegate.getMetadata().getName(); + } catch (Exception e) { + return null; + } + } +} diff --git a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/integrations/DedupingHookTest.java b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/integrations/DedupingHookTest.java index c5b8331c..356b1bbb 100644 --- a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/integrations/DedupingHookTest.java +++ b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/integrations/DedupingHookTest.java @@ -87,33 +87,30 @@ public void afterTrack(TrackSeriesContext seriesContext) { } } - /** A wrapper with its own behavior, to check that hook wrappers compose. */ - private static class CountingDecorator extends Hook { - private final Hook delegate; + /** + * A wrapper with its own behavior, to check that hook wrappers compose. + *

+ * Overrides only the evaluation stages, as a wrapper a customer writes would: the rest are inherited. + */ + private static class CountingDecorator extends HookDecorator { int evaluationsForwarded = 0; int resultsForwarded = 0; CountingDecorator(Hook delegate) { - super(delegate.getMetadata().getName()); - this.delegate = delegate; - } - - @Override - public HookMetadata getMetadata() { - return delegate.getMetadata(); + super(delegate); } @Override public Map beforeEvaluation(EvaluationSeriesContext seriesContext, Map seriesData) { evaluationsForwarded++; - return delegate.beforeEvaluation(seriesContext, seriesData); + return super.beforeEvaluation(seriesContext, seriesData); } @Override public Map afterEvaluation(EvaluationSeriesContext seriesContext, Map seriesData, EvaluationDetail evaluationDetail) { resultsForwarded++; - return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail); + return super.afterEvaluation(seriesContext, seriesData, evaluationDetail); } } @@ -266,6 +263,19 @@ public void forwardsTheStagesItDoesNotDeduplicate() { assertEquals(List.of("beforeIdentify", "afterIdentify", "afterTrack"), hook.stages); } + @Test + public void aDecoratorForwardsTheStagesItDoesNotOverride() { + RecordingHook hook = new RecordingHook("decorated"); + HookRunner runner = runner(EXPOSURE_KEY, new CountingDecorator(hook)); + + identify(runner); + runner.afterTrack("event-key", LDContext.create("user-123"), LDValue.ofNull(), null); + + // The decorator mentions neither identify nor track, and the hook it wraps is still told about + // both: a stage a decorator leaves alone is forwarded rather than dropped. + assertEquals(List.of("beforeIdentify", "afterIdentify", "afterTrack"), hook.stages); + } + @Test public void forwardsAnEvaluationWhoseResultTheSdkDidNotDescribe() { RecordingHook hook = new RecordingHook("deduping");