BlueshiftSoftware / EntityFrameworkCore

NoSQL providers for EntityFramework Core
Other
281 stars 57 forks source link

NullReferenceException with OO models #9

Closed yaseralnajjar closed 6 years ago

yaseralnajjar commented 7 years ago

Hi,

While I was trying to see if mongo can fit my future projects, I landed to this provider... great work guys!

This issue is with version:

2.0.0-preview-t004f07459 .net core 2.0 project template made with vs 2017

Well, the ToListAsync causes an exception when there is a nested object, as in here:

public class Car
{
    public Car()
    {
        Owner = new Owner{Id = ObjectId.GenerateNewId()};
    }
    [BsonId]
    public ObjectId Id { get; set; }
    public string Model { get; set; }
    public int Price { get; set; }
    public Owner Owner { get; set; }
}

public class Owner
{
    public Owner()
    {
        Name = "";
    }
    [BsonId]
    public ObjectId Id { get; set; }
    public string Name { get; set; }
}

But listing works fine when I have such a model (no nested documents):

public class Car
{
    [BsonId]
    public ObjectId Id { get; set; }
    public string Model { get; set; }
    public int Price { get; set; }
}

Controller action is ordinary:

// GET: Cars
public async Task<IActionResult> Index()
{
    var cars = await _context.Car.ToListAsync();

    return View(cars);
}
crhairr commented 7 years ago

I'm surprised that there aren't any async tests for the MongoDb provider itself. However, ToListAsync() appears throughout the Identity provider, whose default user model uses nested documents for claims and tokens. I'll take a look at this and at the very least add some tests for the async provider usage.

Does the exception occur on the call to ToListAsync() itself, or on the call to View()? If you're trying to display Owner using Razor, but Owner is null coming back from the database, you'll get an error from Razor's rendering engine.

yaseralnajjar commented 7 years ago

CoreTwoDemo.zip

Nope, it comes from the call itself... right after stepping over that line (ToListAsync).

I attached the project, you can download it and run it

crhairr commented 7 years ago

After reviewing the problem, I think I see what's going on. There's a relationship between Car and Owner which I misinterpreted as an "owned" relationship (a subdocument versus a navigation).

The first thing to note here is that MongoDb is not a relational database. After all, it's called "NoSQL" for a reason.

That said, recent versions of MongoDB have added support for the $lookup operator as a form of server-side join, but it does not behave the way you might think if you think of it as a replacement for the "LEFT JOIN" operator in SQL. There are some quirks to $lookup that I haven't accounted for yet, and as such EFCore-MongoDB currently does not fully support this kind of navigation property. I thought that there were checks in the model validation to ensure that no such navigations exist, but it appears they've been disabled or removed.

There's a good guide to MongoDB schema design that explains what kinds of schemas MongoDB is designed to handle, and how to avoid schemas it isn't. In your case, I'd recommend nesting a CarOwner subdocument that has information about the car's owner that's relevant to Car itself, or denormalize the Owner.Id reference as Car.OwnerId and perform a Join to fetch the relationship.

I'll work on clearing up the confusion within the provider so that it's more obvious that navigations aren't fully supported right now. Since I'm the only active developer on this project, it might take me a while before I can work out the kinks in the navigation to get them working correctly. I apologize for the inconvenience.

For what it's worth, I committed some changes last night that should obviate the need to use ComplexTypeAttribute on subdocuments: all types are now automatically "mapped". There should be a new build available on MyGet.

Also for what it's worth, I highly recommend you use a "view model" that separates your DB entities from the information displayed in your web views. This mapping layer allows you to introduce business logic validation that is completely separate from DB schema validation, which will make your application considerably more flexible. It also prevents the accidental leaking of potentially sensitive information from your database into a user's browser, assuming there are fields that contain business data that a user should never see directly.

yaseralnajjar commented 7 years ago

use a "view model"

Actually, the design & architecture of this project is just cumbersome, no separate BLL and DAL projects since it's just a demo project after all.

As you said, the right way of designing that scenario is to store the owner id: https://docs.mongodb.com/manual/tutorial/model-referenced-one-to-many-relationships-between-documents/

a subdocument versus a navigation

Well, at least I was actually trying the subdocument feature in mongo... but it also opened the navigational properties subject 😄

But, this does not change the fact that the subdocument feature causes an exception when loading the document.

As you know in EntityFrameworkCore.SqlServer, this is how we load the related properties:

var cars = await _context.Car
                         .Include(c => c.Owner)
                         .ToListAsync();

