Cosmos: Add basic support for collections and dictionaries of primitive types

[AndriySvyryd]

Fixes #14762

[ghost]

Hello @AndriySvyryd!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

[roji]

Looks great, and nice to see some Npgsql code helping out :)

General comments:

• Nested collections (e.g. arrays over array). We should consider doing this now if it's relatively trivial, since seems pretty high-value.
• Value converters, i.e. whatever converters the built-in ValueConverterSelector provides, Cosmos should also provide converters for collections over those primitive types. Npgsql provides this, we could maybe even add this to the core ValueConverterSelector. Should definitely be out of scope for 6.0.

[roji]

There is the question of arrays over arrays, arrays over dictionaries over arrays, etc. I'm assuming this is all supported as we're in JSON, right? If so, this should recurse into FindMapping (and we should probably add tests too).

We could also define this as advanced/out-of-scope, in case this is complex to do.

[roji]

One more thought - there's also the .NET immutable collection types (e.g. ImmutableDictionary), which could be a great match efficient collection change tracking: you can do relatively efficient non-destructive mutation (getting back a new instance), and then you have to assign that instance back (so the comparer can just check reference equality). Npgsql partially supports (npgsql/efcore.pg#1491).

[AndriySvyryd]

One more thought - there's also the .NET immutable collection types (e.g. ImmutableDictionary), which could be a great match efficient collection change tracking: you can do relatively efficient non-destructive mutation (getting back a new instance), and then you have to assign that instance back (so the comparer can just check reference equality). Npgsql partially supports (npgsql/efcore.pg#1491).

Yes, and I added a test for it. We don't snapshot it when it's mapped as IReadOnlyDictionary<string, T>, but we still need to do a structural comparison when the reference is not equal as it still might contain the same values.

[AndriySvyryd]

Value converters, i.e. whatever converters the built-in ValueConverterSelector provides, Cosmos should also provide converters for collections over those primitive types. Npgsql provides this, we could maybe even add this to the core ValueConverterSelector. Should definitely be out of scope for 6.0.

#25343

[maisonbleudeluca-zz]

Is there any provided examples to this new functionality, or could someone provide some?

[roji]

One more thought - there's also the .NET immutable collection types (e.g. ImmutableDictionary), [...]

Yes, and I added a test for it. We don't snapshot it when it's mapped as IReadOnlyDictionary<string, T>, but we still need to do a structural comparison when the reference is not equal as it still might contain the same values.

OK. At the time, when implementing support for ImmutableDictionary, I made a different decision:

Because ImmutableDictionary is immutable, we can use the default value comparer, which doesn't clone for snapshot and just does reference comparison. We could compare contents here if the references are different, but that would penalize the 99% case where a different reference means different contents, which would only save a very rare database update.

In other words, it may be OK for a comparer to return false even for structurally-equal instances, assuming the only downside is a superfluous update that could have been avoided (i.e. assuming it's purely a perf question, structurally comparing for non-equal references slows down the common case in order to benefit the rare case).

Am making a note in #25364 to think about this if/when we move the collection comparers to core.

[AndriySvyryd]

Is there any provided examples to this new functionality, or could someone provide some?

There isn't much to show, just add an array, List<> or Dictionary<string, > property of a built-in primitive type and it will work.

[maisonbleudeluca-zz]

I've tried this already, and it seems to be ignoring the Array that I parsed in the Dbset. I've also tried with Lists, but it comes to the same conclusion. Likewise, I think it's because I'm using the 5.0.8 version Nuget package that doesn't seem to have that update due to how recent it is.

[AndriySvyryd]

Likewise, I think it's because I'm using the 5.0.8 version Nuget package that doesn't seem to have that update due to how recent it is.

Yes, it was merged 3 days ago, it's only available in the latest daily build

[maalhagh]

Is there any provided examples to this new functionality, or could someone provide some?

There isn't much to show, just add an array, List<> or Dictionary<string, > property of a built-in primitive type and it will work.

I'm using version 5.0.9 and it still complains about a dictionary:
The property 'MyProperty' could not be mapped because it is of type 'Dictionary<string, string>', which is not a supported primitive type or a valid entity type.

Is there a specific version in which we should expect this feature?

[AndriySvyryd]

Is there a specific version in which we should expect this feature?

6.0.0-rc1

[shahabganji]

Is this really closed? I am using EF Core 6.0, a sample could be found here, and I am getting the following error 🤔

Unhandled exception. System.InvalidOperationException: The property 'Book.Tags' is of type 'IReadOnlyCollection<string>' which is not supported by the current database provider. Either change the property CLR type, or ignore the property using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelValidator.ValidatePropertyMapping(IModel model, IDiagnosticsLogger`1 logger)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelValidator.Validate(IModel model, IDiagnosticsLogger`1 logger)
   at Microsoft.EntityFrameworkCore.Cosmos.Infrastructure.Internal.CosmosModelValidator.Validate(IModel model, IDiagnosticsLogger`1 logger)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelRuntimeInitializer.Initialize(IModel model, Boolean designTime, IDiagnosticsLogger`1 validationLogger)
   at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.GetModel(DbContext context, ModelCreationDependencies modelCreationDependencies, Boolean designTime)
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel(Boolean designTime)
   at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
   at Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkServicesBuilder.<>c.<TryAddCoreServices>b__8_4(IServiceProvider p)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitFactory(FactoryCallSite factoryCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)

I did a workaround by writing a custom converter from IReadOnlyCollection<string> to string[] tho.

using Microsoft.EntityFrameworkCore.Storage.ValueConversion;


namespace Sample.EFCore.ArrayPrimitiveTypes.Storage.Converters;

public class TagsValueConverter : ValueConverter<IReadOnlyCollection<string>, string[]>
{
    public TagsValueConverter() : base(
        value => value.ToArray(),
        dbValue => dbValue.ToList())
    {
    }
}

[roji]

@shahabganji please open a new issue with a fully, runnable code sample that reproduces the exception.

[shahabganji]

@roji here is the new issue