Conversation
Update src/documentdb/utils/DocumentDBConnectionString.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> fix: updated comments
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces API versioning for the DocumentDB extension API and adds support for a new v0.3.0 API with stricter provider registration and context validation. The PR also includes fixes for connection string parsing issues with special characters in query parameters.
- Introduces v0.3.0 API with extension context-based provider registration that enforces one-provider-per-extension rule
- Maintains backward compatibility with v0.2.0 API (marked as deprecated)
- Fixes connection string parsing for special characters (e.g.,
@) in query parameters - Adds comprehensive test coverage for connection string parsing edge cases
- Updates documentation and release notes for both patch releases (0.5.1 and 0.5.2)
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/migrationServices.ts |
Adds registerProviderWithContext method to enforce one-provider-per-extension rule and tracks extension-to-provider mappings |
src/extension.ts |
Implements dual API support (v0.2.0 and v0.3.0) using createApiProvider |
src/documentdb/utils/DocumentDBConnectionString.ts |
Adds constructor-based sanitization for query parameters and username encoding/decoding support |
src/documentdb/utils/DocumentDBConnectionString.test.ts |
Adds comprehensive test coverage for special characters, duplicate parameters, and edge cases |
src/commands/chooseDataMigrationExtension/chooseDataMigrationExtension.ts |
Refactors to remove async wrapper function and inline action items creation |
api/src/utils/getApi.ts |
Updates to support versioned API selection using getApi<T>() |
api/src/migration/migrationApi.ts |
Adds MigrationApiV030 interface requiring extension context parameter |
api/src/extensionApi.ts |
Extends interfaces from AzureExtensionApi and adds v0.3.0 interface |
package.json, api/package.json |
Version bumps and dependency updates |
| Documentation files | Updates for release notes, API usage examples, and version information |
Files not reviewed (1)
- api/package-lock.json: Language not supported
| * The main API interface for the DocumentDB extension | ||
| */ | ||
| export interface DocumentDBExtensionApi { | ||
| export interface DocumentDBExtensionApi extends AzureExtensionApi { |
There was a problem hiding this comment.
The DocumentDBExtensionApi interface is missing the apiVersion property that was previously documented. According to the AzureExtensionApi interface, it should include an apiVersion: string property. Either add this property to the interface definition or update the documentation to clarify that the version is inherited from AzureExtensionApi.
| // Create the DocumentDB Extension API v0.2.0 | ||
| const documentDBApiV2: DocumentDBExtensionApi = { | ||
| apiVersion: '0.2.0', | ||
| migration: { | ||
| registerProvider: (provider) => { |
There was a problem hiding this comment.
The inline comment states "v0.2.0" but the actual implementation includes more than just the DocumentDB API. The API object should either be documented as deprecated or the comment should clarify its relationship to the new v0.3.0 API. Consider adding a deprecation notice in the comment.
| const availableActions: (QuickPickItem & { | ||
| id: string; | ||
| learnMoreUrl?: string; | ||
| requiresAuthentication?: boolean; | ||
| })[] = (await selectedProvider.getAvailableActions(options)).map((action) => ({ |
There was a problem hiding this comment.
The inline type definition for availableActions is duplicated (appears again when extending with learn more options). Consider extracting this type to a shared interface or type alias to improve maintainability and avoid duplication.
| - **v0.3.0 (Latest)**: The current and only supported version. It requires the extension context for provider registration and enforces a one-provider-per-extension rule. | ||
| - **v0.2.0 (Deprecated)**: This version is deprecated and will be removed in a future release. New integrations should not use it. |
There was a problem hiding this comment.
The documentation states v0.3.0 is the "only supported version" while also stating v0.2.0 is "deprecated" in the next line. This is contradictory. If v0.2.0 is still functional (as the code shows), it should be described as "supported but deprecated" rather than implying it's unsupported.
| - **v0.3.0 (Latest)**: The current and only supported version. It requires the extension context for provider registration and enforces a one-provider-per-extension rule. | |
| - **v0.2.0 (Deprecated)**: This version is deprecated and will be removed in a future release. New integrations should not use it. | |
| - **v0.3.0 (Latest)**: The latest and recommended version. It requires the extension context for provider registration and enforces a one-provider-per-extension rule. | |
| - **v0.2.0 (Supported but Deprecated)**: This version is still supported but deprecated and will be removed in a future release. New integrations should not use it. |
| const encodedParams: string[] = []; | ||
|
|
||
| // Get all unique keys (without duplicates) | ||
| const uniqueKeys = [...new Set([...searchParams.keys()])]; |
There was a problem hiding this comment.
Creating a Set from searchParams.keys() and then spreading it into an array is unnecessary since URLSearchParams.keys() already returns unique keys. The Set construction adds overhead without benefit. Consider using Array.from(searchParams.keys()) or [...searchParams.keys()] directly.
| const uniqueKeys = [...new Set([...searchParams.keys()])]; | |
| const uniqueKeys = [...searchParams.keys()]; |
|
branch protections are making this PR difficult to handle. |
1 participant
No description provided.