eventflow / EventFlow

Async/await first CQRS+ES and DDD framework for .NET
https://docs.geteventflow.net/
Other
2.36k stars 444 forks source link

Entity Framework read model can not load children entities during update #669

Closed twzhangyang closed 1 year ago

twzhangyang commented 5 years ago

Hi @frankebersoll , @rasmus

I am writing a fully example RestAirline which based on eventflow, I realizing there is a bug in EntityFrameworkReadModelStore which can not load whole read model including children entities during model updating. Considering below reade model definition :

 public class BookingReadModel : IReadModel,
    IAmReadModelFor<Domain.Booking.Booking, BookingId, JourneysSelectedEvent>,
    IAmReadModelFor<Domain.Booking.Booking, BookingId, PassengerAddedEvent>,
    IAmReadModelFor<Domain.Booking.Booking, BookingId, PassengerNameUpdatedEvent>
{
    public BookingReadModel()
    {
        Passengers = new List<Passenger>();
        Journeys = new List<Journey>();
    }

    [Key] public string Id { get; protected set; }

    [ConcurrencyCheck] public long Version { get; set; }

    public List<Passenger> Passengers { get; set; }

    public List<Journey> Journeys { get; set; }
}

Unlike flat definition this read model including two nested entities, during model updating this updater didn't work because Passenger list was not load into memory:

public void Apply(IReadModelContext context,
    IDomainEvent<Domain.Booking.Booking, BookingId, PassengerNameUpdatedEvent> domainEvent)
{
// this line doesn't work
    var passenger = Passengers.Single(x => x.PassengerKey == domainEvent.AggregateEvent.PassengerKey);

    passenger.Name = domainEvent.AggregateEvent.Name;
}

I guess it caused by line 233-288 in EntityFrameworkReadModelStore and approach will not load whole read model data including nested entities.

public Task<TReadModel> Query(DbContext context, string id, CancellationToken t, bool tracking = false)
{
    return tracking
        ? _queryByIdTracking(context, t, id)
        : _queryByIdNoTracking(context, t, id);
}

According to EF doc, there are three ways we can get nested entities. Lazy load seems is the best approach which satisfy my requirements, but unfortunately an exception was throw during readmodel update. See Lazy load spike.

Do you have any suggestion?

KoriSeng commented 5 years ago

I could be wrong, but i think read model was designed to be flat and not load related data this way.

I think you could add PassengerId into the PassengerNameUpdatedEvent and use that in the PassengerLocator to update the Passenger.

twzhangyang commented 5 years ago

Hi @7thcubic

Thanks your suggestion, I just took a look IReadModelLocator and I believe I can create a PassengerReadModel and it probably will work.

But I'm thinking that's not the way of Entity Framework works. As a read model I want to do kinds of query that entity framework support, eg:

using (var context = _contextProvider.CreateContext())
{
    var readModel = await context.Bookings
        .Include(x => x.Journeys)
        .ThenInclude(x=>x.Flight)
        .Include(x => x.Passengers)
        .SingleAsync(x => x.Id == query.BookingId, cancellationToken);

    return readModel;
}

That's the benefit of entity framework can bring as a read model, What would you think?

rasmus commented 5 years ago

@twzhangyang That's a very complex read model with a potential for a lot of data being transferred from the database. I would consider creating a read model that specific for the data you need and keeping it in a single table.

KoriSeng commented 5 years ago

I agree with ramus. Without proper optimisation of queries it might slow down the performance. I could see it as a optional feature. But again. This would then split the method of implementation into two.

Right now the models are flat and simple to the point that you could use the same read model and throw them into different stores without changing much if not nothing.

I don't know if having this out weighs the benefit of having the option of changing the read store to another medium at near 0 cost

twzhangyang commented 5 years ago

@7thcubic @rasmus I admit this read model is complex and will affect performance. But I have an API that can get whole booking, I don't know how to design such flat model with entity framework. Maybe NoSQL and Elasticsearch will more suitable for my purpose. I will try to integrate these two implementation and leave entity framework.

Unless each read model is designed for specific Query/UI/Api? in that case Api like get whole booking is not reasonable? Even thinking problems like this I believe it not easy to ensue all the read model can be designed to flat?

What would you think?

KoriSeng commented 5 years ago

If there is a need for complex query types you can always implement your own query and query processor Guide

It might go something like this

using Microsoft.EntityFrameworkCore;
using System.Collections.Generic;

namespace EFModeling.Configuring.FluentAPI.Samples.Relationships.NoForeignKey
{
    #region Model
    class MyContext : DbContext
    {
        public DbSet<Blog> Blogs { get; set; }
        public DbSet<Post> Posts { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Post>()
                .HasOne(p => p.Blog)
                .WithMany(b => b.Posts);
        }
    }

    public class Blog
    {
        public int BlogId { get; set; }
        public string Url { get; set; }

        public List<Post> Posts { get; set; }
    }

    public class Post
    {
        public int PostId { get; set; }
        public string Title { get; set; }
        public string Content { get; set; }

        public Blog Blog { get; set; }
    }
    #endregion
}
public class CustomQueryHandler : IQueryHandler<CustomQuery, Blog>
{
    private readonly   IDbContextProvider<MyContext> _provider;
    public CustomQueryHandler (IDbContextProvider<MyContext> provider)
   {
        _provider = provider;
   }

    public async Task<Blog> ExecuteQueryAsync(CustomQueryquery, CancellationToken cancellationToken)
     {
          return await _provider.CreateContext().Blog(x => x.Id.Equals(query.Id)).Include(x => x.Blogs).FirstOrDefaultAsync();
     }
}

Again this is NOT tested, you could try and see if it helps and solve your issue of lazy loading

jepperc commented 5 years ago

I just ran into this problem my self and am not finding the provided answers to solve my problem. E.g. having the class as a separate readmodel requires more properties on the events that I don't want.

Therefore I am interested in whether @rasmus would be interested in a PR where I open up for adding includes in the configuration of EF read models? Current idea would be a UseEntityFrameworkReadModel overload that takes a list of includes, and then using those inside the EntityFrameworkReadModelStore, but I haven't tested if it is doable that way.

rasmus commented 5 years ago

@jepperc if you have an idea that might improve upon EventFlow I'm all for it, especially if it's improving the developer API.

frankebersoll commented 4 years ago

@jepperc I think we also ran into this issue some time ago, and a colleague had the idea to make the read models configurable. Sounds pretty much like what you're suggesting. We moved to MongoDB for this app and obviously don't have the problem any more.

Do you want to contribute this feature? If you need any assistance in doing so, please let us know!

github-actions[bot] commented 1 year ago

Hello there!

We hope you are doing well. We noticed that this issue has not seen any activity in the past 90 days. We consider this issue to be stale and will be closing it within the next seven days.

If you still require assistance with this issue, please feel free to reopen it or create a new issue.

Thank you for your understanding and cooperation.

Best regards, EventFlow

github-actions[bot] commented 1 year ago

Hello there!

This issue has been closed due to inactivity for seven days. If you believe this issue still needs attention, please feel free to open a new issue or comment on this one to request its reopening.

Thank you for your contribution to this repository.

Best regards, EventFlow