1

Assume that we have a mapped entity named "Product" as:

public class Product
{
    public int Id { get; set; }
    public string Title { get; set; }
    public decimal Price { get; set; }
}

And also a not mapped entity named "Cart" as:

[NotMapped]
public class Cart
{
    public int Id { get; set; }
    public int ProductId { get; set; }
    public int Quantity { get; set; }
    public Product Product { get; set; }
}

What is the best way to populate "Product" into "Cart"? If both entities were mapped then it could be performed like this:

dbContext.Cart.Include(c => c.Product);

Is it possible to use similar syntax or I should query "Product" table?

Edit: This is what I finally did:

var selectedProductIds = cartList.Select(c => c.ProductId).ToList(); 
var selectedProducts = db.Products.Where(p => selectedProductIds.Contains(p.Id)).ToList(); 
var cart = cartList.GroupJoin(selectedProducts, c => c.ProductId, p => p.Id, (c, p)=> new Cart() { Product = p.First(), Quantity=c.Quantity}).ToList();
Abe
  • 39
  • 7
  • Where is `Cart` data coming from? – Ivan Stoev Apr 30 '19 at 18:20
  • They are stored in Session – Abe Apr 30 '19 at 18:25
  • You'll have to query Product table for that, since EF doesn't know about Cart, it doesn't know about their relationship. However, is there any Reason why Cart is not mapped? its properties look like it could represent a database table. – DevilSuichiro Apr 30 '19 at 18:51
  • Thank you @DevilSuichiro but mapping Cart is not appropriate in this case since it reduces performance and even may cause security issues. It's not reasonable to access query db each time a user adds a product to its cart. – Abe May 01 '19 at 06:38
  • Hi Abe, if your last edit answered your question, please remove it from the question and make it an own answer which you can accept. – bummi May 01 '19 at 06:57
  • Hi bummi, I think my question was somehow ambiguous and the answer @steve-py provides somehow answers my question. Besides, it provides some useful information about security issues. So, I decided to mark his answer as the correct one and mention my own solution as an edit. – Abe May 01 '19 at 07:43

1 Answers1

1

An entity reflects the table structure. So yes, if a cart isn't in the data structure and has no entity, then you load products directly.

It looks like you are working on an example like a shopping cart where-by your cart reflects the selected products to purchase. In this sense a cart only needs to exist as a view model. The first consideration is to differentiate between view model and model. (Entity) These should be entirely separate and not intermixed. Entities should only exist within the boundaries of the DbContext that loaded them. They can be detached at treated as POCO C# objects, but this can easily lead to bugs, performance issues, and vulnerabilities.

Can a cart only consist of one product? or a list of different products and quantities?

On the assumption that a cart is a single product (and quantity)

public class CartViewModel
{
    public ProductViewModel { get; set; }
    public int Quantity { get; set; }
}
public class ProductViewModel
{
    public int ProductId { get; set; }
    public string Name { get; set; }
    public decimal Price { get; set; }
}

When the view goes to get a list of products to display, the controller returns back ProductViewModel instances:

return context.Products
    .Where(x => x.IsActive)
    .Select(x => new ProductViewModel
    {
        ProductId = x.ProductId,
        Name = x.Name,
        Price = x.Price
    }).ToList();

"Why not just return the entity?" As the system develops the product table will likely grow to include more columns, and more relationships that need to be considered. For example, associating stock levels. The view does not need to know about anything more than it needs to know about and a serializer will attempt to serialize everything it is fed. This means sending more data to the client than is needed, and can lead to problems where you may have circular references. By utilizing .Select() you optimize a query to just return the fields needed and only expose that data to the client. The ProductViewModel should contain all information you need to display on the client, and the details you will need to complete any action against that product. With Automapper you can set up mapping rules for Entity to ViewModel and utilize ProjectTo<T>() to replace the Select() without the need to map the properties manually each time.

With the list of available products your client-side code can select one of these view models to associate with the Cart and record a quantity. You can calculate a total on-screen using the ProductViewModel's Price value.

When you go to complete an order, you could pass the selected product ViewModel back to the server along with the quantity, but it would be better to just pass the Product's ID back with the quantity. Assuming the server call is going to create an Order for that product:

You might be tempted to do something like this with a view model:

public ActionResult CreateOrder(ProductViewModel product, int quantity)
{
    if (quantity <= 0 || quantity > MaximumOrderSize)
        throw new ArgumentException("quantity", "Naughty User!");

    using (var context = new MyDbContext())
    {
        var order = new Order
        {
            ProductId = product.ProductId,
            Cost = product.Price * quantity,
            // ... obviously other details, like the current user from session state...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

The problem with this is that we are too trusting the data coming back from the client. Someone using debugging tools can intercept the call to our server from their client and set the product.Price to $0.00 or give themselves a 50% discount. Our order would base the cost on the price sent from the client.

Instead:

public ActionResult CreateOrder(int productId, int quantity)
{
    if (quantity <= 0 || quantity > MaximumOrderSize)
        throw new ArgumentException("quantity", "Naughty User!");

    using (var context = new MyDbContext())
    {
        var product = context.Products.Single(x => x.ProductId == productId); // Ensures we have a valid product.
        var order = new Order
        {
            Product = product, // set references rather than FKs.
            Cost = product.Price * quantity,
            // ...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

In this case we have verified that the Product actually exists, and we only use the Price from our trusted data, not what was passed by the client. If for instance the client tampered with the Product ID to an invalid value, our application exception handling (or you can add exception handling per action) will record the fault and can terminate the session if it suspects tampering. The tampering cannot change the cost of the order because that data comes from our server every time.

To outline why you do not want to send Entities around, especially back to the server from the client, let's look at the view model example, except pass back a Product entity:

public ActionResult CreateOrder(Product product, int quantity)
{
    if (quantity <= 0 || quantity > MaximumOrderSize)
        throw new ArgumentException("quantity", "Naughty User!");

    using (var context = new MyDbContext())
    {
        var order = new Order
        {
            Product = product,
            Cost = product.Price * quantity,
            // ...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

This may look "Ok", but there is a big problem. "context" in this case does not know about that instance of Product. It was deserialized from the request and for all intensive purposes looks like a new Product instance you new'ed up. This would result in EF either creating a duplicate Product entry with a new ID (and any other tampered data), or throwing an exception about a duplicate primary key.

Now that is duplicate or error is fixable, we just need to attach the entity...

public ActionResult CreateOrder(Product product, int quantity)
{
    if (quantity <= 0 || quantity > MaximumOrderSize)
        throw new ArgumentException("quantity", "Naughty User!");

    using (var context = new MyDbContext())
    {
        context.Products.Attach(product);
        var order = new Order
        {
            Product = product,
            Cost = product.Price * quantity,
            // ...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

And it should work. In some cases the context may already know about the entity and throw an error if it already exists. (such as cases where we would be looping through data which may have duplicate references) Except we are still trusting the data coming from the client. The user can still have modified the price which will be reflected in the order. This is also potentially very dangerous because if we later go to modify something on the product, or simply mark it as Modified, EF will save any alterations that the hacking client has made to the product as well. For instance:

public ActionResult CreateOrder(Product product, int quantity)
{
    if (quantity <= 0 || quantity > MaximumOrderSize)
        throw new ArgumentException("quantity", "Naughty User!");

    using (var context = new MyDbContext())
    {
        context.Products.Attach(product);
        product.AvailableQuantity -= quantity;
        var order = new Order
        {
            Product = product,
            Cost = product.Price * quantity,
            // ...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

Assuming our Product had an available quantity property that we want to update when placing an order, this call makes that adjustment. The entity is now marked as modified. What we didn't notice is that where our product's price is normally $100, and $100 was sent to the client, that hacking user saw that we were passing back the whole product and was curious what would happen if he changed the price to $50 in the data sent back to the server. Not only would his order be at $50 per product, but he's now changed our product price from $100 to $50 because the modified entity was associated to the context, marked as modified, and the changes were saved. Where we might have had admin functions for altering products, tracking user IDs, modified dates, etc. none of that necessarily may have been updated since we trusted an entity coming back from a client. Even if you record the fact that this user corrupted the data, the potential for disruption to your system becomes huge.

You can choose to save time passing entities and just not trust them on the way back and always reload the entity. But the risk as your system matures is that someone will get sloppy or lazy, figure the entity is there anyways and Attach / Update against the context. You can find several examples of questions in StackOverflow where people have raised questions about errors or issues while doing exactly that.

Edit: For a many product+quantity per cart: You will want to represent the selected products in a collection structure within the cart client side, then pass that collection when doing something like placing an order.

So the ProductViewModel can stay the same, but then we introduce a simple OrderedProductViewModel to represent the ordered products when calling Order:

public class ProductViewModel
{
    public int ProductId { get; set; }
    public string Name { get; set; }
    public decimal Price { get; set; }
}
public class OrderedProductViewModel
{
    public int ProductId { get; set; }
    public int Quantity { get; set; }
}

The concept of a "cart" is strictly client-side, but if we do have a cart model that exists server-side: (just not an entity)

public class CartViewModel
{
    public ICollection<OrderedProductViewModel> { get; set; } = new List<OrderedProductViewModel>();
}

So the product list still returns a collection of ProductViewModels to the view. When the user goes to add a product to the cart (client-side) you store an array of objects consisting of the Product ID and Quantity.

CreateOrder becomes something like:

public ActionResult CreateOrder(ICollection<OrderedProductViewModel> products)
{
    if(products == null || !products.Any())
        throw new ArgumentException("Can't create an order without any selected products.");

    using (var context = new MyDbContext())
    {
        var order = new Order
        {
            OrderLines = products.Select(x => createOrderLine(context, x)).ToList(),
            // ... obviously other details, like the current user from session state...
        };
        context.Orders.Add(order);
        context.SaveChanges();
    }
}

private OrderLine createOrderLine(MyDbContext context, OrderedProductViewModel orderedProduct)
{
    if (orderedProduct.Quantity <= 0 || orderedProduct.Quantity > MaximumOrderSize)
        throw new ArgumentException("orderedProduct.Quantity", "Naughty User!");

    var product = context.Products.Single(x => x.ProductId == orderedProduct.ProductId); 
    var orderLine = new OrderLine
    {
        Product = product,
        Quantity = orderedProduct.Quantity,
        UnitCost = product.Price,
        Cost = product.Price * orderedProduct.Quantity,
        // ...
    };
    return orderLine;
}

It accepts a collection of OrderedProductViewModels, essentially value pairs of product ID and quantity to order. We could have added a quantity value to ProductViewModel and just set that client side and passed that back, but as a general rule it's better to follow Single Responsibility Principle ("S" from S.O.L.I.D.) so that each class or method serves one, and only one purpose so that it only has one reason to change. It also keeps our payload being transmitted only as large as it needs to be. Quantity serves no purpose when listing available products. (unless we want to display stock quantity, but that is a different purpose to ordered quantity) Attributes like product price or even name serve no purpose when creating an order, and as outlined above, can actually be dangerous to accept from the client, as they may accidentally be trusted and used.

The above example is very bare-bones, but should demonstrate the idea. For instance I regularly use a unit of work pattern (Mehdime DbContextScope) for managing the DbContext reference so that I don't need to pass references around. You could have a module-level Context with a lifetime scope to the request managed by an IoC container like Autofac, Unity, or Windsor and that's fine too. Go with what works and refine from there. The key point is not to trust data from the client, and keep payloads small. Entity Framework is very efficient at pulling entities from the DB by ID so there's no need to think you need to cache entities or save time by not reloading data by passing entities from the client. It's prone to vandalism, heavy on traffic between client and server, and prone to all kinds of bugs & unexpected behaviour. (Such as stale data between when data was first read and sent to the client, and when the client returns it to be updated. (concurrent updates) The only way to detect this and handle it is reloading the data from the DB anyways.)

Steve Py
  • 26,149
  • 3
  • 25
  • 43
  • Thank you so much @steve-py for your nice and comprehensive answer concerning mostly security issues. Your solution is great when there are only one product in the cart. But in my case there are multiple products which are added from different pages to the cart. So, I should make use of Session to hold the list. By the way I found a compact solution with little overhead on db which is mentioned as a comment on the question. It would be great if you update your answer for multiple products in the cart. – Abe May 01 '19 at 06:45
  • I've added a section to cover off the change to handle multiple product+quantity in the cart. – Steve Py May 01 '19 at 21:46