Skip to content
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

Fix Cosmos nesting problem #3175

Merged
merged 7 commits into from
Jan 8, 2021
Merged

Conversation

v-kydela
Copy link
Contributor

@v-kydela v-kydela commented Jan 8, 2021

Fixes #3126

Description

Some customers experienced cryptic error messages when using Cosmos. We discovered that deeply nested data was the culprit, so I've added new error messages to help customers who encounter this problem in the future.

Specific Changes

  • Two new tests have been added to cosmosDbPartitionedStorage.test.js
  • CosmosDbPartitionedStorage checks the nesting depth when a Cosmos exception is thrown during a write operation
  • Some code cleanup has been applied to cosmosDbPartitionedStorage.ts and cosmosDbPartitionedStorage.test.js

Testing

Included. Note that waterfall dialogs break recorded tests by generating random instance ID's, and mockHelper.js is designed to fix the test data by looking for waterfall dialogs in the root dialog stack. It is not designed to look for waterfall dialogs inside component dialogs, so it cannot automatically fix the recorded test data for the dialog nesting test. Rather than fixing mockHelper.js or trying some type of non-waterfall dialog, I decided to edit the test data manually because I am told we will be moving to a new testing system soon. This was the simplest solution, but it means that it will need to be manually edited again if new test data is recorded for the dialog nesting test.

@v-kydela v-kydela requested a review from a team as a code owner January 8, 2021 00:58
@coveralls
Copy link

coveralls commented Jan 8, 2021

Pull Request Test Coverage Report for Build 472780698

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.008%) to 84.367%

Files with Coverage Reduction New Missed Lines %
libraries/botbuilder-lg/src/templateExtensions.ts 1 84.48%
libraries/botbuilder-azure/src/cosmosDbPartitionedStorage.ts 21 77.07%
Totals Coverage Status
Change from base Build 472682418: 0.008%
Covered Lines: 17744
Relevant Lines: 20090

💛 - Coveralls

private checkForNestingError(json: object, err: Error | Record<'message', string> | string): void {
const checkDepth = (obj: unknown, depth: number, isInDialogState: boolean): void => {
if (depth > maxDepthAllowed) {
var message: string = `Maximum nesting depth of ${maxDepthAllowed} exceeded.`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let preferred to var.

@joshgummersall joshgummersall merged commit 11b2408 into main Jan 8, 2021
@joshgummersall joshgummersall deleted the jpg/port-cosmos-nesting-error-info branch January 8, 2021 22:48
@joshgummersall
Copy link
Contributor

joshgummersall commented Jan 8, 2021

@v-kydela I accidentally approved & merged before you had a chance to amend the var usage. I handled that and added a lint rule that will complain about var in the future (#3179).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port: Fix Cosmos nesting problem (#4973)
3 participants