diff --git a/ContractTests/Source/Controllers/SdkController.swift b/ContractTests/Source/Controllers/SdkController.swift index cbb63787..8bee1b2b 100644 --- a/ContractTests/Source/Controllers/SdkController.swift +++ b/ContractTests/Source/Controllers/SdkController.swift @@ -34,6 +34,7 @@ final class SdkController: RouteCollection { "event-gzip", "optional-event-gzip", "client-prereq-events", + "client-prereq-cycle-detection", "polling-gzip", "client-per-context-summaries" ] diff --git a/LaunchDarkly/LaunchDarkly/LDClientVariation.swift b/LaunchDarkly/LaunchDarkly/LDClientVariation.swift index 6b32d44a..d390e656 100644 --- a/LaunchDarkly/LaunchDarkly/LDClientVariation.swift +++ b/LaunchDarkly/LaunchDarkly/LDClientVariation.swift @@ -169,13 +169,36 @@ extension LDClient { } private func variationDetailInternal(_ flagKey: LDFlagKey, _ defaultValue: T, needsReason: Bool, methodName: String) -> LDEvaluationDetail where T: Decodable, T: LDValueConvertible { + var visited: Set? = nil + return variationDetailInternal(flagKey, defaultValue, needsReason: needsReason, methodName: methodName, visited: &visited) + } + + private func variationDetailInternal(_ flagKey: LDFlagKey, _ defaultValue: T, needsReason: Bool, methodName: String, visited: inout Set?) -> LDEvaluationDetail where T: Decodable, T: LDValueConvertible { return evaluateWithHooks(flagKey: flagKey, defaultValue: defaultValue, methodName: methodName) { var result: LDEvaluationDetail let featureFlag = flagStore.featureFlag(for: flagKey) if let featureFlag = featureFlag { - featureFlag.prerequisites?.forEach { prereqFlagKey in - // recurse on prerequisites to emulate prereq evaluations occurring with desirable side effects such as events for prereqs - _ = variationDetailInternal(prereqFlagKey, LDValue.null, needsReason: needsReason, methodName: methodName) + if let prerequisites = featureFlag.prerequisites, !prerequisites.isEmpty { + // Recurse on prerequisites to emulate prereq evaluations occurring with desirable side effects + // such as events for prereqs. + // + // `visited` tracks the chain of prerequisite dependencies from the top-level evaluation to + // (but not including) the current flag. The set is allocated lazily: it stays `nil` until we + // descend into the first flag whose `prerequisites` are non-empty, so a variation call on a + // leaf flag pays no heap allocation for cycle bookkeeping. + if visited == nil { + visited = Set() + } + visited!.insert(flagKey) + defer { visited!.remove(flagKey) } + for prereqFlagKey in prerequisites { + if visited!.contains(prereqFlagKey) { + // Cyclic edge: skip descent, continue with remaining prerequisites at this level. + // The requested flag's value and reason (below) are unaffected. + continue + } + _ = variationDetailInternal(prereqFlagKey, LDValue.null, needsReason: needsReason, methodName: methodName, visited: &visited) + } } if featureFlag.value == .null { diff --git a/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift b/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift index 7590ae2e..f4901cfa 100644 --- a/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift +++ b/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift @@ -799,6 +799,69 @@ final class LDClientSpec: QuickSpec { expect(events[7].key) == "flagABD" } } + // Cycle-detection tests exercise the ancestor-set cycle guard added to + // variationDetailInternal. Prior to that guard, any of these flag configurations would + // cause unbounded recursion during a variation() call. The tests set up a cyclic + // prerequisite graph and evaluate one flag on the cycle, asserting (a) the SDK returns + // the flag's cached value unchanged and (b) the emitted evaluation events reflect + // exactly one recording per cycle-safe descent. + context("flag store contains cyclic prerequisites") { + var events = [FeatureEvent]() + beforeEach { + events = [] + testContext.eventReporterMock.recordFlagEvaluationEventsCallback = { + let args = testContext.eventReporterMock.recordFlagEvaluationEventsReceivedArguments! + events.append(FeatureEvent(key: args.flagKey, context: args.context, value: args.value, defaultValue: args.defaultValue, featureFlag: args.featureFlag, includeReason: args.includeReason, isDebug: false)) + } + } + it("skips self-loop prerequisite and returns cached value") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA])) + // Requirement 1.2.5.1: the requested flag's cached value is returned unchanged. + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // The self-prereq is cycle-skipped, so only the top-level evaluation records an event. + expect(events.map { $0.key }) == ["flagA"] + } + it("handles a two-cycle evaluating A") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // A -> B -> [A skipped]. Events emitted deepest-first: B (as prereq of A), then A. + expect(events.map { $0.key }) == ["flagB", "flagA"] + } + it("handles a two-cycle evaluating B") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB])) + expect(testContext.subject.boolVariation(forKey: "flagB", defaultValue: false)) == true + // Symmetric: same graph, entry from B. Events: A (as prereq of B), then B. + expect(events.map { $0.key }) == ["flagA", "flagB"] + } + it("handles a three-cycle") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagC"]) + let flagC = FeatureFlag(flagKey: "flagC", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB, "flagC": flagC])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // A -> B -> C -> [A skipped]. Events emitted deepest-first: C, B, A. + expect(events.map { $0.key }) == ["flagC", "flagB", "flagA"] + } + it("emits the shared descendant once per path in a non-cyclic diamond") { + // Diamond: A -> [B, C], B -> [D], C -> [D]. Not a cycle. Ancestor-set (current-path) + // semantics must let D be reached on each of the two independent paths — so D emits + // twice. A naive "visited across the whole walk" implementation would incorrectly + // count D only once; this case guards against that regression. + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB", "flagC"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagD"]) + let flagC = FeatureFlag(flagKey: "flagC", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagD"]) + let flagD = FeatureFlag(flagKey: "flagD", value: LDValue.bool(true), trackEvents: false, trackReason: false) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB, "flagC": flagC, "flagD": flagD])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // Events (deepest-first per path): D (via B), B, D (via C), C, A. D appears twice. + expect(events.map { $0.key }) == ["flagD", "flagB", "flagD", "flagC", "flagA"] + } + } } }