This appears to cause another exception:

NullReferenceException: Object reference not set to an instance of an object.
Microsoft.EntityFrameworkCore.Query.Internal.QueryBuffer.GetPropertyValue(object entity, IProperty property)

This is the exception when not including the Owner:

NullReferenceException: Object reference not set to an instance of an object.
lambda_method(Closure , ValueBuffer )

I hope there will support for the navigational properties since it will give us the advantage of using the OO model designing. Psst, please don't forget to add interface navigational properties support, without using a base class, cuz that would be a huge advantage for the EF-Mongo over the EF-SQL.

crhairr commented 7 years ago

Please do not use Include() as it is not necessary with MongoDB. MongoDB is built around arbitrary hierarchical document structures, and embedded documents are returned by the server automatically.

Include() is one of several features in the core EFCore library (such as "Owns") that should have been relegated to EFCore.Relational as they are irrelevant for document databases. Just because a feature is available doesn't mean that it's appropriate to use with EFCore-MongoDB. I'll add this to my notes and update the documentation accordingly. I'll also add overrides to the model builder and the query expression visitor to throw exceptions accordingly when an irrelevant feature is referenced.

There are several units tests built around subdocuments and subdocument querying: that feature works as expected as far as I'm aware. That said, I will expand the test model to reflect the use of interfaces, abstract classes, and shared data types.

yaseralnajjar commented 7 years ago

I've tried to update to the new build using the nuget powershell... the package doesn't exist (t0050c6393):

GET https://api.nuget.org/v3-flatcontainer/blueshift.entityframeworkcore.mongodb/index.json
NotFound https://api.nuget.org/v3-flatcontainer/blueshift.entityframeworkcore.mongodb/index.json

EDIT After adding the source to npm the package is restored fine after a couple of requests by the npm with couple of not found responses:

  GET https://api.nuget.org/v3-flatcontainer/blueshift.entityframeworkcore.mongodb/index.json
  GET https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/blueshift.entityframeworkcore.mongodb/index.json
  OK https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/blueshift.entityframeworkcore.mongodb/index.json 345ms
  GET https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/blueshift.entityframeworkcore.mongodb/2.0.0-preview-t0050c6393/blueshift.entityframeworkcore.mongodb.2.0.0-preview-t0050c6393.nupkg
  NotFound https://api.nuget.org/v3-flatcontainer/blueshift.entityframeworkcore.mongodb/index.json 855ms
  OK https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/blueshift.entityframeworkcore.mongodb/2.0.0-preview-t0050c6393/blueshift.entityframeworkcore.mongodb.2.0.0-preview-t0050c6393.nupkg 935ms
  GET https://api.nuget.org/v3-flatcontainer/mongodb.driver/index.json
  GET https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.driver/index.json
  NotFound https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.driver/index.json 87ms
  OK https://api.nuget.org/v3-flatcontainer/mongodb.driver/index.json 529ms
  GET https://api.nuget.org/v3-flatcontainer/mongodb.driver/2.4.4/mongodb.driver.2.4.4.nupkg
  OK https://api.nuget.org/v3-flatcontainer/mongodb.driver/2.4.4/mongodb.driver.2.4.4.nupkg 47ms
  GET https://api.nuget.org/v3-flatcontainer/mongodb.bson/index.json
  GET https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.bson/index.json
  GET https://api.nuget.org/v3-flatcontainer/mongodb.driver.core/index.json
  GET https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.driver.core/index.json
  NotFound https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.bson/index.json 74ms
  OK https://api.nuget.org/v3-flatcontainer/mongodb.bson/index.json 170ms
  GET https://api.nuget.org/v3-flatcontainer/mongodb.bson/2.4.4/mongodb.bson.2.4.4.nupkg
  OK https://api.nuget.org/v3-flatcontainer/mongodb.bson/2.4.4/mongodb.bson.2.4.4.nupkg 51ms
  NotFound https://www.myget.org/F/efcore-mongodb/api/v3/flatcontainer/mongodb.driver.core/index.json 323ms
  OK https://api.nuget.org/v3-flatcontainer/mongodb.driver.core/index.json 690ms
  GET https://api.nuget.org/v3-flatcontainer/mongodb.driver.core/2.4.4/mongodb.driver.core.2.4.4.nupkg
  OK https://api.nuget.org/v3-flatcontainer/mongodb.driver.core/2.4.4/mongodb.driver.core.2.4.4.nupkg 50ms
