dibley1973 / OpenRMS

Open RMS is an Open Source project with the intention of delivering a retail management platform that is free to install, use, modify and distribute.
GNU General Public License v3.0
9 stars 7 forks source link

Help with ValueObject and Entity Framework Core #29

Open jadjare opened 7 years ago

jadjare commented 7 years ago

Hi,

I've been working on issue #1 and as part of that added a Code property to the Item entity. I decided to do this as a ValueObject implementation, rather than a straight string - (I'm not 100% sure whether it's actually worth it in this instance, to be honest!?) However, the code does read quite nicely

    public class Item : AggregateRoot
    {

        public Item(ItemCode code, string name)
            : this(Guid.NewGuid(), code, name) {}

        public Item(Guid id, ItemCode code, string name)
            : base(id)
        {
            if(code == ItemCode.Empty) throw new ArgumentException("The Item Code must be set.  An empty code is not permissable",nameof(code));

            Code = code;
            ChangeName(name);
        }

        // .... rest of class
}

// and an excerpt of ItemCode implementation

public class ItemCode: ValueObject<ItemCode>
{
     public static readonly ItemCode Empty = new ItemCode(string.Empty);
     public string Value { get; private set; }

     private ItemCode() { }

     public ItemCode(string value)
     {
         if(value==null) throw new ArgumentNullException(nameof(value));

         Value = value;
     }
    // .... rest of class
}

After refactoring everything and getting all the tests passing again I've hit a problem. Entity Framework Core.

It immediately moaned about not having an Id field on ItemCode. Now as I recall in EF6 it simply handled this circumstance by treating the property as a value object and added the properties of the class to the parent table as columns, e.g. ItemCode_Value. I think you could also mark it as a [ComplexType] if you wanted to be explicit.

Unfortunately it appears that EF Core doesn't support Value Objects (yet). See this post just authored by Julie Lerman (Oct 2016)
http://thedatafarm.com/data-access/completely-new-ef-in-the-enterprise-course-on-pluralsight/

I tried smacking an Id property on the ItemCode object just to see if I could get it to spin up properly, the database created fine, but PostgreSQL threw a wobble about relationships between objects when trying to create an item... I need to get some sort of SQL management tool, just to visualise what db structure it created - any suggestions?

Given it appears that EF Core doesn't support value objects at present, I guess it might mean one of two or three things:

  1. Don't use Value Objects in the domain
  2. Introduce the concept of State Objects to avoid EF forcing the design of domain. See this Vaughn post (https://vaughnvernon.co/?p=879)
  3. I've messed up the implementation and only that needs correcting.

Any thoughts on the three options above? Note: I've checked in my changes to the branch openrms/develop-#1, in case anyone thinks they've got another way of getting working or can spot any mistakes

dibley1973 commented 7 years ago

I think it would be a shame to not use ValueObjects in the domain just because the chosen ORM version does not support it.

Assuming this issue here early advice seems to be stick with EF 6 if we want them, as it is coming later. However this was a long time ago.

Also interesting bit about postgres here in same issue.

Have you tried using [ComplexType] attribute on the value object? Or does the attribute not even exist yet?

Personally I'd like to find a workaround until they are natively supported as I think it will help and make coding easier in the future as the project grows.

johncollinson2001 commented 7 years ago

Yo,

For a database admin tool for postgreSql use pgadmin: https://www.pgadmin.org/. As you've used SSMS you'll find this very similar.

For your options, defo not no 1 - we've gotta be able to support value objects! Found this issue on the EF backlog so it is coming... https://github.com/aspnet/EntityFramework/issues/246. I think you're implementation is right... we should do one of the following:

I think I'd plump for the bottom one. It's less work, and as long as we mark the code up in a big comment saying that it should be removed once EF core has caught up, that would be fine... so something like:

public class Item 
{
    private ItemCode _itemCode;

    // Only here for the purpose of EF!
    public string ItemCodeValue {
        get { return _itemCode.Value; }
        set { _itemCode.Value = value; }
    }
}

(Don't think that code will build - just tapped out with VS open!)

jadjare commented 7 years ago

Have you tried using [ComplexType] attribute on the value object? Or does the attribute not even exist yet?

That was my first instinct. I went to put it on the ValueObject itself and then quickly realised that I'd have to let the domain know about EF, so stopped there and moved on to creating a ModelBuilder Entity Configuration using the fluent api. That's when I started to get suspicious - where's the ComplexType<T>() method, I couldn't find it. So I guess, based on the evidence that Value Objects aren't supported, that ComplexType is not available.

jadjare commented 7 years ago

@johncollinson2001, not followed the link yet (apologies for that).... your suggestion is not dissimilar to introducing a state object, on the surface it is simpler. However, if the Value Object has many properties it will become quite cumbersome to deal with...

Off the top of my head, a state object implementation would look a little like this: The downside of this is it's yet another bloody DTO!!!

public class ItemState
{
    public Guid Id {get;set;}
    public string ItemCode_Value {get;set;}
    public string Name {get; set;}
    public string Description {get;set;}
}

public class Item: Aggregate<Item>
{
    private readonly ItemState _state;

   //... the standard constructors we've currently got +
   // ... All properties talk back and forth via the ItemState.

   public Item(ItemState state){
        _state=state;
    }

    public string Description { get { return _state.Description; }}

    public void ChangeDescription(string description)
    {
        _state.Description=description
    }
}

///... repository

public Item GetForId(Guid id)
{
    var itemState = _context.Items.Find(id);
    return new Item(itemState);
}
public void Add(Item item)
{
    _context.Items.Add(item.ItemState);
}

Not decided on any particular way at the moment... just providing the above to aid discussion.

dibley1973 commented 7 years ago

@jadjare - Do you then need a property on the Domain Entity which exposes the state for EF to use going back to the DB? If so how do we stop others accessing the state and bypassing our lovely domain methods to manipulate the state data?

johncollinson2001 commented 7 years ago

To keep it entirely out the domain we could define the state object on the infrastructure project, and map the entity to the state object there?

jadjare commented 7 years ago

I've got to disappear! At the moment I'm thinking of just taking @johncollinson2001 's suggestion of just whacking in a property called ItemCodeValue. This will let me merge in my branch in the short term.

This discussion can then carry on separately... It might be, for instance, that we should consider using another ORM tool (e.g. n-hibernate).

P.s. keep thinking / discussing ideas, the decision above is just so my branch doesn't get too far behind resulting in lots of refactoring when I next merge.

dibley1973 commented 7 years ago

N-hibernate gives some nice events to tap into when the entity is updates, I believe...

johncollinson2001 commented 7 years ago

I don't think nhibernate has been ported to .net core yet (after a 30 second Google search mind...)

jadjare commented 7 years ago

Quick update:

For now I've gone with the following so I can get the code checked in and merged back to "develop"

        #region EF Backing Properties

        internal string ItemCodeValue
        {
            get { return Code.Value; }
            private set
            {
                if(Code!=ItemCode.Empty)
                    throw new InvalidOperationException(string.Format("The {0} property cannot be changed. It is provided for data binding operations only", nameof(ItemCodeValue)));

                Code = value;
            }
        }

        #endregion

... and added an Ignore to the ItemConfiguration.

As you might notice above the ItemCodeValue property is internal, this actually means that EF is not picking up the column. This is because I've hit another problem - updating the database schema. I shall open up a separate discussion point for this.

johncollinson2001 commented 7 years ago

Pushed a fix into develop which will allow EF to recreate the database with the new model (see comment on #30 for more info).

Should we mark up areas (such as above) that we intend to remove once support for value objects is available in EF? Nice fat comment above each relevant method/property/etc with a TODO of FIXME heading?

On the comment above, just seen it's been added in a region.

jadjare commented 7 years ago

Although you've struck your remark about a TODO... It's a good call as there easy to find and track. I've added a TODO above the property and will be available to all on my next merge

CESARDELATORRE commented 7 years ago

Since ComplexTypes or any other comparable ValueObject implementation is still not supported in Entity Framework Core, you could still do the following:

The EF team is considering possible approaches for VO or ComplexTypes in the future..