SetOutputIdentity: true broken since update to .Net 9.0

[angelaki]

I'm not quite sure (yet) if this issue was caused by an update of this lib or .Net 9.0 but right now the returned Ids are totally wrong! They weren't before my update. Is anyone already aware of this? Meanwhile I'm checking if the issue is caused by .Net 9.0 or this lib.

[angelaki]

The issue was introduced with 8.1.2. - no relation to .Net 9.0 as far as I can tell. 9.0.0-rc.1 doesn't seam to solve it!

[jhfreestyle]

Hi,
could it be related to this recent commit ?
#1578
#1578
I'm experiencing same issue since upgrade from 8.0.2 to 8.1.2, using sqlserver.
Return ids are wrong and mess up all related entites.

[borisdj]

Is this on SqlServer and are you using just BulkInsert or BulkInsertOrUpdate, or combining it also with IncludeGraph?
And can you make a reproducale test.
Could be the same issue as #1629

[jhfreestyle]

Yes similar ! Using sqlserver, bulkinsertOrUpdate with setOutputId true and preserveOrder true. I'm also preloading new entites with id from -count to -1 (instead 0)
Will do more tests in coming days after christmas
Thanks

[angelaki]

Maybe I find some time for a repro in the first days of January. But yeah, guess could be related to this commit, checked the history myself and thought, that it actually could only be this one.

[PhideasAmbrosianus]

I can confirm I am seeing the same thing. Not sure if it's broken on everything or not, it seems like sometimes it's working and sometimes it is not for me.

I went from 3.1.1 to 3.1.2 and then I experienced the issue. I am replicating in MSSQL EXPRESS.

I am NOT using .NET 9, still on .NET 8

My implementation is this:

// After this write, I check database and Ids SQL database assigned are not set back properly
await _context.BulkInsertAsync(SampleRecords, new BulkConfig { SetOutputIdentity = true });

No includes or graphs and this fails.

I noticed that in 3.1.1, the Ids database assigns are in order of the List I am providing to BulkInsertAsync.
In 3.1.2, the Ids aren't sequentially ordered - they're all there but kind of random.

The table in question does have navigations to other tables - but I also see it working correctly in navigations with other tables. I cannot see any logical pattern that causes one insert set to work Vs. others. In this app, I am doing multiple different BulkInserts of different types and some of them the Ids are assigned right and some are not.

[Liebeck]

We also noticed this "sometimes the order is random" in one of our projects. Unfortunately, some of our production data was compromised and customer data got mixed :-(
Fortunately, we were able to fix it, but it took us 2 days :-( We pinned to BulkExtensions to version 8.1.1 for now

[angelaki]

We also noticed this "sometimes the order is random" in one of our projects. Unfortunately, some of our production data was compromised and customer data got mixed :-( Fortunately, we were able to fix it, but it took us 2 days :-( We pinned to BulkExtensions to version 8.1.1 for now

How did you solve it? By downgrading to version 8.1.2? Or have you found another workaround? And yeah, blew up my production data pretty much, too.

[angelaki]

Argh! Felt again for this version 8.1.2. @borisdj you need to take it offline! This one actually is super dangerous! Sorry I didn't find the time to create a repro, yet. But that one really messes with your data!

[borisdj]

New version with latest source are published v 8.1.3 for EF8 and v 9.0.1 for EF9.
Make a test for the issue with these versions.

[angelaki]

Yep, works for me 👌. @Liebeck can you maybe confirm?

@borisdj have you found an actual reason for this misbehavior? Want to get sure before switching to the latest version agains. Sorry but a bit afraid, that data-mess almost killed me. All appointments in my app got matched to different clients etc.. Wasn't that funny 😉

But never the less: thank you so much for your effort and this great library!!

[borisdj]

@angelaki sorry for the trouble.
There were several PRs merged and few more changes and commits in a short period before and after release of 8.1.2, and in addition was updating code to EF9.
It seems that this bug was short lived in the source and also did not show up with basic tests.
I wasn't able to pinpoint it exactly, as I did not get a reproducible sample.
Best would be to make some test for the ordering and have it run in your app before publish, especially when you upgrade the package to newer version.

[angelaki]

Ok. So I'd still check if I can build a repro so you can maybe find the issue origin and maybe add a test for it?

No need to be sorry! Great work you're doing here, I'm totally fine! Mistakes happen :)

[borisdj]

Sure, will add a test it if you manage to make it reproducible.

[angelaki]

Oh wow ... it took me pretty much effort to build the repro ... but I finally did it ... Here we go:

using EFCore.BulkExtensions;
using Microsoft.EntityFrameworkCore;
using System.Text.Json;

namespace EFCoreBulkInsert;

public class UnitTest1
{
    [Fact]
    public async Task Test1()
    {
        var clients = JsonSerializer.Deserialize<ObjectA[]>(File.ReadAllText("data.json"))!;

        var clientProcesses = clients.Where(c => c.Processes != null).SelectMany(c => c.Processes).ToArray();
        var clientProcessActivities = clientProcesses.SelectMany(p => p.Activities).ToArray();

        using (var context = new AppDbContext("TestDb_813"))
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            await context.BulkInsertAsync(clients, new BulkConfig { SetOutputIdentity = true });

            foreach (var c in clients.Where(c => c.Processes != null)) { foreach (var p in c.Processes) { p.ClientId = c.Id; } }
            await context.BulkInsertAsync(clientProcesses, new BulkConfig { SetOutputIdentity = true });
            foreach (var c in clients.Where(c => c.Processes != null)) { foreach (var p in c.Processes) { foreach (var a in p.Activities) { a.ClientProcessId = p.Id; } } }
            await context.BulkInsertAsync(clientProcessActivities);

            await context.SaveChangesAsync();
        }

        using (var context = new AppDbContext("TestDb_813"))
        {
            var data = context.Set<ObjectC>().First(c => c.Id == 39291);
            Assert.Equal(3266, data.ClientProcessId);
        }
    }
}

public class AppDbContext(string Name) : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder options)
        => options.UseSqlServer($"Server=localhost;Database={Name};Trusted_Connection=True;TrustServerCertificate=True;");

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<ObjectA>(p =>
        {
            p.ToTable("A");
            p.Property(p => p.Id).ValueGeneratedOnAdd();
            p.HasMany(p => p.Processes).WithOne().HasForeignKey(p => p.ClientId);
        });

        modelBuilder.Entity<ObjectB>(p =>
        {
            p.ToTable("B");
            p.Property(p => p.Id).ValueGeneratedOnAdd();
            p.HasMany(p => p.Activities).WithOne().HasForeignKey(p => p.ClientProcessId);
        });

        modelBuilder.Entity<ObjectC>(p =>
        {
            p.ToTable("C");
            p.Property(p => p.Id).ValueGeneratedOnAdd();
        });
    }
}

