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

Fix to #30244 - Json column fails to materialize only when default interceptor used #30403

Merged
merged 2 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/EFCore/Query/Internal/EntityMaterializerSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,14 @@ BlockExpression CreateAccessorDictionaryExpression()
Expression CreateAccessorReadExpression()
=> property is IServiceProperty serviceProperty
? serviceProperty.ParameterBinding.BindToParameter(bindingInfo)
: valueBufferExpression
.CreateValueBufferReadValueExpression(
: (property as IProperty)?.IsPrimaryKey() == true
? Expression.Convert(
valueBufferExpression.CreateValueBufferReadValueExpression(
typeof(object),
property.GetIndex(),
property),
property.ClrType)
: valueBufferExpression.CreateValueBufferReadValueExpression(
property.ClrType,
property.GetIndex(),
property);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,32 @@

namespace Microsoft.EntityFrameworkCore.Cosmos;

public class MaterializationInterceptionCosmosTest : MaterializationInterceptionTestBase,
public class MaterializationInterceptionCosmosTest : MaterializationInterceptionTestBase<MaterializationInterceptionCosmosTest.CosmosLibraryContext>,
IClassFixture<MaterializationInterceptionCosmosTest.MaterializationInterceptionCosmosFixture>
{
public MaterializationInterceptionCosmosTest(MaterializationInterceptionCosmosFixture fixture)
: base(fixture)
{
}

public class CosmosLibraryContext : LibraryContext
{
public CosmosLibraryContext(DbContextOptions options)
: base(options)
{
}

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

modelBuilder.Entity<TestEntity30244>();
}
}

public override LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
=> new CosmosLibraryContext(Fixture.CreateOptions(interceptors, inject));

public class MaterializationInterceptionCosmosFixture : SingletonInterceptorsFixtureBase
{
protected override string StoreName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,32 @@

namespace Microsoft.EntityFrameworkCore;

public class MaterializationInterceptionInMemoryTest : MaterializationInterceptionTestBase,
public class MaterializationInterceptionInMemoryTest : MaterializationInterceptionTestBase<MaterializationInterceptionInMemoryTest.InMemoryLibraryContext>,
IClassFixture<MaterializationInterceptionInMemoryTest.MaterializationInterceptionInMemoryFixture>
{
public MaterializationInterceptionInMemoryTest(MaterializationInterceptionInMemoryFixture fixture)
: base(fixture)
{
}

public class InMemoryLibraryContext : LibraryContext
{
public InMemoryLibraryContext(DbContextOptions options)
: base(options)
{
}

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

modelBuilder.Entity<TestEntity30244>().OwnsMany(e => e.Settings);
}
}

public override LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
=> new InMemoryLibraryContext(Fixture.CreateOptions(interceptors, inject));

public class MaterializationInterceptionInMemoryFixture : SingletonInterceptorsFixtureBase
{
protected override string StoreName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ public virtual async Task Predicate_based_on_element_of_json_array_of_primitives
else
{
Assert.Throws<InvalidOperationException>(() => query.ToList());
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

namespace Microsoft.EntityFrameworkCore;

public abstract class MaterializationInterceptionTestBase : SingletonInterceptorsTestBase
public abstract class MaterializationInterceptionTestBase<TContext> : SingletonInterceptorsTestBase<TContext>
where TContext : DbContext
{
protected MaterializationInterceptionTestBase(SingletonInterceptorsFixtureBase fixture)
: base(fixture)
Expand Down Expand Up @@ -169,6 +170,97 @@ public virtual void Intercept_query_materialization_for_empty_constructor(bool i
}
}

private static int _id;

[ConditionalTheory] // Issue #30244
[InlineData(false)]
[InlineData(true)]
public virtual async Task Intercept_query_materialization_with_owned_types(bool async)
{
var creatingInstanceCounts = new Dictionary<Type, int>();
var createdInstanceCounts = new Dictionary<Type, int>();
var initializingInstanceCounts = new Dictionary<Type, int>();
var initializedInstanceCounts = new Dictionary<Type, int>();
LibraryContext? context = null;

var interceptors = new[]
{
new ValidatingMaterializationInterceptor(
(data, instance, method) =>
{
Assert.Same(context, data.Context);
Assert.Equal(QueryTrackingBehavior.TrackAll, data.QueryTrackingBehavior);

int count;
var clrType = data.EntityType.ClrType;
switch (method)
{
case nameof(IMaterializationInterceptor.CreatingInstance):
count = creatingInstanceCounts.GetOrAddNew(clrType);
creatingInstanceCounts[clrType] = count + 1;
Assert.Null(instance);
break;
case nameof(IMaterializationInterceptor.CreatedInstance):
count = createdInstanceCounts.GetOrAddNew(clrType);
createdInstanceCounts[clrType] = count + 1;
Assert.Same(clrType, instance!.GetType());
break;
case nameof(IMaterializationInterceptor.InitializingInstance):
count = initializingInstanceCounts.GetOrAddNew(clrType);
initializingInstanceCounts[clrType] = count + 1;
Assert.Same(clrType, instance!.GetType());
break;
case nameof(IMaterializationInterceptor.InitializedInstance):
count = initializedInstanceCounts.GetOrAddNew(clrType);
initializedInstanceCounts[clrType] = count + 1;
Assert.Same(clrType, instance!.GetType());
break;
}
})
};

using (context = CreateContext(interceptors, inject: true))
{
context.Add(
new TestEntity30244
{
Id = _id++,
Name = "TestIssue",
Settings = { new("Value1", "1"), new("Value2", "9") }
});

_ = async
? await context.SaveChangesAsync()
: context.SaveChanges();

context.ChangeTracker.Clear();

var entity = async
? await context.Set<TestEntity30244>().OrderBy(e => e.Id).FirstOrDefaultAsync()
: context.Set<TestEntity30244>().OrderBy(e => e.Id).FirstOrDefault();

Assert.NotNull(entity);
Assert.Contains(("Value1", "1"), entity.Settings.Select(e => (e.Key, e.Value)));
Assert.Contains(("Value2", "9"), entity.Settings.Select(e => (e.Key, e.Value)));

Assert.Equal(2, creatingInstanceCounts.Count);
Assert.Equal(1, creatingInstanceCounts[typeof(TestEntity30244)]);
Assert.Equal(2, creatingInstanceCounts[typeof(KeyValueSetting30244)]);

Assert.Equal(2, createdInstanceCounts.Count);
Assert.Equal(1, createdInstanceCounts[typeof(TestEntity30244)]);
Assert.Equal(2, createdInstanceCounts[typeof(KeyValueSetting30244)]);

Assert.Equal(2, initializingInstanceCounts.Count);
Assert.Equal(1, initializingInstanceCounts[typeof(TestEntity30244)]);
Assert.Equal(2, initializingInstanceCounts[typeof(KeyValueSetting30244)]);

Assert.Equal(2, initializedInstanceCounts.Count);
Assert.Equal(1, initializedInstanceCounts[typeof(TestEntity30244)]);
Assert.Equal(2, initializedInstanceCounts[typeof(KeyValueSetting30244)]);
}
}

[ConditionalTheory]
[InlineData(false)]
[InlineData(true)]
Expand Down
34 changes: 28 additions & 6 deletions test/EFCore.Specification.Tests/SingletonInterceptorsTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

namespace Microsoft.EntityFrameworkCore;

public abstract class SingletonInterceptorsTestBase
public abstract class SingletonInterceptorsTestBase<TContext>
where TContext : DbContext
{
protected SingletonInterceptorsTestBase(SingletonInterceptorsFixtureBase fixture)
{
Expand Down Expand Up @@ -46,9 +47,31 @@ public Pamphlet(Guid id, string? title)
public string? Title { get; set; }
}

public class LibraryContext : PoolableDbContext
public class TestEntity30244
{
public LibraryContext(DbContextOptions options)
[DatabaseGenerated((DatabaseGeneratedOption.None))]
public int Id { get; set; }

public string? Name { get; set; }
public List<KeyValueSetting30244> Settings { get; } = new();
}

public class KeyValueSetting30244
{
public KeyValueSetting30244(string key, string value)
{
Key = key;
Value = value;
}

public string Key { get; set; }
public string Value { get; set; }
}


public abstract class LibraryContext : PoolableDbContext
{
protected LibraryContext(DbContextOptions options)
: base(options)
{
}
Expand All @@ -69,10 +92,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
}
}

public LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
=> new(Fixture.CreateOptions(interceptors, inject));
public abstract LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject);

public abstract class SingletonInterceptorsFixtureBase : SharedStoreFixtureBase<LibraryContext>
public abstract class SingletonInterceptorsFixtureBase : SharedStoreFixtureBase<TContext>
{
public virtual DbContextOptions CreateOptions(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,32 @@

namespace Microsoft.EntityFrameworkCore;

public class MaterializationInterceptionSqlServerTest : MaterializationInterceptionTestBase,
public class MaterializationInterceptionSqlServerTest : MaterializationInterceptionTestBase<MaterializationInterceptionSqlServerTest.SqlServerLibraryContext>,
IClassFixture<MaterializationInterceptionSqlServerTest.MaterializationInterceptionSqlServerFixture>
{
public MaterializationInterceptionSqlServerTest(MaterializationInterceptionSqlServerFixture fixture)
: base(fixture)
{
}

public class SqlServerLibraryContext : LibraryContext
{
public SqlServerLibraryContext(DbContextOptions options)
: base(options)
{
}

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

modelBuilder.Entity<TestEntity30244>().OwnsMany(e => e.Settings, b => b.ToJson());
}
}

public override LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
=> new SqlServerLibraryContext(Fixture.CreateOptions(interceptors, inject));

public class MaterializationInterceptionSqlServerFixture : SingletonInterceptorsFixtureBase
{
protected override string StoreName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,32 @@

namespace Microsoft.EntityFrameworkCore;

public class MaterializationInterceptionSqliteTest : MaterializationInterceptionTestBase,
public class MaterializationInterceptionSqliteTest : MaterializationInterceptionTestBase<MaterializationInterceptionSqliteTest.SqliteLibraryContext>,
IClassFixture<MaterializationInterceptionSqliteTest.MaterializationInterceptionSqliteFixture>
{
public MaterializationInterceptionSqliteTest(MaterializationInterceptionSqliteFixture fixture)
: base(fixture)
{
}

public class SqliteLibraryContext : LibraryContext
{
public SqliteLibraryContext(DbContextOptions options)
: base(options)
{
}

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

modelBuilder.Entity<TestEntity30244>().OwnsMany(e => e.Settings, b => b.ToJson());
}
}

public override LibraryContext CreateContext(IEnumerable<ISingletonInterceptor> interceptors, bool inject)
=> new SqliteLibraryContext(Fixture.CreateOptions(interceptors, inject));

public class MaterializationInterceptionSqliteFixture : SingletonInterceptorsFixtureBase
{
protected override string StoreName
Expand All @@ -25,5 +43,8 @@ protected override IServiceCollection InjectInterceptors(
IServiceCollection serviceCollection,
IEnumerable<ISingletonInterceptor> injectedInterceptors)
=> base.InjectInterceptors(serviceCollection.AddEntityFrameworkSqlite(), injectedInterceptors);

public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
=> base.AddOptions(builder.ConfigureWarnings(e => e.Ignore(SqliteEventId.CompositeKeyWithValueGeneration)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maumar Had to suppress his warning, which I don't think should apply when the owned type is mapped to JSON. Are we tracking this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#26708, but maybe we need something json specific?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will people get this warning if they explicitly configure a composite key for a mapped JSON collection when that configuration is valid? /cc @AndriySvyryd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will people get this warning if they explicitly configure a composite key for a mapped JSON collection when that configuration is valid?

Yes, unless they mark the non-fk property as never generated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maumar Given Andriy's response, I think we need a new issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}