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

Regression going from .NET/EF 8 to 9 with CosmosDb, will no longer save/load Ulid and Custom struct types #34936

Closed
mip1983 opened this issue Oct 18, 2024 · 5 comments

Comments

@mip1983
Copy link

mip1983 commented Oct 18, 2024

Creating this as a separate issue from #31376 , described below with repro (https://github.com/mip1983/CosmosDbEF)

Hi @roji, thanks for the input, I'm a bit confused by what your saying about the complex type support on CosmosDb, as mapping my struct as a complex type (where it's not nullable) seems to stopped one of the errors I was getting after updating to .NET 9. Just to explain the steps I've gone through:

Existing .NET 8 solution, I have a particular entity that includes properties:

With no special mapping or configuration applied in 'OnModelCreating', this entity would save and load from CosmosDb. This is an example from the Azure UI of this property on a record:
Image

Without changing anything else, just running the .NET 9 upgrade tool on the project in visual studio (which updates 'Aspire.Microsoft.EntityFrameworkCore.Cosmos' to 9.0.0-rc.1.24511.1), I get mapping errors on startup about the Ulid and DateTimeRange properties as not a known primitive type.

The Ulid property would just save/load as a string by itself somehow. I've now needed to add a Ulid converter in 'ConfigureConventions'. Not sure why it worked before and not now?

By adding this to the mapping for the non-nullable 'DateTimeRange' property:

modelBuilder.Entity<Enquiry>().ComplexProperty(o => o.DateRange);

The error goes away and my solution loads. The records seem to load ok, I haven't tried saving/updating yet. I don't know why I need this now and didn't before (edit: have tried on my repro, see link on the end, it works with this mapping).

With the nullable 'DateTimeRange', that worked before, I have some objects with null on this property and others with it populated. I'm not really sure how to map this now, I've got it set to ignore for the moment just to get it running.

For me, one of the key things about CosmosDb is that you're not bound by a flat 2d table structure, records are just objects as JSON so can have this complexity, as long as it serializes/deserializes. At least that's how it was with the CosmosDb client. I'm finding it to be one of the challenges going from the client to EF is how it handles these complex objects, it feels like it really 'wants' to be relational. I wish it would treat one object as an entity to be tracked, it doesn't matter what type or complexity a property is as long as it can be turned to/from JSON. If you attach a 'complex' object to another, it can stop tracking that complex object as it's own thing and just treat it as part of the thing it's attached to (ideally without needing to tell it that it's an owned or complex type, isn't that implicit with a record in Cosmos?). That's how it's going the get saved to the DB. Anyway, I'm digressing.

Update @roji

Repro project here: https://github.com/mip1983/CosmosDbEF

Originally posted by @mip1983 in #31376

@ajcvickers
Copy link
Contributor

Note for team: in previous releases, the Cosmos type mapper allowed value type. We brought this in line with other type mappers to only map types that are known to be compatible. However, value types can be written so they "just work" in Cosmos, and we are now blocking that. We should document this as a breaking change, if we have not already done so.

I will investigate workarounds and the potential for new API to support any serializable type explicitly. I will also investigate the use of ComplexType here, since it seems like it should not work...

@mip1983
Copy link
Author

mip1983 commented Nov 6, 2024

Thanks @ajcvickers, any work arounds would be appreciate as I'm in the midst of updating everything to .NET 9. Do you think something will be in the works for this before GA or is that just cutting it to fine?

@roji
Copy link
Member

roji commented Nov 6, 2024

@mip1983 GA is basically completely finished at this point - but we'll investigate and get back to you soon.

@ajcvickers
Copy link
Contributor

ajcvickers commented Nov 19, 2024

@mip1983 As a workaround, you can replace the Cosmos type mapper with one that will allow your types. This requires the use of internal code, but should be safe for use with EF9. We will investigate this further as part of the EF10 release. Ultimately, we may support this through custom serialization, but we're not sure exactly what that will look like.

To use a custom type mapper, use ReplaceService like this:

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.ReplaceService<ITypeMappingSource, ReplacementCosmosTypeMappingSource>();
    }

Using something like this:

public class ReplacementCosmosTypeMappingSource(TypeMappingSourceDependencies dependencies)
    : CosmosTypeMappingSource(dependencies)
{
    protected override CoreTypeMapping? FindMapping(in TypeMappingInfo mappingInfo)
    {
        var mapping = base.FindMapping(mappingInfo);
        
        if (mapping is null
            && mappingInfo.ClrType?.IsValueType == true)
        {
            return new CosmosTypeMapping(mappingInfo.ClrType);
        }

        return mapping;
    }
}

Note that there is another option for a workaround, which is to use value converters. However, value converters cannot generate JSON structure, such as you use for DateTimeRange, but rather will store the whole thing as a string in the JSON. This is probably not what you want, but I can show the code if you are interested.

@ajcvickers ajcvickers added this to the 10.0.0 milestone Nov 19, 2024
@ajcvickers ajcvickers removed their assignment Nov 26, 2024
@ajcvickers
Copy link
Contributor

Note from team meeting: closing this as by-design. Custom serialization is tracked by #17306.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Dec 3, 2024
@ajcvickers ajcvickers removed this from the 10.0.0 milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants