Plugin Extensions: Custom limits for extensions-per-plugin (#69430)

* feat(plugins): remove global limit on extensions per placement * feat: add a way to limit extension per plugin at the getter * test(plugins): fix failing getPluginExtensions test * refactor(panelmenu): put back limit of 2 extensions per plugin * tests: add a test for checking all extensions are returned by default --------- Co-authored-by: Jack Westbrook <[email protected]>
grafana · Jun 2, 2023 · 7b2bd48 · 7b2bd48
1 parent d25cea9
commit 7b2bd48
Show file tree

Hide file tree

Showing 8 changed files with 65 additions and 72 deletions.
diff --git a/packages/grafana-runtime/src/services/pluginExtensions/getPluginExtensions.ts b/packages/grafana-runtime/src/services/pluginExtensions/getPluginExtensions.ts
@@ -5,9 +5,11 @@ import { isPluginExtensionComponent, isPluginExtensionLink } from './utils';
 export type GetPluginExtensions<T = PluginExtension> = ({
   extensionPointId,
   context,
+  limitPerPlugin,
 }: {
   extensionPointId: string;
   context?: object | Record<string | symbol, unknown>;
+  limitPerPlugin?: number;
 }) => {
   extensions: T[];
 };

diff --git a/public/app/features/dashboard/utils/getPanelMenu.ts b/public/app/features/dashboard/utils/getPanelMenu.ts
@@ -277,6 +277,7 @@ export function getPanelMenu(
   const { extensions } = getPluginExtensions({
     extensionPointId: PluginExtensionPoints.DashboardPanelMenu,
     context: createExtensionContext(panel, dashboard),
+    limitPerPlugin: 2,
   });
 
   if (extensions.length > 0 && !panel.isEditing) {

diff --git a/public/app/features/plugins/extensions/constants.ts b/public/app/features/plugins/extensions/constants.ts
diff --git a/public/app/features/plugins/extensions/createPluginExtensionRegistry.test.ts b/public/app/features/plugins/extensions/createPluginExtensionRegistry.test.ts
@@ -63,33 +63,6 @@ describe('createRegistry()', () => {
     );
   });
 
-  it('should register maximum 2 extensions / plugin / placement', () => {
-    const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link1, link1] }]);
-
-    expect(Object.getOwnPropertyNames(registry)).toEqual([placement1]);
-
-    // Placement 1
-    expect(registry[placement1]).toHaveLength(2);
-    expect(registry[placement1]).toEqual(
-      expect.arrayContaining([
-        expect.objectContaining({
-          pluginId,
-          config: {
-            ...link1,
-            configure: expect.any(Function),
-          },
-        }),
-        expect.objectContaining({
-          pluginId,
-          config: {
-            ...link1,
-            configure: expect.any(Function),
-          },
-        }),
-      ])
-    );
-  });
-
   it('should not register link extensions with invalid path configured', () => {
     const registry = createPluginExtensionRegistry([
       { pluginId, extensionConfigs: [{ ...link1, path: 'invalid-path' }, link2] },

diff --git a/public/app/features/plugins/extensions/createPluginExtensionRegistry.ts b/public/app/features/plugins/extensions/createPluginExtensionRegistry.ts
@@ -1,14 +1,11 @@
 import type { PluginPreloadResult } from '../pluginPreloader';
 
-import { MAX_EXTENSIONS_PER_POINT } from './constants';
-import { ExtensionsPerPlugin } from './extensionsPerPlugin';
 import type { PluginExtensionRegistryItem, PluginExtensionRegistry } from './types';
 import { deepFreeze, logWarning } from './utils';
 import { isPluginExtensionConfigValid } from './validators';
 
 export function createPluginExtensionRegistry(pluginPreloadResults: PluginPreloadResult[]): PluginExtensionRegistry {
   const registry: PluginExtensionRegistry = {};
-  const extensionsPerPlugin = new ExtensionsPerPlugin();
 
   for (const { pluginId, extensionConfigs, error } of pluginPreloadResults) {
     if (error) {
@@ -19,13 +16,6 @@ export function createPluginExtensionRegistry(pluginPreloadResults: PluginPreloa
     for (const extensionConfig of extensionConfigs) {
       const { extensionPointId } = extensionConfig;
 
-      if (!extensionsPerPlugin.allowedToAdd(extensionConfig)) {
-        logWarning(
-          `"${pluginId}" plugin has reached the limit of ${MAX_EXTENSIONS_PER_POINT} for "${extensionPointId}", skip registering extension "${extensionConfig.title}".`
-        );
-        continue;
-      }
-
       if (!extensionConfig || !isPluginExtensionConfigValid(pluginId, extensionConfig)) {
         continue;
       }

diff --git a/public/app/features/plugins/extensions/extensionsPerPlugin.ts b/public/app/features/plugins/extensions/extensionsPerPlugin.ts
diff --git a/public/app/features/plugins/extensions/getPluginExtensions.test.ts b/public/app/features/plugins/extensions/getPluginExtensions.test.ts
@@ -48,6 +48,52 @@ describe('getPluginExtensions()', () => {
     );
   });
 
+  test('should not limit the number of extensions per plugin by default', () => {
+    // Registering 3 extensions for the same plugin for the same placement
+    const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link1, link1, link2] }]);
+    const { extensions } = getPluginExtensions({ registry, extensionPointId: extensionPoint1 });
+
+    expect(extensions).toHaveLength(3);
+    expect(extensions[0]).toEqual(
+      expect.objectContaining({
+        pluginId,
+        type: PluginExtensionTypes.link,
+        title: link1.title,
+        description: link1.description,
+        path: link1.path,
+      })
+    );
+  });
+
+  test('should be possible to limit the number of extensions per plugin for a given placement', () => {
+    const registry = createPluginExtensionRegistry([
+      { pluginId, extensionConfigs: [link1, link1, link1, link2] },
+      {
+        pluginId: 'my-plugin',
+        extensionConfigs: [
+          { ...link1, path: '/a/my-plugin/declare-incident' },
+          { ...link1, path: '/a/my-plugin/declare-incident' },
+          { ...link1, path: '/a/my-plugin/declare-incident' },
+          { ...link2, path: '/a/my-plugin/declare-incident' },
+        ],
+      },
+    ]);
+
+    // Limit to 1 extension per plugin
+    const { extensions } = getPluginExtensions({ registry, extensionPointId: extensionPoint1, limitPerPlugin: 1 });
+
+    expect(extensions).toHaveLength(2);
+    expect(extensions[0]).toEqual(
+      expect.objectContaining({
+        pluginId,
+        type: PluginExtensionTypes.link,
+        title: link1.title,
+        description: link1.description,
+        path: link1.path,
+      })
+    );
+  });
+
   test('should return with an empty list if there are no extensions registered for a placement yet', () => {
     const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link2] }]);
     const { extensions } = getPluginExtensions({ registry, extensionPointId: 'placement-with-no-extensions' });

diff --git a/public/app/features/plugins/extensions/getPluginExtensions.ts b/public/app/features/plugins/extensions/getPluginExtensions.ts
@@ -26,23 +26,36 @@ import {
 type GetExtensions = ({
   context,
   extensionPointId,
+  limitPerPlugin,
   registry,
 }: {
   context?: object | Record<string | symbol, unknown>;
   extensionPointId: string;
+  limitPerPlugin?: number;
   registry: PluginExtensionRegistry;
 }) => { extensions: PluginExtension[] };
 
 // Returns with a list of plugin extensions for the given extension point
-export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, registry }) => {
+export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, limitPerPlugin, registry }) => {
   const frozenContext = context ? getReadOnlyProxy(context) : {};
   const registryItems = registry[extensionPointId] ?? [];
   // We don't return the extensions separated by type, because in that case it would be much harder to define a sort-order for them.
   const extensions: PluginExtension[] = [];
+  const extensionsByPlugin: Record<string, number> = {};
 
   for (const registryItem of registryItems) {
     try {
       const extensionConfig = registryItem.config;
+      const { pluginId } = registryItem;
+
+      // Only limit if the `limitPerPlugin` is set
+      if (limitPerPlugin && extensionsByPlugin[pluginId] >= limitPerPlugin) {
+        continue;
+      }
+
+      if (extensionsByPlugin[pluginId] === undefined) {
+        extensionsByPlugin[pluginId] = 0;
+      }
 
       // LINK
       if (isPluginExtensionLinkConfig(extensionConfig)) {
@@ -67,6 +80,7 @@ export const getPluginExtensions: GetExtensions = ({ context, extensionPointId,
         };
 
         extensions.push(extension);
+        extensionsByPlugin[pluginId] += 1;
       }
 
       // COMPONENT
@@ -84,6 +98,7 @@ export const getPluginExtensions: GetExtensions = ({ context, extensionPointId,
         };
 
         extensions.push(extension);
+        extensionsByPlugin[pluginId] += 1;
       }
     } catch (error) {
       if (error instanceof Error) {