fix: add proper checks for Hierarchical PartionKeys#1283
fix: add proper checks for Hierarchical PartionKeys#1283manekinekko wants to merge 1 commit intomasterfrom
Conversation
| paths: [`/${partitionKey}`], | ||
| }; | ||
| } | ||
| else if (typeof partitionKey === 'object') { |
There was a problem hiding this comment.
I think the partition key might be an array, which will still resolve to object and in this case will work just fine. Looks good to me :)
There was a problem hiding this comment.
I understand the confusion here, but partitionKey here represents actually entityDescriptor.partitionKey object, not the partition keys values (paths).
There was a problem hiding this comment.
Pull Request Overview
This PR enhances partition key handling, broadens dependency compatibility, and cleans up related code.
- Adds runtime checks in provider to distinguish string vs. object partitionKey definitions.
- Updates package.json to support NestJS v10 and v11 and loosen Azurite version.
- Removes a no-op test hook and tweaks core module destructuring to adjust forwarded SDK options.
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sample/cosmos-db/src/event/event.entity.ts | Minor formatting (trailing commas/semicolons) to align syntax. |
| package.json | Expanded version ranges for @nestjs/* and azurite dependencies. |
| lib/cosmos-db/cosmos-db.providers.ts | Added type guards and error handling for partitionKey definitions. |
| lib/cosmos-db/cosmos-db.decorators.spec.ts | Removed commented mock and replaced with an empty beforeEach hook. |
| lib/cosmos-db/cosmos-db-core.module.ts | Excluded connectionName from destructuring to change SDK parameters. |
Files not reviewed (1)
- sample/cosmos-db/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
lib/cosmos-db/cosmos-db.decorators.spec.ts:5
- [nitpick] This empty
beforeEachblock is redundant and can be removed to streamline the tests.
beforeEach(() => {});
| provide: cosmosConnectionName, | ||
| useFactory: async (cosmosModuleOptions: AzureCosmosDbOptions): Promise<any> => { | ||
| const { dbName, retryAttempts, retryDelay, connectionName, ...cosmosOptions } = cosmosModuleOptions; | ||
| const { dbName, retryAttempts, retryDelay, ...cosmosOptions } = cosmosModuleOptions; |
There was a problem hiding this comment.
connectionName is no longer excluded and will be spread into cosmosOptions, potentially passing an unsupported option to the Azure SDK. Re-introduce connectionName in the destructuring exclusion or explicitly delete it from cosmosOptions.
| const { dbName, retryAttempts, retryDelay, ...cosmosOptions } = cosmosModuleOptions; | |
| const { dbName, retryAttempts, retryDelay, connectionName, ...cosmosOptions } = cosmosModuleOptions; |
|
This seems to solve the issue for us 👍 |
| }; | ||
| } | ||
| else if (typeof partitionKey === 'object') { | ||
| containerOptions.partitionKey = partitionKey; |
There was a problem hiding this comment.
I found one issue, it throws an error if the property path is an empty array
@CosmosPartitionKey({
paths: [],
version: PartitionKeyDefinitionVersion.V2,
kind: PartitionKeyKind.MultiHash,
})
|
Hey! |
5 participants
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1165
What is the new behavior?
Does this PR introduce a breaking change?
Other information