Installing MongoDB.Driver.Core 2.4.4.
Installing MongoDB.Bson 2.4.4.
Installing MongoDB.Driver 2.4.4.
Installing Blueshift.EntityFrameworkCore.MongoDB 2.0.0-preview-t0050c6393.
Installing NuGet package Blueshift.EntityFrameworkCore.MongoDB 2.0.0-preview-t0050c6393.
yaseralnajjar commented 7 years ago

After updating to the latest package(t0050c6393), retrieving the subdocument (owner) works fine 👍

crhairr commented 7 years ago

Woohoo! That's what I like to see.

If you don't mind, I'd like to keep this issue open as I work through the Navigations implementation. That way, I can send you an update if/when I get it working.

crhairr commented 7 years ago

I apologize for how long it took, but after a lot of research and going back and forth working around some issues in EF Core, I finally got navigations working.

There are a few caveats:

It at least works for now. I'm going to continue working on navigation properties until it correctly uses the $lookup operator for a server-side join, but for now you can start uses models that make proper use of relationships.

yaseralnajjar commented 7 years ago

I will update the demo project as soon as possible to see if the navigation works between the owner and the car. (last thing I tried to do is making the owner inherit from an identity user and I didn't finish it properly).

==============

In case you don't already know:

(In the mssql provider, I was working on another project, and even if X points to a collection of Y and Y doesn't point to X... it will automatically add a column in Y pointing to X).

Here are the models:

public class University : IEntity<int>
    {
        public University()
        {
            Documents = new List<Document>();
        }
        public string Name { get; set; }
        public string Country { get; set; }
        public string City { get; set; }
        public string Description { get; set; }
        public int Commission { get; set; }

        public ICollection<Document> Documents { get; set; }
    }

public class Document : IEntity<int>
    {
        public int Id { get; set; }
        public DocumentType DocumentType { get; set; }
        public string DocumentFilePath { get; set; }
    }

This will result in adding a column to the Documents table that reserves the UniversityId automatically .

crhairr commented 7 years ago

Yeah, EF Core uses shadow properties to map relationships, and the columns are derived from those. It was one of the things I had to work around to get the navigations to work for a document DB.

Also, I didn't see this last night before I posted, but the build appears to be broken. I'll have to fix it before you can pull an update where relationships will actually work. KoreBuild changed the way they do things and I have to figure it out. Sorry for the second delay!

crhairr commented 7 years ago

The build is fixed and a package has been published. The new version is 2.1.0-preview1 (keeping with EF-Core's current version). At some point, I'm going to expand the unit tests and prepare a final (non-preview) release.

yaseralnajjar commented 7 years ago

Sorry for the late response... gonna update my repo asap :)

yaseralnajjar commented 7 years ago

Hi,

I couldn't get the latest update from myget... I tried by downloading the update from nuget UI, console, and csproj file.

Install-Package : Unable to find package Microsoft.Extensions.Configuration with version (>= 2.1.0-preview1-27512)
  - Found 12 version(s) in nuget.org [ Nearest version: 2.0.0 ]
  - Found 5 version(s) in Microsoft Visual Studio Offline Packages [ Nearest version: 1.1.2 ]
  - Found 0 version(s) in EFCore-MongoDb
crhairr commented 7 years ago

Since EFCore-MongoDB is still in preview, it's being built against the current EFCore preview build. To get those - and all current .NET Core previews - you'll need to add this to your NuGet.config file:

<add key="AspNetCore" value="https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json" />

This will go away when I publish the initial release (which will likely be as 2.1.0 to keep with the current EFCore release).

yaseralnajjar commented 7 years ago

Sorry for being late... I just wasted lots of time trying to get the packages and figured out I need to install the .net core 2.02 sdk !

I updated my demo repo by commenting out the joining code and adding the relationship between the Car and the Owner.

Here is what I found:

The navigation works fine in the normal case 😉 But when I specify the Owner of the car as Employee object the adding will fail.

 public static class CarModelExtensions
    {
        public static Car ToModel(this CarViewModel carViewModel)
        {
            var result = new Car
            {
                Model = carViewModel.Model,
                Price = carViewModel.Price,

                Owner = new Owner
                {
                    Name = carViewModel.OwnerName
                }
            };

            return result;
        }
}

If you replace this line:

Owner = new Owner

with this:

Owner = new Employee

Even thought I explicitly made adding the entity goes into the owner collection:

public class CarRepository : DataService<Car, string>
    {
                public override async Task<Car> Add(Car car)
        {
            var owner = car.Owner;
            var ownerRepo = new OwnerRepository(Context);
            await ownerRepo.Add(owner);
            //car.OwnerId = owner.Id;

            return await base.Add(car);
        }
      }

    public class OwnerRepository : DataService<Owner, ObjectId>
    {
        public OwnerRepository(EFMongoDemoDbContext context) : base(context)
        {
        }

        public override async Task<Owner> Add(Owner entity)
        {
            Context.Owners.Add(entity);
            await CommitChanges();

            return entity;
        }
    }
crhairr commented 7 years ago

I'm a bit confused as to what you're trying to accomplish there. But I'd like to point out a few things:

1) If you're using the EFCore-MongoDB Identity manager, be aware that it stores identities in a different MongoDb database instance called "__identities". Relationships will not work between your EFMongDemoDbContext and the IdentityMongoDbContext. You can also create a navigation to a different DbSet<> in EFMongoDemoDbContext that mirrors the identities in the identity context, with the understanding that changes to one will not affect the other without manual intervention.

2) In order for a navigation to work, you need to have a top-level Entity defined in your EFMongoDemoDbContext model. This is usually accomplished just by adding a DbSet<> to the context. Without a top-level Entity (complete with a PrimaryKey/BsonId property), the property will just be treated as a complex type and serialized into the current entity's document.

