Cosmos DB: Migration to V3 SDK #717
Conversation
ealsur
force-pushed
the
cosmosdbv3
branch
11 times, most recently
from
c6ce32d
to
da0b1b9
Compare
|
Tagging @divyagandhisethi - for reference - for PR: kedacore/external-scaler-azure-cosmos-db#1 |
| if (item is string) | ||
| { | ||
| convertedItem = JObject.Parse(item.ToString()); | ||
| JObject asJObject = JObject.Parse(item.ToString()); |
There was a problem hiding this comment.
Can we use System.Text.Json here? Would that allow you to remove a ref to Newtonsoft?
There was a problem hiding this comment.
Nevermind... I forgot we require Newtonsoft.
There was a problem hiding this comment.
The SDK could take System.Text.Json using a custom serializer if that is the approach we want to take.
| document = await _context.Service.GetContainer(_context.ResolvedAttribute.DatabaseName, _context.ResolvedAttribute.CollectionName) | ||
| .ReadItemAsync<T>(_context.ResolvedAttribute.Id, partitionKey); | ||
|
|
||
| _originalItem = JObject.FromObject(document); |
There was a problem hiding this comment.
I'm wondering if we want to continue supporting this "update-in-place" behavior. It's never worked for out-of-proc languages and has resulted in a decent amount of confusion as it seems all of the bindings handle it differently.
@fabiocav -- thoughts?
| /// <summary> | ||
| /// Gets or sets the lease options for the DocumentDB Trigger. | ||
| /// </summary> | ||
| public ChangeFeedHostOptions LeaseOptions { get; set; } = new ChangeFeedHostOptions(); |
There was a problem hiding this comment.
They were already there in the Trigger properties. Normally these are for a very advanced customization of the trigger, and having it as a general configuration (for all the Triggers in the project) wouldn't have sense.
| /// </summary> | ||
| [AppSetting] | ||
| [ConnectionString] |
There was a problem hiding this comment.
I should probably know this... but what's the difference between this and [AppSetting]?
There was a problem hiding this comment.
I actually don't know, but the CosmosDBAttribute file had it as ConnectionString so thought it should be consistent?
|
|
||
| Type documentType = CosmosDBTriggerAttributeBindingProviderGenerator.GetParameterType(context.Parameter); | ||
|
|
||
| if (typeof(JArray).IsAssignableFrom(documentType)) |
There was a problem hiding this comment.
Oh right... I forgot we required a Newtonsoft ref...
There was a problem hiding this comment.
If we were to change to System.Text.Json, which would be the equivalent?
| // TODO: What's up with string | ||
| if (typeof(string).IsAssignableFrom(documentType)) | ||
| { | ||
| documentType = typeof(JObject); // When binding to JArray, use JObject as contract. |
There was a problem hiding this comment.
Yes, I added E2E tests for it.
There was a problem hiding this comment.
public static void TriggerWithString(
[CosmosDBTrigger(DatabaseName, CollectionName, CreateLeaseCollectionIfNotExists = true, LeaseCollectionPrefix = "withstring")] string documents,
ILogger log)
{
foreach (var document in JArray.Parse(documents))
{
log.LogInformation("Trigger with string called!");
}
}
| } | ||
|
|
||
| Dictionary<string, object> bindingData = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase); | ||
| bindingData.Add("CosmosDBTrigger", value); |
There was a problem hiding this comment.
Was this supported before? Not sure if we need this?
There was a problem hiding this comment.
I thought it was required, but I removed it and still worked.
This PR migrates the current implementation to V3 SDK and bumps the version to 4.0.0.
Closes #610