-
Notifications
You must be signed in to change notification settings - Fork 55
Caching integration settings and restoring last successful cache #190
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,18 +33,19 @@ class StateManager { | |||||||||||||||
| deepLinkData.init(errorHandler, storageJson); | ||||||||||||||||
| userInfo.init(errorHandler, storageJson); | ||||||||||||||||
| context.init(errorHandler, storageJson); | ||||||||||||||||
| integrations.init(errorHandler, storageJson); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| StateManager(Store store, System system, Configuration configuration) | ||||||||||||||||
| : system = SystemState(system), | ||||||||||||||||
| configuration = ConfigurationState(configuration), | ||||||||||||||||
| integrations = IntegrationsState({}), | ||||||||||||||||
| integrations = IntegrationsState(store), | ||||||||||||||||
| filters = FiltersState(store), | ||||||||||||||||
| deepLinkData = DeepLinkDataState(store), | ||||||||||||||||
| userInfo = UserInfoState(store), | ||||||||||||||||
| context = ContextState(store, configuration) { | ||||||||||||||||
| _ready = Future.wait<void>( | ||||||||||||||||
| [filters.ready, deepLinkData.ready, userInfo.ready, context.ready]) | ||||||||||||||||
| [filters.ready, deepLinkData.ready, userInfo.ready, context.ready, integrations.ready]) | ||||||||||||||||
| .then((_) => _isReady = true); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -500,19 +501,50 @@ class TransformerConfigMap { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| class IntegrationsState extends StateNotifier<Map<String, dynamic>> { | ||||||||||||||||
| IntegrationsState(super.integrations); | ||||||||||||||||
| final Store _store; | ||||||||||||||||
| final String _key = "integrations"; | ||||||||||||||||
| Map<String, dynamic>? _cachedState; | ||||||||||||||||
|
|
||||||||||||||||
| IntegrationsState(this._store) : super({}); | ||||||||||||||||
|
|
||||||||||||||||
| @override | ||||||||||||||||
| Map<String, dynamic> get state => super.state; | ||||||||||||||||
|
|
||||||||||||||||
| @override | ||||||||||||||||
| set state(Map<String, dynamic> state) => super.state = state; | ||||||||||||||||
| set state(Map<String, dynamic> newState) { | ||||||||||||||||
| super.state = newState; | ||||||||||||||||
| // Persist to store when state is updated | ||||||||||||||||
| _store.setPersisted(_key, newState); | ||||||||||||||||
|
Comment on lines
+514
to
+517
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| void addIntegration(String key, Map<String, dynamic> settings) { | ||||||||||||||||
| final integrations = state; | ||||||||||||||||
| integrations[key] = settings; | ||||||||||||||||
| state = integrations; | ||||||||||||||||
|
Comment on lines
521
to
523
|
||||||||||||||||
| final integrations = state; | |
| integrations[key] = settings; | |
| state = integrations; | |
| state = { | |
| ...state, | |
| key: settings, | |
| }; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadCachedSettings method has a race condition. If called concurrently before _cachedState is set, multiple callers will all fetch from the store and could set different values. Consider using a Future to ensure only one load operation happens at a time, similar to how PersistedState handles this with _getCompleter.
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The init method ignores the errorHandler and storageJson parameters. If these parameters are not needed, consider removing them from the signature, or document why they're unused. For consistency with other state classes, this method should either use these parameters or have a different signature.
| void init(ErrorHandler errorHandler, bool storageJson) { | |
| void init() { |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadCachedSettings() call is not awaited in the init method. This means initialization will complete before cached settings are loaded, potentially causing a race condition where settings might not be available when expected. Consider making init async and awaiting the call, or ensure the ready mechanism properly waits for this operation.
| Future<void> get ready => Future.value(); | |
| void init(ErrorHandler errorHandler, bool storageJson) { | |
| loadCachedSettings(); | |
| Future<void> get ready => loadCachedSettings(); | |
| Future<void> init(ErrorHandler errorHandler, bool storageJson) async { | |
| await loadCachedSettings(); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ import 'package:shared_preferences/shared_preferences.dart'; | |||||
|
|
||||||
| import 'mocks/mocks.dart'; | ||||||
| import 'mocks/mocks.mocks.dart'; | ||||||
| import 'mocks/mock_store.dart'; | ||||||
|
|
||||||
| void main() { | ||||||
| WidgetsFlutterBinding.ensureInitialized(); | ||||||
|
|
@@ -137,6 +138,125 @@ void main() { | |||||
| ); | ||||||
| expect(analytics, isA<Analytics>()); | ||||||
| }); | ||||||
|
|
||||||
| group("Integration Settings Persistence", () { | ||||||
| test("settings loading and fallback behavior", () async { | ||||||
| // Setup | ||||||
| final testSettings = {"test_integration": {"setting1": "value1"}}; | ||||||
| final defaultSettings = {"default_integration": {"enabled": true}}; | ||||||
| AnalyticsPlatform.instance = MockPlatform(); | ||||||
| LogFactory.logger = Mocks.logTarget(); | ||||||
| SharedPreferences.setMockInitialValues({}); | ||||||
|
|
||||||
| // Test Case 1: First initialization with successful network fetch | ||||||
| // --------------------------------------------------------------- | ||||||
| // Create an in-memory store for testing | ||||||
| final mockStore1 = InMemoryStore(storageJson: true); | ||||||
|
|
||||||
| final httpClient1 = Mocks.httpClient(); | ||||||
| when(httpClient1.settingsFor(any)) | ||||||
| .thenAnswer((_) => Future.value(SegmentAPISettings(testSettings))); | ||||||
|
|
||||||
| // Initialize analytics | ||||||
| final analytics1 = Analytics( | ||||||
| Configuration("test_key"), | ||||||
| mockStore1, | ||||||
| httpClient: (_) => httpClient1, | ||||||
| ); | ||||||
| await analytics1.init(); | ||||||
|
|
||||||
| // Verify network settings are used | ||||||
| final stateSettings1 = await analytics1.state.integrations.state; | ||||||
|
||||||
| final stateSettings1 = await analytics1.state.integrations.state; | |
| final stateSettings1 = analytics1.state.integrations.state; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics2.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings2 = analytics2.state.integrations.state;
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics3.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings3 = analytics3.state.integrations.state;
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics4.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings4 = analytics4.state.integrations.state;
| final stateSettings4 = await analytics4.state.integrations.state; | |
| final stateSettings4 = analytics4.state.integrations.state; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:mockito/mockito.dart'; | ||
| import 'package:segment_analytics/analytics.dart'; | ||
| import 'package:segment_analytics/state.dart'; | ||
| import 'package:shared_preferences/shared_preferences.dart'; | ||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:segment_analytics/analytics_platform_interface.dart'; | ||
| import 'package:segment_analytics/logger.dart'; | ||
|
|
||
| import 'mocks/mocks.dart'; | ||
| import 'mocks/mocks.mocks.dart'; | ||
| import 'mocks/mock_store.dart'; | ||
|
|
||
| // Test that verifies the priority order of settings: | ||
| // 1. Network settings | ||
| // 2. Cached settings | ||
| // 3. Default settings | ||
| // 4. Empty map | ||
| void main() { | ||
| WidgetsFlutterBinding.ensureInitialized(); | ||
|
|
||
| group("Integration Settings Priority Test", () { | ||
| test("Settings priority order: network > cache > default > empty", () async { | ||
| // Setup | ||
| final networkSettings = {"network_integration": {"setting1": "network_value"}}; | ||
| final cachedSettings = {"cached_integration": {"setting1": "cached_value"}}; | ||
| final defaultSettings = {"default_integration": {"setting1": "default_value"}}; | ||
|
|
||
| AnalyticsPlatform.instance = MockPlatform(); | ||
| LogFactory.logger = Mocks.logTarget(); | ||
| SharedPreferences.setMockInitialValues({}); | ||
|
|
||
| // Test Case 1: First Initialization with Network Success | ||
| // --------------------------------------------------------------- | ||
| // Create an in-memory store with no cached settings | ||
| final mockStore1 = InMemoryStore(storageJson: true); | ||
|
|
||
| // Note: settings will be stored directly in the InMemoryStore | ||
|
|
||
| // Create HTTP client that returns network settings | ||
| final httpClient1 = Mocks.httpClient(); | ||
| when(httpClient1.settingsFor(any)) | ||
| .thenAnswer((_) => Future.value(SegmentAPISettings(networkSettings))); | ||
|
|
||
| // Initialize analytics | ||
| final analytics1 = Analytics( | ||
| Configuration("test_key", | ||
| defaultIntegrationSettings: defaultSettings | ||
| ), | ||
| mockStore1, | ||
| httpClient: (_) => httpClient1, | ||
| ); | ||
| await analytics1.init(); | ||
|
|
||
| // Verify network settings are used in this session | ||
| final stateSettings1 = analytics1.state.integrations.state; | ||
| expect(stateSettings1, equals(networkSettings), | ||
| reason: "Should use settings from network on first run"); | ||
|
|
||
| // Verify settings were persisted to storage | ||
| final storedSettings = await mockStore1.getPersisted("integrations"); | ||
| expect(storedSettings, equals(networkSettings), | ||
| reason: "Network settings should be stored to cache"); | ||
|
Comment on lines
+61
to
+63
|
||
|
|
||
| // Test Case 2: Network Failure with Cached Settings | ||
| // --------------------------------------------------------------- | ||
| // Create an in-memory store with cached settings | ||
| final mockStore2 = InMemoryStore(storageJson: true); | ||
|
|
||
| // Seed the store with cached settings | ||
| await mockStore2.setPersisted("integrations", networkSettings); | ||
|
|
||
| // Create HTTP client that fails | ||
| final failingHttpClient = Mocks.httpClient(); | ||
| when(failingHttpClient.settingsFor(any)) | ||
| .thenAnswer((_) => Future.value(null)); | ||
|
|
||
| // Initialize analytics | ||
| final analytics2 = Analytics( | ||
| Configuration("test_key", | ||
| defaultIntegrationSettings: defaultSettings | ||
| ), | ||
| mockStore2, | ||
| httpClient: (_) => failingHttpClient, | ||
| ); | ||
| await analytics2.init(); | ||
|
|
||
| // Verify cached settings are used when network fails | ||
| final stateSettings2 = analytics2.state.integrations.state; | ||
| expect(stateSettings2, equals(networkSettings), | ||
| reason: "Should use cached settings when network fails"); | ||
|
|
||
| // Test Case 3: No Network, No Cache - Uses Default Settings | ||
| // --------------------------------------------------------------- | ||
| final mockStore3 = InMemoryStore(storageJson: true); | ||
|
|
||
| // No need to add any cached settings as the store starts empty | ||
|
|
||
| final failingHttpClient2 = Mocks.httpClient(); | ||
| when(failingHttpClient2.settingsFor(any)) | ||
| .thenAnswer((_) => Future.value(null)); | ||
|
|
||
| final analytics3 = Analytics( | ||
| Configuration("test_key", | ||
| defaultIntegrationSettings: defaultSettings | ||
| ), | ||
| mockStore3, | ||
| httpClient: (_) => failingHttpClient2, | ||
| ); | ||
| await analytics3.init(); | ||
|
|
||
| // Verify default settings are used when no network and no cache | ||
| final stateSettings3 = analytics3.state.integrations.state; | ||
| expect(stateSettings3, equals(defaultSettings), | ||
| reason: "Should use default settings when network fails and no cache"); | ||
|
|
||
| // Test Case 4: No Network, No Cache, No Default - Uses Empty Map | ||
| // --------------------------------------------------------------- | ||
| final mockStore4 = InMemoryStore(storageJson: true); | ||
|
|
||
| // No need to add any cached settings as the store starts empty | ||
|
|
||
| final failingHttpClient3 = Mocks.httpClient(); | ||
| when(failingHttpClient3.settingsFor(any)) | ||
| .thenAnswer((_) => Future.value(null)); | ||
|
|
||
| final analytics4 = Analytics( | ||
| Configuration("test_key"), // No default settings | ||
| mockStore4, | ||
| httpClient: (_) => failingHttpClient3, | ||
| ); | ||
| await analytics4.init(); | ||
|
|
||
| // Verify empty map is used as last resort | ||
| final stateSettings4 = analytics4.state.integrations.state; | ||
| expect(stateSettings4, equals({}), | ||
| reason: "Should use empty map when all else fails"); | ||
| }); | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check
cachedSettings.isNotEmptyis insufficient to determine if cached settings were loaded. TheloadCachedSettings()method returns an empty map{}when no cached settings exist, and{}.isNotEmptyisfalse. However, if cached settings exist but are an empty map (e.g.,{}), they would also fail this check. Consider checking ifcachedSettingscame from the store vs. the fallback, or use a nullable return type to distinguish between "no cache" and "empty cache".