borisdj / EFCore.BulkExtensions

Entity Framework EF Core efcore Bulk Batch Extensions with BulkCopy in .Net for Insert Update Delete Read (CRUD), Truncate and SaveChanges operations on SQL Server, PostgreSQL, MySQL, SQLite
https://codis.tech/efcorebulk
Other
3.67k stars 588 forks source link

System.Text.Json.JsonReaderException on BulkInsertAsync when SetOutputIdentity = true, and IsJson column is null #1572

Closed manuel-fernandez-rodriguez closed 1 week ago

manuel-fernandez-rodriguez commented 1 month ago

Description

BulkInsertAsync bombs out in SQL Server when trying to insert a row with a null value for a column configured as IsJson.

Interestingly, it only dies if you request it to retrieve the identity column value, as shown in the repro steps below.

Seems to be loosely related to #1565

Reproducing the bug

proof.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="EFCore.BulkExtensions" Version="8.1.1" />    
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="8.0.8" />
  </ItemGroup>
</Project>

Program.cs

using EFCore.BulkExtensions;
using Microsoft.EntityFrameworkCore;

using var context = new DataContext();

context.Database.EnsureDeleted();
context.Database.EnsureCreated();

await context.BulkInsertAsync([new People() { Name = "John Doe", Address = new() { Street = "1428 Elm St." } }]);
Console.WriteLine("John is saved!");

await context.BulkInsertAsync([new People() { Name = "Jane Homeless", Address = null }]);
Console.WriteLine("Jane is safe, too");

await context.BulkInsertAsync([new People() { Name = "Private Ryan", Address = null }], c => c.SetOutputIdentity = true);
// System.Text.Json.JsonReaderException:
// ''0x00' is invalid after a single JSON value. Expected end of data. LineNumber: 0 | BytePositionInLine: 4.'

Console.WriteLine("Oh, no! we failed at saving Private Ryan!");

public class People {
    public long Id { get; set; }
    public string Name { get; set; }
    public Address Address { get; set; }
}

public class Address
{
    public string Street { get; set; }
}

public class DataContext : DbContext
{
    public DbSet<People> People { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) =>
        optionsBuilder.UseSqlServer("REDACTED");

    protected override void OnModelCreating(ModelBuilder modelBuilder) =>
        modelBuilder.Entity<People>().OwnsOne(p => p.Address, p => p.ToJson());
}
manuel-fernandez-rodriguez commented 1 month ago

After reviewing the library code, I have found a partial workaround.

If you're only interested in retrieving the identity column and not any other potentially DB generated columns, you can instruct the library to only retrieve identity columns by replacing the bombing out line in the repro above by:

await context.BulkInsertAsync([new People() { Name = "Private Ryan", Address = null }], c => 
{
    c.SetOutputIdentity = true;
    c.SetOutputNonIdentityColumns = false;
});
borisdj commented 2 weeks ago

The issue seems caused by Mapping null from Expression Builder into Json property which does work with current ExpressionBuilder. And when only PK is loaded Builder append SelectPK so it works. Not sure how to improve this of possible, so will add a comment regarding this into ReadMe.

manuel-fernandez-rodriguez commented 2 weeks ago

@borisdj I don't think this should be labelled as a question, from my humble point of view, it is a bug.

And, since, it hasn't been solved, I also think the issue should remain open, so that other people can see this problem exists and contribute, or at least, to prevent them to waste the same time it took for me to diagnose it.

borisdj commented 2 weeks ago

I can agree. Still since do not have a solution, will label it as Enhancement and leave it open for now. As for info it is in ReadMe, more people will find it there then among issues, and search works also for closed issues. If anyone comes up with a fix or proposal write it or make a PR.

manuel-fernandez-rodriguez commented 1 week ago

@borisdj I've got good news for you :-)

I found the reason of the exception and turns out we were following a red herring.

The root of the problem wasn't really reading from the Output table, but more like in the handling of null values in Json by EF Core.

I found the issue dotnet/efcore#34960 that gave me a hint of what might be happening.

In the end, the problem is due to the fact that, when reading and storing an owned Json entity whose value happens to be null in the database, your library was storing the string "null", which, altho is a valid Json value, EF Core seems to be unable to understand.

So, I presumed that storing a real null instead of the string should fix the problem.

Thus, I dug into your code and found the place where the mishandling of null values was taking place, amended it, re-executed my repro with that change, and everything worked as expected.

I will submit a PR that fixes the issue.

borisdj commented 1 week ago

Thx for contrib.

manuel-fernandez-rodriguez commented 1 week ago

Thx for contrib.

My pleasure 😊