Skip to content

Commit fcab81f

Browse files
tanderson-ldtanderson-ld
authored
fix: improves handling of invalid contexts and adds SSE Client logging. (#207)
**Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** SDK-1413 **Describe the solution you've provided** Adds context validity checks to start and identify that prevents state change if invalid context is provided. Adds logging to SSE client and necessary adapter to use LDLogger.
1 parent d2878b1 commit fcab81f

21 files changed

+407
-45
lines changed

‎packages/common/test/ld_context_test.dart

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,22 @@ void main() {
6363
expect(LDContextBuilder().build().canonicalKey, '');
6464
});
6565

66+
test('can modify an invalid context to become valid', () {
67+
// Start with an invalid context (no kind specified)
68+
final invalidContext = LDContextBuilder().build();
69+
expect(invalidContext.valid, false);
70+
expect(invalidContext.canonicalKey, '');
71+
72+
// Modify it to become valid by adding a valid kind
73+
final validContext = LDContextBuilder.fromContext(invalidContext)
74+
.kind('user', 'user-key')
75+
.build();
76+
77+
expect(validContext.valid, true);
78+
expect(validContext.canonicalKey, 'user-key');
79+
expect(validContext.keys, <String, String>{'user': 'user-key'});
80+
});
81+
6682
test('can change the key of a context during build', () {
6783
final context = LDContextBuilder()
6884
.kind('user', 'user-key')

‎packages/common_client/lib/src/context_modifiers/anonymous_context_modifier.dart

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,20 @@ const _anonContextKeyNamespace = 'LaunchDarkly_AnonContextKey';
88

99
final class AnonymousContextModifier implements ContextModifier {
1010
final Persistence _persistence;
11+
final LDLogger _logger;
1112

12-
AnonymousContextModifier(Persistence persistence)
13-
: _persistence = persistence;
13+
AnonymousContextModifier(Persistence persistence, LDLogger logger)
14+
: _persistence = persistence,
15+
_logger = logger;
1416

1517
/// For any anonymous contexts, which do not have keys specified, generate
1618
/// or read a persisted key for the anonymous kinds present. If persistence
1719
/// is available, then the key will be stable.
1820
@override
1921
Future<LDContext> decorate(LDContext context) async {
2022
if (!context.valid) {
21-
return context;
23+
_logger.warn(
24+
'AnonymousContextModifier was asked to modify an invalid context and will attempt to do so. This is expected if starting with an empty context.');
2225
}
2326
// Before we make a builder we should check if any anonymous contexts
2427
// without keys exist.

‎packages/common_client/lib/src/context_modifiers/env_context_modifier.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ final class AutoEnvContextModifier implements ContextModifier {
4242

4343
@override
4444
Future<LDContext> decorate(LDContext context) async {
45+
if (!context.valid) {
46+
_logger.warn(
47+
'AutoEnvContextModifier was asked to modify an invalid context and will attempt to do so. This is expected if starting with an empty context.');
48+
}
49+
4550
final builder = LDContextBuilder.fromContext(context);
4651

4752
for (final recipe in _recipes) {

‎packages/common_client/lib/src/data_sources/streaming_data_source.dart

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,20 @@ import 'data_source_status.dart';
1111

1212
typedef MessageHandler = void Function(MessageEvent);
1313
typedef ErrorHandler = void Function(dynamic);
14-
typedef SseClientFactory = SSEClient Function(Uri uri,
15-
HttpProperties httpProperties, String? body, SseHttpMethod? method);
14+
typedef SseClientFactory = SSEClient Function(
15+
Uri uri,
16+
HttpProperties httpProperties,
17+
String? body,
18+
SseHttpMethod? method,
19+
EventSourceLogger? logger);
1620

1721
SSEClient _defaultClientFactory(Uri uri, HttpProperties httpProperties,
18-
String? body, SseHttpMethod? method) {
22+
String? body, SseHttpMethod? method, EventSourceLogger? logger) {
1923
return SSEClient(uri, {'put', 'patch', 'delete'},
2024
headers: httpProperties.baseHeaders,
2125
body: body,
22-
httpMethod: method ?? SseHttpMethod.get);
26+
httpMethod: method ?? SseHttpMethod.get,
27+
logger: logger);
2328
}
2429

2530
final class StreamingDataSource implements DataSource {
@@ -109,7 +114,8 @@ final class StreamingDataSource implements DataSource {
109114
_uri,
110115
_httpProperties,
111116
_useReport ? _contextString : null,
112-
_useReport ? SseHttpMethod.report : SseHttpMethod.get);
117+
_useReport ? SseHttpMethod.report : SseHttpMethod.get,
118+
LDLoggerToEventSourceAdapter(_logger));
113119

114120
_subscription = _client!.stream.listen((event) async {
115121
if (_stopped) {
@@ -147,3 +153,22 @@ final class StreamingDataSource implements DataSource {
147153
_dataController.close();
148154
}
149155
}
156+
157+
/// Adapter to convert LDLogger to EventSourceLogger
158+
class LDLoggerToEventSourceAdapter implements EventSourceLogger {
159+
final LDLogger _logger;
160+
161+
LDLoggerToEventSourceAdapter(this._logger);
162+
163+
@override
164+
void debug(String message) => _logger.debug(message);
165+
166+
@override
167+
void info(String message) => _logger.info(message);
168+
169+
@override
170+
void warn(String message) => _logger.warn(message);
171+
172+
@override
173+
void error(String message) => _logger.error(message);
174+
}

‎packages/common_client/lib/src/ld_common_client.dart

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ final class LDCommonClient {
308308
_envReport = await _makeEnvironmentReport();
309309

310310
// set up context modifiers, adding the auto env modifier if turned on
311-
_modifiers = [AnonymousContextModifier(_persistence)];
311+
_modifiers = [AnonymousContextModifier(_persistence, _logger)];
312312
if (_config.autoEnvAttributes == AutoEnvAttributes.enabled) {
313313
_modifiers.add(
314314
AutoEnvContextModifier(_envReport, _persistence, _config.logger));
@@ -440,6 +440,13 @@ final class LDCommonClient {
440440

441441
Future<void> _identifyInternal(LDContext context,
442442
{bool waitForNetworkResults = false}) async {
443+
if (!context.valid) {
444+
const message =
445+
'LDClient was provided an invalid context. The context will be ignored. Existing flags will be used for evaluations until identify is called with a valid context.';
446+
_logger.warn(message);
447+
throw Exception(message);
448+
}
449+
443450
await _setAndDecorateContext(context);
444451
final completer = Completer<void>();
445452
_eventProcessor?.processIdentifyEvent(IdentifyEvent(context: _context));

‎packages/common_client/test/context_decorators/anonymous_context_modifier_test.dart

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,22 @@
11
import 'package:launchdarkly_common_client/launchdarkly_common_client.dart';
22
import 'package:launchdarkly_common_client/src/context_modifiers/anonymous_context_modifier.dart';
33
import 'package:launchdarkly_common_client/src/persistence/persistence.dart';
4+
import 'package:launchdarkly_dart_common/launchdarkly_dart_common.dart';
45
import 'package:test/test.dart';
6+
import 'package:mocktail/mocktail.dart';
57

68
import '../mock_persistence.dart';
79

10+
class MockAdapter extends Mock implements LDLogAdapter {}
11+
812
void main() {
13+
setUpAll(() {
14+
registerFallbackValue(LDLogRecord(
15+
level: LDLogLevel.debug,
16+
message: '',
17+
time: DateTime.now(),
18+
logTag: ''));
19+
});
920
group('without persistence', () {
1021
test('it populates keys for anonymous contexts that lack them', () async {
1122
final context = LDContextBuilder()
@@ -16,7 +27,8 @@ void main() {
1627
.anonymous(true)
1728
.build();
1829

19-
final decorator = AnonymousContextModifier(InMemoryPersistence());
30+
final decorator =
31+
AnonymousContextModifier(InMemoryPersistence(), LDLogger());
2032
final decoratedContext = await decorator.decorate(context);
2133

2234
expect(decoratedContext.attributesByKind['user']!.key, isNotEmpty);
@@ -33,7 +45,8 @@ void main() {
3345
.anonymous(true)
3446
.build();
3547

36-
final decorator = AnonymousContextModifier(InMemoryPersistence());
48+
final decorator =
49+
AnonymousContextModifier(InMemoryPersistence(), LDLogger());
3750
final decoratedContext = await decorator.decorate(context);
3851

3952
expect(decoratedContext.attributesByKind['user']!.key,
@@ -49,7 +62,8 @@ void main() {
4962
.anonymous(true)
5063
.build();
5164

52-
final decorator = AnonymousContextModifier(InMemoryPersistence());
65+
final decorator =
66+
AnonymousContextModifier(InMemoryPersistence(), LDLogger());
5367
final decoratedContext = await decorator.decorate(context);
5468
final decoratedContext2 = await decorator.decorate(context);
5569

@@ -74,7 +88,7 @@ void main() {
7488
encodePersistenceKey('user'): 'the-user-key',
7589
encodePersistenceKey('company'): 'the-company-key',
7690
};
77-
final decorator = AnonymousContextModifier(mockPersistence);
91+
final decorator = AnonymousContextModifier(mockPersistence, LDLogger());
7892

7993
final decoratedContext = await decorator.decorate(context);
8094

@@ -92,7 +106,7 @@ void main() {
92106
.build();
93107

94108
final mockPersistence = MockPersistence();
95-
final decorator = AnonymousContextModifier(mockPersistence);
109+
final decorator = AnonymousContextModifier(mockPersistence, LDLogger());
96110

97111
final decoratedContext = await decorator.decorate(context);
98112

@@ -106,4 +120,25 @@ void main() {
106120
encodePersistenceKey('company')]);
107121
});
108122
});
123+
124+
group('invalid context handling', () {
125+
test('it logs a info log when asked to modify an invalid context',
126+
() async {
127+
final invalidContext =
128+
LDContextBuilder().build(); // This creates an invalid context
129+
final mockAdapter = MockAdapter();
130+
final logger = LDLogger(adapter: mockAdapter, level: LDLogLevel.info);
131+
final decorator = AnonymousContextModifier(InMemoryPersistence(), logger);
132+
133+
final result = await decorator.decorate(invalidContext);
134+
135+
expect(result.valid, false);
136+
137+
final logRecord = verify(() => mockAdapter.log(captureAny())).captured[0]
138+
as LDLogRecord;
139+
expect(logRecord.level, LDLogLevel.warn);
140+
expect(logRecord.message,
141+
'AnonymousContextModifier was asked to modify an invalid context and will attempt to do so. This is expected if starting with an empty context.');
142+
});
143+
});
109144
}

‎packages/common_client/test/context_decorators/env_context_modifier_test.dart

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,19 @@ import 'package:launchdarkly_common_client/src/context_modifiers/env_context_mod
22
import 'package:launchdarkly_common_client/src/persistence/persistence.dart';
33
import 'package:launchdarkly_dart_common/launchdarkly_dart_common.dart';
44
import 'package:test/test.dart';
5+
import 'package:mocktail/mocktail.dart';
6+
7+
class MockAdapter extends Mock implements LDLogAdapter {}
58

69
void main() {
10+
setUpAll(() {
11+
registerFallbackValue(LDLogRecord(
12+
level: LDLogLevel.debug,
13+
message: '',
14+
time: DateTime.now(),
15+
logTag: ''));
16+
});
17+
718
group('env reporter with various configurations', () {
819
test('reporter has all attributes', () async {
920
final mockPersistence = InMemoryPersistence();
@@ -449,4 +460,66 @@ void main() {
449460
expect(key1, key2);
450461
});
451462
});
463+
464+
group('invalid context handling', () {
465+
test('it logs an info log when asked to modify an invalid context',
466+
() async {
467+
final invalidContext =
468+
LDContextBuilder().build(); // This creates an invalid context
469+
final mockPersistence = InMemoryPersistence();
470+
final mockAdapter = MockAdapter();
471+
final logger = LDLogger(adapter: mockAdapter, level: LDLogLevel.info);
472+
final envReporter = ConcreteEnvReporter(
473+
applicationInfo: Future.value(null),
474+
osInfo: Future.value(null),
475+
deviceInfo: Future.value(null),
476+
locale: Future.value(null));
477+
478+
final report = await PrioritizedEnvReportBuilder()
479+
.setConfigLayer(envReporter)
480+
.build();
481+
482+
final decorator = AutoEnvContextModifier(report, mockPersistence, logger);
483+
484+
final result = await decorator.decorate(invalidContext);
485+
486+
expect(result.valid, false);
487+
488+
final logRecord = verify(() => mockAdapter.log(captureAny())).captured[0]
489+
as LDLogRecord;
490+
expect(logRecord.level, LDLogLevel.warn);
491+
expect(logRecord.message,
492+
'AutoEnvContextModifier was asked to modify an invalid context and will attempt to do so. This is expected if starting with an empty context.');
493+
});
494+
495+
test('it makes an invalid context valid by adding environment attributes',
496+
() async {
497+
final invalidContext = LDContextBuilder().build();
498+
final mockPersistence = InMemoryPersistence();
499+
final logger = LDLogger();
500+
final envReporter = ConcreteEnvReporter(
501+
applicationInfo: Future.value(ApplicationInfo(
502+
applicationId: 'mockID',
503+
applicationName: 'mockName',
504+
applicationVersion: 'mockVersion',
505+
applicationVersionName: 'mockVersionName')),
506+
osInfo: Future.value(OsInfo(
507+
family: 'mockFamily',
508+
name: 'mockOsName',
509+
version: 'mockOsVersion')),
510+
deviceInfo: Future.value(
511+
DeviceInfo(model: 'mockModel', manufacturer: 'mockManufacturer')),
512+
locale: Future.value('mockLocale'));
513+
514+
final report = await PrioritizedEnvReportBuilder()
515+
.setConfigLayer(envReporter)
516+
.build();
517+
518+
final decorator = AutoEnvContextModifier(report, mockPersistence, logger);
519+
520+
final result = await decorator.decorate(invalidContext);
521+
522+
expect(result.valid, true);
523+
});
524+
});
452525
}

‎packages/common_client/test/data_sources/streaming_data_source_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class MockSseClient implements SSEClient {
6060
withReasons: withReasons, useReport: useReport),
6161
httpProperties: httpProperties,
6262
clientFactory: (Uri uri, HttpProperties properties, String? body,
63-
SseHttpMethod? method) {
63+
SseHttpMethod? method, EventSourceLogger? logger) {
6464
factoryCallback?.call(uri, properties, body, method);
6565
return client;
6666
});

‎packages/common_client/test/ld_dart_client_test.dart

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,45 @@ void main() {
286286
});
287287
});
288288

289+
group('given an invalid context', () {
290+
test('start returns false with invalid context', () async {
291+
final invalidClient = LDCommonClient(
292+
TestConfig('', AutoEnvAttributes.disabled, offline: true),
293+
CommonPlatform(),
294+
LDContextBuilder()
295+
.kind('invalid#kind', '') // invalid kind and invalid key
296+
.build(),
297+
DiagnosticSdkData(name: '', version: ''));
298+
299+
expect(await invalidClient.start(), false);
300+
});
301+
302+
test('identify returns IdentifyError with invalid context', () async {
303+
final invalidClient = LDCommonClient(
304+
TestConfig('', AutoEnvAttributes.disabled, offline: true),
305+
CommonPlatform(),
306+
LDContextBuilder()
307+
.kind('user', 'bob')
308+
.build(), // Valid initial context
309+
DiagnosticSdkData(name: '', version: ''));
310+
311+
await invalidClient.start();
312+
313+
final invalidContext = LDContextBuilder()
314+
.kind('invalid#kind', '') // invalid kind and invalid key
315+
.build();
316+
317+
expect(
318+
await invalidClient.identify(invalidContext), isA<IdentifyError>());
319+
320+
// check subsequent identify with valid context is accepted
321+
final validContext = LDContextBuilder().kind('user', 'alice').build();
322+
323+
final identifyResult = await invalidClient.identify(validContext);
324+
expect(identifyResult, isA<IdentifyComplete>());
325+
});
326+
});
327+
289328
group('given mock flag data with prerequisites', () {
290329
late LDCommonClient client;
291330
late MockPersistence mockPersistence;

0 commit comments

Comments
 (0)