3) Due to the lack of proper support for Complex Types from EFCore, EFCore-MongoDB only supports navigations from at the document root. That is, EFCore's metadata system will not detect navigations contained within subdocument (complex type) properties. Consider the following:

public class EntityA
{
    [Key]
    public ObjectId Id {get;}
}

public class ComplexType1
{
    // This class does not have a Key, so it's treated as a complex type and
    // this "navigation" will not be detected.
    public EntityA EntityAItem {get;}
}

public class EntityB
{
    [Key]
    public ObjectId Id {get;}

    // WILL NOT detect the navigation from ComplexType1 to EntityA.
    // All values in the list, and the EntityAItem within, will be serialized
    // as subdocuments of EntityB.
    public IList<ComplexType1> List {get;}

    // This will be treated as a proper navigation. Only the EntityA.Id will be serialized,
    // and a $lookup (.Include(entityB => entityB.EntityAItem)) will be required to retrieve
    // the instance of EntityA.
    public EntityA EntityAItem {get;}
}

Please note: without support for complex types in the EFCore metadata system, this CAN NOT be easily worked around. Feel free to help me convince the EFCore team to get proper complex type support implemented. They have implemented something called "Owned" types, but these do not behave as true complex types and do not support deep nesting (only one level of hierarchy below the parent). You can find the complex type issue in their repo's issue list here.

yaseralnajjar commented 7 years ago

I just realized the business logic is totally broken in the demo!

What I'm trying to do is a simple rent-a-car app where: 1- The owner of the car is an identity user who can add cars (after logging in). 2- Employee is someone who can add limited number of cars. He is an Owner with some limitation. 3- Role for specific owner something like AdminRole to manage all cars. 4- Owners can edit only their own cars.

If you're using the EFCore-MongoDB Identity manager, be aware that it stores identities in a different MongoDb database instance called "__identities".

I'm using the normal userManager to add the users and I got a Users collection that can be referred in the same mongodb instance, is there something wrong with this approach?

I'm gonna work on those and add proper relationships... even I realized I didn't have a collection of cars in the owner model 😄

crhairr commented 6 years ago

Async queries, complex type subdocuments, and relationship navigations are all supported.

yaseralnajjar commented 6 years ago

Great job !

After half a year of intense dev... I realized that the main problem was the model design, since mongodb is meant for de-normalizing.

Quoting from a good read:

  1. Unless there is a compelling reason to do so, favor embedding.
  2. Needing to access an object on its own is a compelling reason to not embed the object in another >document.
  3. Unbounded array growth is a bad design.
  4. Don’t be afraid of joins on the application side. A properly indexed collection and query can have highly performant results.
  5. When denormalizing your data, consider the read to write ratio of the application.
  6. Finally, how you model your data depends on your application’s data access patterns. Match your schema design to how your application reads and writes the data.

Src: https://dev.to/kenwalger/schema-design-considerations-in-mongodb-47f

Also, for the inheritance part employee inherits from the owner this was a wrong design as well, instead, I should've used composition one-to-one relationship, (an employee or an owner) to (a user).

Isn't it the time fire this awesome package into nuget :wink: