From 00851c6b5d52901850f7cf4b07738cccc9bd7c93 Mon Sep 17 00:00:00 2001 From: Anton Nikolaev Date: Tue, 11 May 2021 17:02:24 -0700 Subject: [PATCH] Fix bug with plugin re-installation after uninstallation Summary: Fixed a bug when plugin installation status wouldn't be saved between sessions when plugin uninstalled and then re-installed again. Before the fix, after Flipper restart, such plugin was uninstalled again because its package name was not removed from "uninstalledPlugins" state. This was because plugin id was used by mistake instead of name in few places. To try avoiding this issue in future I've also renamed "uninstalledPlugins" to "uninstalledPluginNames" to make it more clear than package name should be used there rather than ID. As this field is persisted, I also added migration which moves data to the renamed field. Reviewed By: passy Differential Revision: D28314447 fbshipit-source-id: fbe3edc258b78fe7fbb0d966f93aabcdf3b66d4b --- .../createMockFlipperWithPlugin.node.tsx.snap | 2 +- .../__tests__/pluginManager.node.tsx | 6 ++-- desktop/app/src/dispatcher/pluginManager.tsx | 2 +- desktop/app/src/dispatcher/plugins.tsx | 5 ++-- .../src/reducers/__tests__/plugins.node.tsx | 8 ++--- desktop/app/src/reducers/index.tsx | 8 +++-- desktop/app/src/reducers/plugins.tsx | 30 +++++++++++++++---- .../src/utils/__tests__/exportData.node.tsx | 10 +++---- 8 files changed, 48 insertions(+), 23 deletions(-) diff --git a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index c36458426..0e9098431 100644 --- a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -67,6 +67,6 @@ Object { "loadedPlugins": Map {}, "marketplacePlugins": Array [], "selectedPlugins": Array [], - "uninstalledPlugins": Set {}, + "uninstalledPluginNames": Set {}, } `; diff --git a/desktop/app/src/dispatcher/__tests__/pluginManager.node.tsx b/desktop/app/src/dispatcher/__tests__/pluginManager.node.tsx index 868901fd4..4b95402f4 100644 --- a/desktop/app/src/dispatcher/__tests__/pluginManager.node.tsx +++ b/desktop/app/src/dispatcher/__tests__/pluginManager.node.tsx @@ -153,7 +153,9 @@ test('uninstall plugin', async () => { mockFlipper.getState().plugins.loadedPlugins.has('plugin1'), ).toBeFalsy(); expect( - mockFlipper.getState().plugins.uninstalledPlugins.has('flipper-plugin1'), + mockFlipper + .getState() + .plugins.uninstalledPluginNames.has('flipper-plugin1'), ).toBeTruthy(); expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy(); }); @@ -179,7 +181,7 @@ test('uninstall bundled plugin', async () => { expect( mockFlipper .getState() - .plugins.uninstalledPlugins.has('flipper-bundled-plugin'), + .plugins.uninstalledPluginNames.has('flipper-bundled-plugin'), ).toBeTruthy(); expect(mockClient.sandyPluginStates.has('bundled-plugin')).toBeFalsy(); }); diff --git a/desktop/app/src/dispatcher/pluginManager.tsx b/desktop/app/src/dispatcher/pluginManager.tsx index 92672e70d..36a7a73b7 100644 --- a/desktop/app/src/dispatcher/pluginManager.tsx +++ b/desktop/app/src/dispatcher/pluginManager.tsx @@ -58,7 +58,7 @@ import { const maxInstalledPluginVersionsToKeep = 2; function refreshInstalledPlugins(store: Store) { - removePlugins(store.getState().plugins.uninstalledPlugins.values()) + removePlugins(store.getState().plugins.uninstalledPluginNames.values()) .then(() => cleanupOldInstalledPluginVersions(maxInstalledPluginVersionsToKeep), ) diff --git a/desktop/app/src/dispatcher/plugins.tsx b/desktop/app/src/dispatcher/plugins.tsx index 580940665..4c01de9a3 100644 --- a/desktop/app/src/dispatcher/plugins.tsx +++ b/desktop/app/src/dispatcher/plugins.tsx @@ -85,14 +85,15 @@ export default async (store: Store, logger: Logger) => { ), ); - const uninstalledPlugins = store.getState().plugins.uninstalledPlugins; + const uninstalledPluginNames = + store.getState().plugins.uninstalledPluginNames; const bundledPlugins = getBundledPlugins(); const loadedPlugins = filterNewestVersionOfEachPlugin( bundledPlugins, await getDynamicPlugins(), - ).filter((p) => !uninstalledPlugins.has(p.name)); + ).filter((p) => !uninstalledPluginNames.has(p.name)); const initialPlugins: PluginDefinition[] = loadedPlugins .map(reportVersion) diff --git a/desktop/app/src/reducers/__tests__/plugins.node.tsx b/desktop/app/src/reducers/__tests__/plugins.node.tsx index 962639695..a256e0bf8 100644 --- a/desktop/app/src/reducers/__tests__/plugins.node.tsx +++ b/desktop/app/src/reducers/__tests__/plugins.node.tsx @@ -40,7 +40,7 @@ test('add clientPlugin', () => { disabledPlugins: [], selectedPlugins: [], marketplacePlugins: [], - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), installedPlugins: new Map(), initialised: false, }, @@ -61,7 +61,7 @@ test('add devicePlugin', () => { disabledPlugins: [], selectedPlugins: [], marketplacePlugins: [], - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), installedPlugins: new Map(), initialised: false, }, @@ -82,7 +82,7 @@ test('do not add plugin twice', () => { disabledPlugins: [], selectedPlugins: [], marketplacePlugins: [], - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), installedPlugins: new Map(), initialised: false, }, @@ -120,7 +120,7 @@ test('add gatekeeped plugin', () => { selectedPlugins: [], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: false, }, addGatekeepedPlugins(gatekeepedPlugins), diff --git a/desktop/app/src/reducers/index.tsx b/desktop/app/src/reducers/index.tsx index 13d96688d..5d1a1ffae 100644 --- a/desktop/app/src/reducers/index.tsx +++ b/desktop/app/src/reducers/index.tsx @@ -33,6 +33,8 @@ import notifications, { import plugins, { State as PluginsState, Action as PluginsAction, + persistMigrations as pluginsPersistMigrations, + persistVersion as pluginsPersistVersion, } from './plugins'; import supportForm, { State as SupportFormState, @@ -169,8 +171,10 @@ export default combineReducers({ { key: 'plugins', storage, - whitelist: ['marketplacePlugins', 'uninstalledPlugins'], - transforms: [setTransformer({whitelist: ['uninstalledPlugins']})], + whitelist: ['marketplacePlugins', 'uninstalledPluginNames'], + transforms: [setTransformer({whitelist: ['uninstalledPluginNames']})], + version: pluginsPersistVersion, + migrate: createMigrate(pluginsPersistMigrations), }, plugins, ), diff --git a/desktop/app/src/reducers/plugins.tsx b/desktop/app/src/reducers/plugins.tsx index ec7db9a4c..480d745c7 100644 --- a/desktop/app/src/reducers/plugins.tsx +++ b/desktop/app/src/reducers/plugins.tsx @@ -27,7 +27,9 @@ export interface MarketplacePluginDetails extends DownloadablePluginDetails { availableVersions?: DownloadablePluginDetails[]; } -export type State = { +export type State = StateV1; + +type StateV1 = { devicePlugins: DevicePluginMap; clientPlugins: ClientPluginMap; loadedPlugins: Map; @@ -37,11 +39,27 @@ export type State = { failedPlugins: Array<[ActivatablePluginDetails, string]>; selectedPlugins: Array; marketplacePlugins: Array; - uninstalledPlugins: Set; + uninstalledPluginNames: Set; installedPlugins: Map; initialised: boolean; }; +type StateV0 = Omit & { + uninstalledPlugins: Set; +}; + +export const persistVersion = 1; +export const persistMigrations = { + 1: (state: any) => { + const stateV0 = state as StateV0; + const stateV1: StateV1 = { + ...stateV0, + uninstalledPluginNames: new Set(stateV0.uninstalledPlugins), + }; + return stateV1 as any; + }, +}; + export type RegisterPluginAction = { type: 'REGISTER_PLUGINS'; payload: PluginDefinition[]; @@ -107,7 +125,7 @@ const INITIAL_STATE: State = { failedPlugins: [], selectedPlugins: [], marketplacePlugins: [], - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), installedPlugins: new Map(), initialised: false, }; @@ -169,7 +187,7 @@ export default function reducer( return produce(state, (draft) => { draft.installedPlugins.clear(); action.payload.forEach((p) => { - if (!draft.uninstalledPlugins.has(p.id)) { + if (!draft.uninstalledPluginNames.has(p.name)) { draft.installedPlugins.set(p.id, p); } }); @@ -188,7 +206,7 @@ export default function reducer( draft.clientPlugins.delete(plugin.id); draft.devicePlugins.delete(plugin.id); draft.loadedPlugins.delete(plugin.id); - draft.uninstalledPlugins.add(plugin.name); + draft.uninstalledPluginNames.add(plugin.name); }); } else if (action.type === 'PLUGIN_LOADED') { const plugin = action.payload; @@ -198,7 +216,7 @@ export default function reducer( } else { draft.clientPlugins.set(plugin.id, plugin); } - draft.uninstalledPlugins.delete(plugin.id); + draft.uninstalledPluginNames.delete(plugin.details.name); draft.loadedPlugins.set(plugin.id, plugin.details); }); } else if (action.type === 'PLUGINS_INITIALISED') { diff --git a/desktop/app/src/utils/__tests__/exportData.node.tsx b/desktop/app/src/utils/__tests__/exportData.node.tsx index 49508fa92..911154431 100644 --- a/desktop/app/src/utils/__tests__/exportData.node.tsx +++ b/desktop/app/src/utils/__tests__/exportData.node.tsx @@ -767,7 +767,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present selectedPlugins: ['TestPlugin'], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: true, }; const op = determinePluginsToProcess( @@ -842,7 +842,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien selectedPlugins: ['RandomPlugin'], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: true, }; const op = determinePluginsToProcess([client1, client2], device1, plugins); @@ -894,7 +894,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async selectedPlugins: ['TestPlugin'], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: true, }; const op = determinePluginsToProcess([client1, client2], device1, plugins); @@ -984,7 +984,7 @@ test('test determinePluginsToProcess for multiple clients on different device', selectedPlugins: ['TestPlugin'], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: true, }; const op = determinePluginsToProcess( @@ -1070,7 +1070,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => { selectedPlugins: ['TestPlugin'], marketplacePlugins: [], installedPlugins: new Map(), - uninstalledPlugins: new Set(), + uninstalledPluginNames: new Set(), initialised: true, }; const op = determinePluginsToProcess(