public class ObjectA
{
    public int Id { get; set; }
    public virtual ICollection<ObjectB> Processes { get; set; }
}

public class ObjectB
{
    public int Id { get; set; }
    public int ClientId { get; set; }
    public string? ShortDescription { get; set; }

    public virtual ICollection<ObjectC> Activities { get; set; }
}

public class ObjectC
{
    public int Id { get; set; }
    public int ClientProcessId { get; set; }
}

Executing this with 8.1.2 vs. 8.1.3 results in different data! Problem is, that the data.json contains possibly sensitive data in ShortDescription and obfuscating it leads to correct results! It seams to be some kind of value in it!

@borisdj are you interessted in figuring out whats wrong here? I could send you the file via mail.

[borisdj]

You can send it to mail.

[angelaki]

@borisdj just sent you the file. I'm super excited if you can find the reason for this issue!

[borisdj]

@angelaki I did some more investigation but still no luck.
However you could try to take several versions from the commit history and put test into each to check.
Take a look at a list on this page (there is one with desc: Version upgrade 8.1.2):
https://github.com/borisdj/EFCore.BulkExtensions/commits/master/?after=481c0045dfc761528d400f4121b8fe5788a55fbf+69

If you find 2 sources (prior and after) that works correctly and incorrectly than by using Bisection method could with few hops isolate the one where the issue started, then we could inspect the changes in that commit or debug it.

[angelaki]

@borisdj but have you at least been able to reproduce the issue? Just to make sure. So it's just checking commit for commit for finding the first one having this issue, right?

[borisdj]

Not sure, I have downloaded this version, copied files from v8 folder to overwrite csproj(s).
Then added you your test and put the file you sent.
The test had passed, but there are not Asserts in the test, and not sure what order you expected.

Change a test to make it check for expected order and to fail if not valid.
Then when taking commit do not go one after another, instead take 2 that are say 20 or so commits apart, then take one in the middle, then middle from one side, etc (that hop method is called Bisection and it enable faster search)

[angelaki]

Ok, fair - this isn't a real test, yet 😉. But I hoped it might be enough for you to find the issue. If you run it twice with different versions and compare the created databases, you'll see they differ. Since the data provided shouldn't be used in a public test atm I thought it might be enough for you to find the issue and define a test actually hitting the problem's source.

And sure, not gonna check them one by one - but thanks for the hint!

[angelaki]

@borisdj Ok, here we got! Commit 5af41f2df4e6189e798cf7dc4dd3fc69ba37523b breaks the behavior - for some reason?! Except the typo in the Commit Message I can't find any issue with it? I adopted the unit test above so it actually fails now.

Could you be so kind and take this final step to find the issue? Seams so weird to me ...

[kvpt]

The second change in this commit looks suspicious, comparing two values by checking the results of toString call is not a good way to do that.
@angelaki I made a PR to fix that, not sure if it will fix your issue though.
You can run your repro with this code change to check if it change something ?
If it was the change that caused this issue to appear it should fix it.

[borisdj]

Seems that this line was the cause:
var property = type.GetProperty(name, BindingFlags.DeclaredOnly | BindingFlags.Public | BindingFlags.Instance);
it was changed from:
var property = type.GetProperty(name, BindingFlags.DeclaredOnly);

It is in a method GetPropertyUnambiguous that is called from 'OrderBy' function.
And looks like it is still broken, but still not sure why it is making this problem.

@Zinoberous maybe you have some ideas as you have made the PR:
e7053e1

[angelaki]

@borisdj yeah, actually still broken. So weired ... But guess you haven't checked any deep what is actually causing this issue? Lucky me hasn't relied on the latest version eventhough at the time testing the data constellation was working!

[angelaki]

Any news on this? It took me so much effort to reproduce this issue it will break my heart if it won't get fixed ;)

@borisdj have you found the reason for it? Must be super weird since even changing the values of my dataset changes the result.
And @kvpt if you're interested in reproducing / finding the issue, I can send you the datafile, too?

[borisdj]

@kvpt didn't catch the time to dig more into this.
Don't worry will sort it out soon.
Either will remove (or refactor) the change that caused the issue, but first will to try to figure the exact reason why it is causing the issue.
In either way it will be fixed.