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

Question: why do you not allow IImmutableList<T> for collection navigational properties with backing fields? #21176

Open
Tracked by #240
JonPSmith opened this issue Jun 8, 2020 · 5 comments

Comments

@JonPSmith
Copy link

This is a question, rather than a request for a change!

I want to create entity classes where no properties can be changed outside the class (DDD-styled class). For instance:

public class Book
{
     private readonly List<Review> _reviews;
     public IEnumerable<Review> Reviews => _reviews.ToList();
     //... other properties left out
}

From my testing I know that collection navigational properties can use the following types -

  • IEnumerable<T>
  • IReadOnlyCollection<T>

I want to move away from using IEnumerable<T>, as it has (small) performance problems when you call it multiple times. I would like to use IImmutableList<T> as that sounds like a better type because it ensures that a) it takes a copy (AsReadOnly doesn't take a copy) and, b) makes parallel operations work better. But I get I use IImmutableList<T>.

   TestSaveBookOneReviewAndReadOk [0:00.749] Failed: System.InvalidCastException : Unable to cast object of type 'System.Collections.Generic.HashSet`1[DataLayer.EfClasses.Review]' to type 'System.Collections.Immutable.IImmutableList`1[DataLayer.EfClasses.Review]'.
System.InvalidCastException : Unable to cast object of type 'System.Collections.Generic.HashSet`1[DataLayer.EfClasses.Review]' to type 'System.Collections.Immutable.IImmutableList`1[DataLayer.EfClasses.Review]'.
   at lambda_method(Closure , InternalEntityEntry )
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.RelationshipsSnapshot..ctor(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.EnsureRelationshipSnapshot()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntrySubscriber.SnapshotAndSubscribe(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.StartTracking(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState oldState, EntityState newState, Boolean acceptChanges, Boolean modifyProperties)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState entityState, Boolean acceptChanges, Boolean modifyProperties, Nullable`1 forceStateWhenUnknownKey)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.PaintAction(EntityEntryGraphNode`1 node)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityEntryGraphIterator.TraverseGraph[TState](EntityEntryGraphNode`1 node, Func`2 handleNode)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.AttachGraph(InternalEntityEntry rootEntry, EntityState targetState, EntityState storeGeneratedWithKeySetTargetState, Boolean forceStateWhenUnknownKey)
   at Microsoft.EntityFrameworkCore.DbContext.SetEntityState(InternalEntityEntry entry, EntityState entityState)
   at Microsoft.EntityFrameworkCore.DbContext.SetEntityState[TEntity](TEntity entity, EntityState entityState)
   at Microsoft.EntityFrameworkCore.DbContext.Add[TEntity](TEntity entity)
   at Test.UnitTests.TestDataLayer.Ch08_RelationshipBackingFields.TestSaveBookOneReviewAndReadOk() in C:\Users\JonPSmith\source\repos\EfCoreinAction-SecondEdition\Test\UnitTests\TestDataLayer\Ch08_RelationshipBackingFields.cs:line 151

IImmutableList<T> seems to tick all the boxes by supporting IEnumerable<T>, ICollection<T>, IList<T>. Also AsReadOnly() doesn't work with a HashSet, but ToImmutableList() does. so why does EF Core not support it? Is there something fundamental or just the way it is written?

NOTE: I have tried this on EF Core 3.1 and 5-preview4

@ajcvickers
Copy link
Contributor

@JonPSmith I have been playing with this and I think we could make it work in a future version. It's more tricky than the other collection types because the backing field is not directly convertable to the property, and vice versa.

@JonPSmith
Copy link
Author

Hi @ajcvickers,

Thanks for looking at this. Totally happy to got with IReadOnlyCollection<T>, but just wondered why its that way.

For the DDD part of my book I'm going with

public class Book
{
     private readonly IList<Review> _reviews; //Or HashSet if performance matters
     public IReadOnlyCollection<Review> Reviews => _reviews.ToImmutableList();
     //... other properties left out
}

I found that ToImmutableList() works for HashSet<T>, IList<T> etc. while AsReadOnly() doesn't work on either of those. I haven't checked said-by-side performance of ToImmutableList() and AsReadOnly() but I can't think its much (Doh! famous last works - I better check it)

I have left this open, but my question has been answered. Happy for you to close it if you want to.

@ajcvickers
Copy link
Contributor

@JonPSmith It's worth keeping open--I suspect we can make this work.

@JonPSmith
Copy link
Author

Timings of each, repeated to overcome first-use cost

1,000 x AsReadOnly took .14 ms., ave. per run = 135.6 ns.
1,000 x ToImmutableList took 2.60 ms., ave. per run = 2,596.2 ns.
1,000 x AsReadOnly took .00 ms., ave. per run = .9 ns.
1,000 x ToImmutableList took .08 ms., ave. per run = 81.0 ns.

So ToImmutableList is ~100 times slower but its so small I'm not sure it matters. I'll ponder on that.

@maumar
Copy link
Contributor

maumar commented Oct 12, 2023

also consider JSON when/if this is implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants