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

ExecuteUpdate on Json properties fails. #3348

Closed
onionhammer opened this issue Nov 4, 2024 · 7 comments
Closed

ExecuteUpdate on Json properties fails. #3348

onionhammer opened this issue Nov 4, 2024 · 7 comments
Labels
duplicate This issue or pull request already exists

Comments

@onionhammer
Copy link

onionhammer commented Nov 4, 2024

Configure model builder as follows:

        // Configure users
        modelBuilder.Entity<User>(entity =>
        {
            entity.OwnsOne(e => e.SystemRole, builder =>
            {
                builder.Property(e => e.Role).HasConversion<string>();
                builder.ToJson();
            });
          }

Execute an update on the the property in question

        var rows = await db.Users.Where(u => u.Id == userId). ExecuteUpdate(
            u => u
                .SetProperty(u => u.SystemRole, systemRole),
            CancellationToken.None);

Fails with:

        .ExecuteUpdate(u => u.SetProperty<string>(
            propertyExpression: p => p.FirstName, 
            valueExpression: "John").SetProperty<RoleAssignment<SystemRole>>(
            propertyExpression: u => u.SystemRole, 
            valueExpression: __systemRole_1))' could not be translated. Additional information: The following lambda argument to 'SetProperty' does not represent a valid property to be set: 'u => u.SystemRole'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

Succeeds if the configuration changes:


            entity.Property(e => e.SystemRole).HasColumnType("jsonb");
@roji
Copy link
Member

roji commented Nov 4, 2024

@onionhammer I can't really look into issues in UpdateBatchAsync() since that's not part of EF or the PostgreSQL provider - it's a 3rd-party extension.

@onionhammer
Copy link
Author

onionhammer commented Nov 4, 2024

@roji sorry, that's my own helper wrapper of ExecuteUpdate leaking into the bug's description, I fixed the description. ExecuteUpdateAsync is the code undergirding that method which is failing (per the stacktrace) - if this isnt a PG specific issue, perhaps we can transfer this issue to the efcore repo?

https://github.com/dotnet/efcore/blob/147264444aca02fcc86544893e3890411f3bc2bb/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.ExecuteUpdate.cs#L126

Is throwing, even though SystemRole is just a JSONB column.

@onionhammer onionhammer changed the title UpdateBatchAsync on Json properties fails. ExecuteUpdate on Json properties fails. Nov 4, 2024
@roji
Copy link
Member

roji commented Nov 4, 2024

I'm still not sure exactly what you're reporting, because you're posting snippets; please always submit a minimal, runnable code sample (e.g. a console program), otherwise it's hard to understand what you're trying to achieve.

Specifically, you seem to be saying that if you do HasColumnType("jsonb"), the update succeeds - it's expected that you need to do that.

@onionhammer
Copy link
Author

onionhammer commented Nov 5, 2024

@roji Sorry I wasn't clear enough in the initial thread, I'll be less terse here:

Both code snippets below result in a "JSONB" column in the actual underlying database, but only the first example allows you to reassign the property (which goes into the JSONB column in the database)

Example 1: Allows ExecuteUpdate

using Microsoft.EntityFrameworkCore;

namespace C2C.Connect.Repository;

public class PostgresContext(DbContextOptions options) : DbContext(options)
{
    internal DbSet<Model.UserIdentity> Users => Set<Model.UserIdentity>();

    /// <summary>
    /// Configure the database context
    /// </summary>
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        // Configure users
        modelBuilder.Entity<Model.UserIdentity>(entity =>
        {
            entity.ToTable("users");
            entity.HasKey(e => e.Id);
            entity.Property(e => e.SystemRole).HasColumnType("jsonb"); // <-- `SystemRole` is a json column on `users` table
        });     
    }
}

Example 2: Throws an exception when running ExecuteUpdate

using Microsoft.EntityFrameworkCore;

namespace C2C.Connect.Repository;

public class PostgresContext(DbContextOptions options) : DbContext(options)
{
    internal DbSet<Model.UserIdentity> Users => Set<Model.UserIdentity>();

    /// <summary>
    /// Configure the database context
    /// </summary>
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        // Configure users
        modelBuilder.Entity<Model.UserIdentity>(entity =>
        {
            entity.ToTable("users");
            entity.HasKey(e => e.Id);

            entity.OwnsOne(e => e.SystemRole, builder =>
            {
                builder.Property(e => e.Role).HasConversion<string>();
                builder.ToJson(); // <--- `SystemRole` is a json column on `users` table
            });
        });
    }
}

The code used to modify the table would look like this:

    var systemRole = new SystemRole("Administrator");
    await db.Users.Where(u => u.Id == userId). ExecuteUpdateAsync(
            u => u.SetProperty(u => u.SystemRole, systemRole),
            CancellationToken.None);

The 2nd example fails with the exception above

      .ExecuteUpdate(u => u.SetProperty<string>(
            propertyExpression: p => p.FirstName, 
            valueExpression: "John").SetProperty<RoleAssignment<SystemRole>>(
            propertyExpression: u => u.SystemRole, 
            valueExpression: __systemRole_1))' could not be translated. Additional information: The following lambda argument to 'SetProperty' does not represent a valid property to be set: 'u => u.SystemRole'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

I would be happy to build an entire repro repository for you if you would find that valuable, but it would potentially be a wasted hour-long exercise if you already knew exactly what was going on.

@roji
Copy link
Member

roji commented Nov 5, 2024

Thanks for the additional detail - that all makes sense. The first mapping style is the "legacy", Npgsql-specific jsonb POCO mapping, whereas the second is the more general EF owned entity-based ToJson() method. It indeed makes sense that doing ExecuteUpdate() on the root of a ToJson()-mapped JSON column doesn't work - this is a general EF limitation rather than something Npgsql-specific.

dotnet/efcore#28766 tracks allowing changing properties inside a mapped JSON document (so partial update, rather than replacing the entire document; not quite what you want). dotnet/efcore#32719 more more or less tracks what you want: it's primarily concerned with non-owned entity types in ExecuteUpdate() (so entities mapped to a different table with a foreign key); but there's some analysis at the bottom around owned entities.

More generally, the direction in EF 10 is going to be to allow mapping JSON columns via complex types, instead of the current owned entity approach (which has many problems). EF 8 already supports using complex types for non-JSON scenarios (table splitting, i.e. map some columns of the table to the complex type), and dotnet/efcore#32058 also added support for including such non-JSON complex types in ExecuteUpdate(). I hope that as part of this work, it will also be possible to update JSON-mapped complex types, so you'll have to wait until that's done (in the meantime, you can use SQL to express your UPDATE).

I'll go ahead and close this as an EF-side issue - let me know if that all makes sense.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Nov 5, 2024
@roji
Copy link
Member

roji commented Nov 5, 2024

Duplicate of dotnet/efcore#32719

@roji roji marked this as a duplicate of dotnet/efcore#32719 Nov 5, 2024
@roji roji added the duplicate This issue or pull request already exists label Nov 5, 2024
@onionhammer
Copy link
Author

onionhammer commented Nov 5, 2024

Makes sense, thanks @roji !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants