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.

Sign up for GitHub

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

Jump to bottom

[release/8.0] Cosmos: Generate ordinal values for key properties that used not to be persisted. #32489

Merged
merged 2 commits into from
Jan 3, 2024
Merged

[release/8.0] Cosmos: Generate ordinal values for key properties that used not to be persisted. #32489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a522e14
Cosmos: Generate ordinal values for key properties that used not to b…
AndriySvyryd Nov 30, 2023
1e70e42
Cosmos: Generate ordinal values for key properties that used not to b…
AndriySvyryd Dec 1, 2023
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
12 changes: 10 additions & 2 deletions src/EFCore.Cosmos/Extensions/CosmosPropertyExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ namespace Microsoft.EntityFrameworkCore;
/// </remarks>
public static class CosmosPropertyExtensions
{
private static readonly bool _useOldBehavior31664 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue31664", out var enabled31664) && enabled31664;

private static readonly bool _useOldBehavior32363 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32363", out var enabled32363) && enabled32363;

/// <summary>
/// Returns the property name that the property is mapped to when targeting Cosmos.
/// </summary>
Expand All @@ -34,8 +40,10 @@ private static string GetDefaultJsonPropertyName(IReadOnlyProperty property)
{
var pk = property.FindContainingPrimaryKey();
if (pk != null
&& (property.ClrType == typeof(int) || ownership.Properties.Contains(property))
&& property.IsShadowProperty()
&& ((property.ClrType == typeof(int)
&& (property.IsShadowProperty() || _useOldBehavior32363 || _useOldBehavior31664))
|| ownership.Properties.Contains(property))
&& (property.IsShadowProperty() || !_useOldBehavior32363 || _useOldBehavior31664)
&& pk.Properties.Count == ownership.Properties.Count + (ownership.IsUnique ? 0 : 1)
&& ownership.Properties.All(fkProperty => pk.Properties.Contains(fkProperty)))
{
Expand Down
5 changes: 4 additions & 1 deletion src/EFCore.Cosmos/Metadata/Conventions/CosmosValueGenerationConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public CosmosValueGenerationConvention(
{
}

private static readonly bool _useOldBehavior31664 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue31664", out var enabled31664) && enabled31664;

/// <summary>
/// Called after an annotation is changed on an entity type.
/// </summary>
Expand Down Expand Up @@ -80,7 +83,7 @@ public virtual void ProcessEntityTypeAnnotationChanged(
if (pk != null
&& !property.IsForeignKey()
&& pk.Properties.Count == ownership.Properties.Count + 1
&& property.IsShadowProperty()
&& (property.IsShadowProperty() || _useOldBehavior31664)
&& ownership.Properties.All(fkProperty => pk.Properties.Contains(fkProperty)))
{
return ValueGenerated.OnAddOrUpdate;
Expand Down
55 changes: 46 additions & 9 deletions ...edQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ namespace Microsoft.EntityFrameworkCore.Cosmos.Query.Internal;

public partial class CosmosShapedQueryCompilingExpressionVisitor
{
private static readonly bool _useOldBehavior32363 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32363", out var enabled32363) && enabled32363;

private abstract class CosmosProjectionBindingRemovingExpressionVisitorBase : ExpressionVisitor
{
private static readonly MethodInfo GetItemMethodInfo
Expand Down Expand Up @@ -596,25 +599,27 @@ private Expression CreateGetValueExpression(
return _projectionBindings[jObjectExpression];
}

var entityType = property.DeclaringType as IEntityType;
var ownership = entityType?.FindOwnership();
var storeName = property.GetJsonPropertyName();
if (storeName.Length == 0)
{
var entityType = property.DeclaringType as IEntityType;
if (entityType == null
|| !entityType.IsDocumentRoot())
{
var ownership = entityType?.FindOwnership();
if (ownership != null
&& !ownership.IsUnique
&& property.IsOrdinalKeyProperty())
&& !ownership.IsUnique)
{
var readExpression = _ordinalParameterBindings[jObjectExpression];
if (readExpression.Type != type)
if (property.IsOrdinalKeyProperty())
{
readExpression = Convert(readExpression, type);
}
var ordinalExpression = _ordinalParameterBindings[jObjectExpression];
if (ordinalExpression.Type != type)
{
ordinalExpression = Convert(ordinalExpression, type);
}

return readExpression;
return ordinalExpression;
}
}

var principalProperty = property.FindFirstPrincipal();
Expand Down Expand Up @@ -648,6 +653,38 @@ private Expression CreateGetValueExpression(
return Default(type);
}

// Workaround for old databases that didn't store the key property
if (!_useOldBehavior32363
&& ownership != null
&& !ownership.IsUnique
&& !entityType.IsDocumentRoot()
&& property.ClrType == typeof(int)
&& !property.IsForeignKey()
&& property.FindContainingPrimaryKey() is { Properties.Count: > 1 }
&& property.GetJsonPropertyName().Length != 0
&& !property.IsShadowProperty())
{
var readExpression = CreateGetValueExpression(
jObjectExpression, storeName, type.MakeNullable(), property.GetTypeMapping());

var nonNullReadExpression = readExpression;
if (nonNullReadExpression.Type != type)
{
nonNullReadExpression = Convert(nonNullReadExpression, type);
}

var ordinalExpression = _ordinalParameterBindings[jObjectExpression];
if (ordinalExpression.Type != type)
{
ordinalExpression = Convert(ordinalExpression, type);
}

return Condition(
Equal(readExpression, Constant(null, readExpression.Type)),
ordinalExpression,
nonNullReadExpression);
}

return Convert(
CreateGetValueExpression(jObjectExpression, storeName, type.MakeNullable(), property.GetTypeMapping()),
type);
Expand Down
115 changes: 102 additions & 13 deletions test/EFCore.Cosmos.FunctionalTests/EmbeddedDocumentsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ public virtual async Task Can_attach_owner_with_dependents()
public virtual async Task Can_manipulate_embedded_collections(bool useIds)
{
var options = Fixture.CreateOptions(seed: false);
var swappedOptions = Fixture.CreateOptions(modelBuilder => modelBuilder.Entity<Person>(
eb => eb.OwnsMany(
v => v.Addresses, b =>
{
b.OwnsMany(a => a.Notes).ToJsonProperty("IdNotes");
b.OwnsMany(a => a.IdNotes).ToJsonProperty("Notes");
})),
seed: false);

Address existingAddress1Person2;
Address existingAddress1Person3;
Expand Down Expand Up @@ -201,6 +209,10 @@ await context.AddAsync(
if (useIds)
{
addedAddress2.IdNotes = existingAddress1Person2.IdNotes;
foreach (var note in addedAddress2.IdNotes)
{
note.AddressId = 0;
}
}
else
{
Expand Down Expand Up @@ -373,6 +385,75 @@ async Task AssertState(EmbeddedTransportationContext context, bool useIds)
}
}

[ConditionalFact]
public virtual async Task Old_still_works()
{
var options = Fixture.CreateOptions(seed: false);
var swappedOptions = Fixture.CreateOptions(modelBuilder => modelBuilder.Entity<Person>(
eb => eb.OwnsMany(
v => v.Addresses, b =>
{
b.OwnsMany(a => a.Notes).ToJsonProperty("IdNotes");
b.OwnsMany(a => a.IdNotes).ToJsonProperty("Notes");
})),
seed: false);

using (var context = new EmbeddedTransportationContext(options))
{
await context.AddAsync(new Person
{
Id = 1,
Addresses = new List<Address>
{
new()
{
Street = "Second",
City = "Village",
Notes = new List<Note>
{
new() { Content = "First note" }
},
IdNotes = new List<NoteWithId>
{
new() { Id = 3, Content = "Second note" }
}
}
}
});

await context.SaveChangesAsync();
}

using (var context = new EmbeddedTransportationContext(options))
{
var people = await context.Set<Person>().ToListAsync();
var address = people.Single().Addresses.Single();

Assert.Equal("First note", address.Notes.Single().Content);

var idNote = address.IdNotes.Single();
Assert.Equal(3, idNote.Id);
Assert.Equal("Second note", idNote.Content);

var noteEntry = context.Entry(idNote);
var noteJson = noteEntry.Property<JObject>("__jObject").CurrentValue;

Assert.Equal(3, noteJson[nameof(NoteWithId.Id)]);
Assert.Null(noteJson[nameof(NoteWithId.AddressId)]);
}

using (var context = new EmbeddedTransportationContext(swappedOptions))
{
var people = await context.Set<Person>().ToListAsync();
var address = people.Single().Addresses.Single();

Assert.Equal("Second note", address.Notes.Single().Content);
Assert.Equal("First note", address.IdNotes.Single().Content);
}
}

public record struct CosmosPage<T>(List<T> Results, string ContinuationToken);

[ConditionalFact]
public virtual async Task Properties_on_owned_types_can_be_client_generated()
{
Expand Down Expand Up @@ -408,6 +489,7 @@ public virtual async Task Can_use_non_int_keys_for_embedded_entities()
v => v.Addresses, b =>
{
b.Property<Guid>("Id");
b.Ignore(a => a.IdNotes);
}));
},
seed: false);
Expand Down Expand Up @@ -600,19 +682,15 @@ protected override ITestStoreFactory TestStoreFactory
=> CosmosTestStoreFactory.Instance;

public virtual CosmosTestStore TestStore { get; }
private Action<ModelBuilder> OnModelCreatingAction { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
=> OnModelCreatingAction?.Invoke(modelBuilder);

public DbContextOptions CreateOptions(
public EmbeddedTransportationContextOptions CreateOptions(
Action<ModelBuilder> onModelCreating = null,
bool seed = true)
{
OnModelCreatingAction = onModelCreating;
var options = CreateOptions(TestStore);
var embeddedOptions = new EmbeddedTransportationContextOptions(options, onModelCreating);
TestStore.Initialize(
ServiceProvider, () => new EmbeddedTransportationContext(options), c =>
ServiceProvider, () => new EmbeddedTransportationContext(embeddedOptions), c =>
{
if (seed)
{
Expand All @@ -621,14 +699,19 @@ public DbContextOptions CreateOptions(
});

ListLoggerFactory.Clear();
return options;
return embeddedOptions;
}

protected override IServiceCollection AddServices(IServiceCollection serviceCollection)
=> base.AddServices(serviceCollection);
protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
=> ((EmbeddedTransportationContext)context).Options.OnModelCreating?.Invoke(modelBuilder);

protected override object GetAdditionalModelCacheKey(DbContext context)
=> OnModelCreatingAction?.GetHashCode();
{
var options = ((EmbeddedTransportationContext)context).Options;
return options.OnModelCreating == null
? null
: options;
}

public Task InitializeAsync()
=> Task.CompletedTask;
Expand All @@ -637,13 +720,18 @@ public Task DisposeAsync()
=> TestStore.DisposeAsync();
}

public record class EmbeddedTransportationContextOptions(DbContextOptions Options, Action<ModelBuilder> OnModelCreating);

protected class EmbeddedTransportationContext : TransportationContext
{
public EmbeddedTransportationContext(DbContextOptions options)
: base(options)
public EmbeddedTransportationContext(EmbeddedTransportationContextOptions options)
: base(options.Options)
{
Options = options;
}

public EmbeddedTransportationContextOptions Options { get; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Vehicle>(
Expand Down Expand Up @@ -734,6 +822,7 @@ public class Note
public class NoteWithId
{
public int Id { get; set; }
public int AddressId { get; set; }
public string Content { get; set; }
}
}
Loading