Botbuilder-js: Upgrade CosmosDB SDK to the latest release

[sgellock]

CosmosDB has a new release of their SDK for NodeJS. We should consider pulling this in before code-complete, for the v4 release

[v-ducvo]

Hi...I filed and issue relating to CosmosDb here:
https://github.com/Microsoft/botbuilder-js/issues/403

Can someone test to make sure it does not reproduce in this release/version? Thanks.

[stevengum]

Right now we're snapped to 1.14.5 which is the latest release, and post-Ignite we will update to the latest release when a new SDK is released.

[iMicknl]

The new SDK is already released (and it is awesome!). 1.14.5 is the latest release of documentdb, however it recently got rebranded to @azure/cosmos which is on version 2.0.0-4 now.

[carlosscastro]

Thanks @iMicknl!!! Is the surface API the same or is it substantially different? We are code complete tonight, so we might have to take that for a next release in a few weeks.

[iMicknl]

You can read more in detail about the new Azure CosmosDB JavasScript SDK here, still in public preview by the way. Note that they dropped Node 4 support, I am not sure what BotBuilder SDK v4 supports?

I don't know if the surface API is totally different, as far as I know the biggest change is the support for promises and renaming of the models.

[stevengum]

Hooray for Promises! (Their note about Node 4 support)

If users are on the current Node LTS (8.12.0) they will definitely be able to use the v4 SDK.

[daveta]

We are up to date on Cosmos for v4.1.

Apparently there's a new library just released in the past week+.

[johnataylor]

We are on the latest version of the documentdb package 1.14.5 last published 4 months ago.

However, there is a new CosmosDB package in npmjs under a different name @azure/cosmos this was published 13 days ago.

Presumably we should switch to this package. This will, however, require some code change. The concepts remain the same and an initial look at the API change suggests the changes will be relatively minor.

[johnataylor]

per conversation with @sgellock moving this to next milestone.

[johnataylor]

We should be moving to the Azure/* packages. However, they imply a major rev of our implementation as the code can be quite different now. In addition these new packages are essential v1 for the new client. The documentation and samples are not all there. Moving this to the next milestone. (We also want to do more significant work around state in the next milestone too.)

[mdrichardson]

I've got a partial implementation in Node with written and passing tests. It uses the newer @azure/cosmos. I'm hoping to get to a dotnet/C# version, but the newer C# Cosmos SDK is behind.

[stevengum]

@mdrichardson can you open a PR for review, and then we'll take a look and decide if we want to reallocate resources for this for 4.3? That said, I believe the current plan is to move all state management enhancements/updates to 4.4 and I don't think we're going to change this plan at the moment.

[mdrichardson]

@stevengum I spoke with @cleemullins about this a week or so ago and his guidance was more or less to have a C# version ready first. I posted this here (and elsewhere) just to make sure it was visible to you guys so we don't need to duplicate effort.

I can still open a PR, if you'd like.

[stevengum]

If that was his guidance, let's hold off on the PRs for now until a matching implementation is done in C#. @cleemullins and @johnataylor will have stronger opinions on the final implementation, and maybe @daveta can chime in as well. Thanks for the heads up and initiative!

[stevengum]

@mdrichardson, I took a _very_ cursory glance at your branch and one thing that stood out is the following code under CosmosDbStorage.delete():

        await keys.map(async (k: string) => {
            try {
                const reqOptions = { partitionKey: this.settings.partitionValue };
                await this.container
                    .item(CosmosDbKeyEscape.escapeKey(k), this.settings.partitionKey)
                    .delete(reqOptions);
            } catch (err) {
                // Don't thow an error if trying to delete something that doesn't exist
                if (err.code !== 404) {
                    throw new Error(`Unable to delete document: ${JSON.stringify(err)}`);
                }
            }
        });

This doesn't seem like the right behavior; if the document doesn't exist, then the bot's key is incorrect. If one delete fails due to an incorrect key, that might be a transient blip; but if all of the deletes fail due to missing documents then that's a problem. One of the more likely problems being that the keys the bot is formulating are incorrect. The bot's developer should have these errors be exposed to them and they can decide how they want to proceed.

There's also a small typo in the comment 😄

[mdrichardson]

@stevengum That's a good catch. I'll look over it and do some more testing. While I was trying to get the partitionKey/partitionValue system figured out, I recall I'd see documents be deleted and still get a 404 error. Now that I've got the key/value figured out, I'll do some testing and commit/push if a change is needed.

[mdrichardson]

@stevengum

I found out why I originally didn't throw for the 404 errors on delete. The current tests say it shouldn't throw and the current implementation doesn't throw, either. I think your reasoning is sound, so I added console.log() messages for when this happens. But would we expect this to happen during use? I tested a couple of bots and never had the message show, but I'd guess that if the current implementation wasn't supposed to throw, there was a reason. What do you think?

[mdrichardson]

Related: https://github.com/Microsoft/BotBuilder/issues/5107

[cleemullins]

@mdrichardson Can you share what we should do with this? Do you have PR's ready? I would love to either:

1. Get onto the latest packages and close this issue.
2. Punt this to 4.5, at which point we do that same work.

My preference is #2, as we have significant samples and other doc changes in flight for this cycle and the change now may impact that.

[mdrichardson]

@cleemullins I had a JS version somewhat ready to go, but we've made enough changes to CosmosDbStorage that I'll need to re-look at it and make additional updates.

There's some big API changes from documentdb (Node)/Cosmos SDK V2 (C#) to @azure/cosmos (Node)/Cosmos SDK V3 (C#) that will prevent us from upgrading until both the Node and C# modules are ready, to ensure parity. Last I looked, the C# Cosmos SDK V3 (the new one and still in Preview) didn't allow non-partitioned DBs at all, which breaks backwards-compatibility. I'll take another look, but I don't believe it's ready.

There's some partitionKey things that we may as well handle when we make this change, too. I sent a Design Proposal for it that could use your review. Let me know if you didn't receive it.

I'd recommend punting this to 4.5, at least.

[mdrichardson]

@cleemullins Update: C# Cosmos SDK V3 team has an actual task, now, to make PartitionKey optional (for backwards compatibility. That SDK is still in preview. Looks like GA is planned for first half of 2019 still, so we might be able to update to the new SDKs in the next couple of months.

[mdrichardson]

@cleemullins The C# Cosmos SDK V3 is pretty close to GA now and I believe PartitionKey is optional (backwards-compat achieved). I can start working on the updates, if you'd like.

[sgellock]

introduces breaking changes

[Stevenic]

Closed as wontfix

[christopheranderson]

Just an update, this was done and is implemented here: https://github.com/microsoft/botbuilder-js/blob/master/libraries/botbuilder-azure/src/cosmosDbPartitionedStorage.ts

So we now support old way on documentdb package and new way on @azure/cosmos package.