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

Trying to save a non-scalar nested JSON collection (List<List<X>>) saves incorrect data #33913

Closed
roji opened this issue Jun 5, 2024 · 6 comments · Fixed by #34264
Closed
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug

Comments

@roji
Copy link
Member

roji commented Jun 5, 2024

In the below, when saving List<List<MyEntity>>, SaveChanges() sends: @p0='[{"Capacity":2},{"Capacity":2}]', which seems to be the ToString() representation of the nested List.

Assuming List<List<T>> is an unsupported scenario, we should catch this in model validation.

await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

context.Add(new MyEntity
{
    JsonbFields = [
        [new() { Key = "key1", Value = "value1"}, new() { Key = "key2", Value = "value2"}],
        [new() { Key = "key1", Value = "value1"}, new() { Key = "key2", Value = "value2"}]
    ]
});

context.SaveChanges();

public class BlogContext : DbContext
{
    public DbSet<MyEntity> MyEntity { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<MyEntity>().OwnsMany(x => x.JsonbFields, r => r.ToJson());
    }
}

public class MyEntity
{
    public int Id { get; set; }
    public List<List<JsonbField>>? JsonbFields { get; set; }
}

public class JsonbField
{
    public required string Key { get; set; }
    public required string Value { get; set; }
}

Reported by @zN3utr4l in npgsql/efcore.pg#3172

@ajcvickers
Copy link
Member

Notes following team discussion:

  • I verified that this is working "as designed" in that the entity type is List<JsonbField>--see model debug view below.
  • Queries also work "fine". (Note that "working" here means that Capacity, which is the only read-write property, is persisted to and from the database. The list contents is not persisted.)
  • Non-concrete types (e.g. ICollection) do not have this issue because they can't be mapped as an owned entity type by default.
  • Even though everything is working "by-design" here, it isn't working in the sense that the data is not being persisted. It's hard to see how doing this by convention can ever be correct.

Bottom line: we should blog this in validation if it is done by convention. Open question: what does "by convention" mean in this case? The code above is explicitly mapping the entity type List<JsonbField> using OwnsMany.

Model: 
  EntityType: MyEntity
    Properties: 
      Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd
    Navigations: 
      JsonbFields (List<List<JsonbField>>) Collection ToDependent List<JsonbField>
    Keys: 
      Id PK
  EntityType: List<JsonbField> Owned
    Properties: 
      MyEntityId (no field, int) Shadow Required PK FK AfterSave:Throw
      Id (no field, int) Shadow Required PK AfterSave:Throw ValueGenerated.OnAdd
      Capacity (no field, int) Required
    Keys: 
      MyEntityId, Id PK
    Foreign keys: 
      List<JsonbField> {'MyEntityId'} -> MyEntity {'Id'} Required Ownership Cascade ToDependent: JsonbFields```

@ajcvickers ajcvickers self-assigned this Jul 1, 2024
@ajcvickers ajcvickers added this to the 9.0.0 milestone Jul 11, 2024
@ajcvickers
Copy link
Member

Note from design meeting: error, but make it suppressible.

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 21, 2024
@zN3utr4l
Copy link

Hi @ajcvickers, I just saw today that the problem has been "fixed", but a version hasn't been released yet, when is the release of the version containing this fix scheduled for release?

I saw the code of the pull and added the exception "AccidentalEntityType" which says

The type '{entityType}' has been mapped as an entity type. If you are mapping this type intentionally, then please suppress this warning and report the issue on GitHub

I don't understand if my problem is not resolved or something else, with the new version the List<List<JsonbField>> field will be corrected or it will throw this exception and once again the db will show the entity in the wrong way?

[
  { "Capacity": 2 },
  { "Capacity": 2 }
]

So as you say in this #33913 (comment) the correct way to instantiate this field would be using an OwnsMany, even though in .NET 6 the List<List<JsonbField>>field is instantiated correctly?

@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview7 Jul 29, 2024
@zN3utr4l
Copy link

zN3utr4l commented Jul 29, 2024

Unable to create a 'DbContext' of type ''. The exception 'Owned entity type 'JsonbField' is mapped to table 'public.JsonbField' and contains JSON columns. This is currently not supported. All owned types containing a JSON column must be mapped to a JSON column themselves.' was thrown while attempting to create an instance. For the different patterns supported at design time, see https://go.microsoft.com/fwlink/?linkid=851728

OwnsMany throws an error, or maybe I'm not mapping the column well, these are the changes I made:

Models

namespace ErrorNpgsql.Models;

public class JsonbFieldContent
{
    public required string Key { get; set; }

    public required string Value { get; set; }
}

public class JsonbField
{
    public List<JsonbFieldContent>? Content { get; set; }

}

public class MyEntity
{
    public int Id { get; set; }

    public required ICollection<JsonbField> JsonbFields { get; set; }
}

Configuration

using ErrorNpgsql.Models;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Builders;

namespace ErrorNpgsql.Configuration;

internal class MyEntityConfiguration : IEntityTypeConfiguration<MyEntity>
{
    public void Configure(EntityTypeBuilder<MyEntity> builder)
    {
        builder.OwnsMany(x => x.JsonbFields, j =>
        {
            j.OwnsOne(x => x.Content, c => { c.ToJson(); });
        });
    }
}

I also tried adding the public int Id { get; set; } field to the JsonbField entity and adding these lines in the OwnsMany configurations:

builder.OwnsMany(x => x.JsonbFields, j =>
{
    //NEW
    j.WithOwner().HasForeignKey(nameof(MyEntity));

    //NEW
    j.HasKey("Id"); // shadow id property

    j.OwnsOne(x => x.Content, c => { c.ToJson(); });
});

@ajcvickers
Copy link
Member

ajcvickers commented Aug 12, 2024

@zN3utr4l Mapping of nested collections of primitive types like List<List<int>> is not supported by relational database providers. See #30713

@zN3utr4l
Copy link

zN3utr4l commented Aug 20, 2024

Hi @ajcvickers, I understood this, but if you see my example above #33913 (comment), you can't even do a FK Relation (OwnsMany).

Please take a look at the table structure I had thought of.

Maybe it's wrong and it can be structured in another way, because the error is "All owned types containing a JSON column must be mapped to a JSON column themselves", and I don't think it has anything to do with List<List>.

In this comment #33913 (comment), you suggest:

The code above is explicitly mapping the entity type List<JsonbField> using OwnsMany.

@zN3utr4l zN3utr4l mentioned this issue Aug 20, 2024
34 tasks
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants