-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: overwrite the deprecated googledrive extension config #7974
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| import { describe, it, expect, vi } from 'vitest'; | ||
| import { isDeprecatedGoogleDriveExtension, syncBundledExtensions } from './bundled-extensions'; | ||
| import type { FixedExtensionEntry } from '../../ConfigContext'; | ||
|
|
||
| vi.mock('./bundled-extensions.json', () => ({ | ||
| default: [ | ||
| { | ||
| id: 'developer', | ||
| name: 'developer', | ||
| display_name: 'Developer', | ||
| description: 'General development tools.', | ||
| enabled: true, | ||
| type: 'builtin', | ||
| timeout: 300, | ||
| }, | ||
| { | ||
| id: 'googledrive', | ||
| name: 'googledrive', | ||
| display_name: 'Google Drive', | ||
| description: 'Google Drive integration.', | ||
| enabled: true, | ||
| type: 'stdio', | ||
| cmd: 'googledrive-mcp', | ||
| args: [], | ||
| env_keys: [], | ||
| timeout: 300, | ||
| }, | ||
| ], | ||
| })); | ||
|
|
||
| describe('isDeprecatedGoogleDriveExtension', () => { | ||
| it('returns true for builtin googledrive', () => { | ||
| const ext = { | ||
| name: 'Google Drive', | ||
| type: 'builtin', | ||
| description: 'Google Drive extension', | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for builtin google_drive', () => { | ||
| const ext = { | ||
| name: 'google_drive', | ||
| type: 'builtin', | ||
| description: 'Google Drive extension', | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for stdio googledrive with GOOGLE_DRIVE_CREDENTIALS_PATH', () => { | ||
| const ext = { | ||
| name: 'Google Drive', | ||
| type: 'stdio', | ||
| description: 'Google Drive extension', | ||
| cmd: 'some-cmd', | ||
| args: [], | ||
| env_keys: ['GOOGLE_DRIVE_CREDENTIALS_PATH'], | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for stdio googledrive with GOOGLE_DRIVE_OAUTH_PATH', () => { | ||
| const ext = { | ||
| name: 'Google Drive', | ||
| type: 'stdio', | ||
| description: 'Google Drive extension', | ||
| cmd: 'some-cmd', | ||
| args: [], | ||
| env_keys: ['GOOGLE_DRIVE_OAUTH_PATH'], | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for stdio googledrive without deprecated env keys', () => { | ||
| const ext = { | ||
| name: 'Google Drive', | ||
| type: 'stdio', | ||
| description: 'Google Drive extension', | ||
| cmd: 'some-cmd', | ||
| args: [], | ||
| env_keys: [], | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for non-googledrive extensions', () => { | ||
| const ext = { | ||
| name: 'developer', | ||
| type: 'builtin', | ||
| description: 'Developer tools', | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for non-googledrive stdio with those env keys', () => { | ||
| const ext = { | ||
| name: 'some-other-ext', | ||
| type: 'stdio', | ||
| description: 'Other extension', | ||
| cmd: 'some-cmd', | ||
| args: [], | ||
| env_keys: ['GOOGLE_DRIVE_CREDENTIALS_PATH'], | ||
| enabled: true, | ||
| bundled: true, | ||
| } as FixedExtensionEntry; | ||
| expect(isDeprecatedGoogleDriveExtension(ext)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('syncBundledExtensions', () => { | ||
| it('overwrites deprecated builtin googledrive extension', async () => { | ||
| const addExtensionFn = vi.fn().mockResolvedValue(undefined); | ||
| const existingExtensions = [ | ||
| { | ||
| name: 'googledrive', | ||
| type: 'builtin', | ||
| description: 'Google Drive', | ||
| enabled: true, | ||
| bundled: true, | ||
| }, | ||
| ] as FixedExtensionEntry[]; | ||
|
|
||
| await syncBundledExtensions(existingExtensions, addExtensionFn); | ||
|
|
||
| expect(addExtensionFn).toHaveBeenCalledWith( | ||
| 'googledrive', | ||
| expect.objectContaining({ type: 'stdio', bundled: true }), | ||
| true | ||
| ); | ||
| }); | ||
|
|
||
| it('overwrites stdio googledrive with deprecated env keys', async () => { | ||
| const addExtensionFn = vi.fn().mockResolvedValue(undefined); | ||
| const existingExtensions = [ | ||
| { | ||
| name: 'googledrive', | ||
| type: 'stdio', | ||
| description: 'Google Drive', | ||
| cmd: 'some-cmd', | ||
| args: [], | ||
| env_keys: ['GOOGLE_DRIVE_CREDENTIALS_PATH'], | ||
| enabled: true, | ||
| bundled: true, | ||
| }, | ||
| ] as FixedExtensionEntry[]; | ||
|
|
||
| await syncBundledExtensions(existingExtensions, addExtensionFn); | ||
|
|
||
| expect(addExtensionFn).toHaveBeenCalledWith( | ||
| 'googledrive', | ||
| expect.objectContaining({ type: 'stdio', bundled: true, env_keys: [] }), | ||
| true | ||
| ); | ||
| }); | ||
|
|
||
| it('skips already bundled non-deprecated extensions', async () => { | ||
| const addExtensionFn = vi.fn().mockResolvedValue(undefined); | ||
| const existingExtensions = [ | ||
| { | ||
| name: 'developer', | ||
| type: 'builtin', | ||
| description: 'Developer tools', | ||
| enabled: true, | ||
| bundled: true, | ||
| timeout: 300, | ||
| }, | ||
| ] as FixedExtensionEntry[]; | ||
|
|
||
| await syncBundledExtensions(existingExtensions, addExtensionFn); | ||
|
|
||
| expect(addExtensionFn).not.toHaveBeenCalledWith( | ||
| 'developer', | ||
| expect.anything(), | ||
| expect.anything() | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,29 @@ type BundledExtension = { | |
| allow_configure?: boolean; | ||
| }; | ||
|
|
||
| const DEPRECATED_BUILTINS = ['googledrive', 'google_drive']; | ||
| const DEPRECATED_GOOGLE_DRIVE_IDS = ['googledrive', 'google_drive']; | ||
| const DEPRECATED_GOOGLE_DRIVE_ENV_KEYS = [ | ||
| 'GOOGLE_DRIVE_CREDENTIALS_PATH', | ||
| 'GOOGLE_DRIVE_OAUTH_PATH', | ||
| ]; | ||
|
|
||
| export function isDeprecatedGoogleDriveExtension(ext: FixedExtensionEntry): boolean { | ||
| if (!DEPRECATED_GOOGLE_DRIVE_IDS.includes(nameToKey(ext.name))) { | ||
| return false; | ||
| } | ||
| if (ext.type === 'builtin') { | ||
| return true; | ||
| } | ||
| if ( | ||
| ext.type === 'stdio' && | ||
| 'env_keys' in ext && | ||
| Array.isArray(ext.env_keys) && | ||
| ext.env_keys.some((key: string) => DEPRECATED_GOOGLE_DRIVE_ENV_KEYS.includes(key)) | ||
| ) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Synchronizes built-in extensions with the config system. | ||
|
|
@@ -39,20 +61,21 @@ export async function syncBundledExtensions( | |
| // Cast the imported JSON data to the expected type | ||
| const bundledExtensions = bundledExtensionsData as BundledExtension[]; | ||
|
|
||
| for (let i = existingExtensions.length - 1; i >= 0; i--) { | ||
| const ext = existingExtensions[i]; | ||
| if (ext.type == 'builtin' && DEPRECATED_BUILTINS.includes(ext.name)) { | ||
| existingExtensions.splice(i, 1); | ||
| } | ||
| } | ||
|
|
||
| // Process each bundled extension | ||
| for (const bundledExt of bundledExtensions) { | ||
| // Find if this extension already exists | ||
| const existingExt = existingExtensions.find((ext) => nameToKey(ext.name) === bundledExt.id); | ||
|
Comment on lines
65
to
67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The migration check is only reached for entries that match a Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the release version, the googledrive will be in the |
||
|
|
||
| // Skip if extension exists and is already marked as bundled | ||
| if (existingExt && 'bundled' in existingExt && existingExt.bundled) continue; | ||
| // Skip if extension exists and is already marked as bundled, except when | ||
| // we must migrate deprecated extensions. | ||
| if ( | ||
| existingExt && | ||
| 'bundled' in existingExt && | ||
| existingExt.bundled && | ||
| !isDeprecatedGoogleDriveExtension(existingExt) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Create the config for this extension | ||
| let extConfig: ExtensionConfig; | ||
|
|
||
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.
These tests only assert that
addExtensionFnwas called at least once, butsyncBundledExtensionsalready callsaddExtensionFnfor other missing bundled defaults in the fixture. That means the test passes even when the deprecated googledrive entry is never detected or overwritten, so it does not protect the migration behavior this commit is adding.Useful? React with 👍 / 👎.
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.
i have updated my tests based on your comment, but seems the ui has not refreshed