Create builders for BaseEntity

[mcamou]

With the addition of invariants we need some way to create entities and execute the validation after they have been created. The current approach (creating from a Hash) is fragile. Instead, we should have a self.build method (and a private constructor).

Proposed sample usage:

class User < BaseEntity
  attribute :name, String
  reference :dept, Department
  child :computer
  child_list :foos
end

class Car < BaseEntity
  NUM_WHEELS = 4

  children :wheels

  invariant :wheels count_exactly(NUM_WHEELS)
end

# Use in code

a_hash = get_user_data()
a_dept = Department.find(...)

a_user = User.build do | u |
  u.name = "A User"
  u.dept = a_dept

  u.computer = make_computer do | c |
    ...
  end

  a_hash[:foos].each do | f_hash |
    u.make_foo do | f |
      f.index = f_hash[:index]
    end
    # or
    u.add_foo Foo.build do | f |
      f.index = f_hash[:index]
    end
  end
end

Car.build do | c |
  1..NUM_WHEELS.each do | i |
     c.make_wheel { | w | }
   end
end

[mcamou]

So... the build method seems to be a 3-liner:

    def self.build
      new_object = self.new
      yield new_object
      #TODO Validate invariants
      new_object
    end

Now the question is... should we go ahead and get rid of full_update, the constructor that takes a Hash, and all their associated methods? (make, check_input_h, load_*_attributes, etc)? From what I see, full_update is used by Transaction#rollback and the load_*_attributes and check_input_h are used by make and the constructor (and make is just used in the tests).

[mcamou]

We'll leave this issue open, even though the builders are already implemented we still have to clean up the methods mentioned in the last comment.

[mcamou]

For starters I'll remove the Hash constructor and the make method. check_input_h and load_*_attributes will have to wait until we remove full_update

[mcamou]

Heh heh... not a three-liner... still battling with getting _version right (and changing all tests to use the new syntax)

[mcamou]

Having problems with propagating the _version down during the call to BaseEntity.build. Example:

  a_company = Company.build do |c|
      c.name = 'Abstra.cc S.A'
      c.add_local_office LocalOffice.build do | lo |
        lo.description = 'branch1'
        lo.add_address Address.build do | a |
          a.description = 'addr1'
        end
      end
    end

The problem is that the object is built from the inside out, so the parent's version isn't available when build is called. If you set the version during add_X, it works at the first level (LocalOffice), but by that time the second-level add_X (Address) has already been called and its version is set to nil.

What I'm thinking is that BaseEntity.build could a second parameter, the parent's version. If the BaseEntity is an aggregate root (i.e., no parent), its version is assigned in the constructor. For children, you would have to pass the parent's version to the child builder:

c.add_local_office LocalOffice.build(self._version) do | lo |
  ...
end

We can make it nicer by using make_X instead of add_X (which also takes care of the version):

c.make_local_office do | lo |
   ...
end

As a matter of fact, we should never use the add_X / build combo on children, instead always use make_X. I don't see a way to enforce this since we really can't remove the build method. make_X calls it internally and it's needed for BaseEntities, but we could use that convention.

A solution could be not to add the add_X method for child attributes and instead move its functionality directly into make_X. That would probably break client code quite a bit but since this builder thing is probably going to break it anyway it might be a good idea to swallow the pill right now instead of later. Actually it would be even nicer to change the name of make_X to add_X to make it more explicit that you're adding a child (i.e. for child attributes, add_X takes a Proc instead of an object).

WDYT?

[mcamou]

The other big problem is loading from the DB in the Repository. First of all, we're making one query per child. Second, having eliminated the Hash constructor, we have to go either through the build method or assigning directly to the instance variables. I'm thinking that, as part of this, we should redo all this part, make a single query with joins to get the whole Aggregate (or use Lazy references for the children) and use full_update to load the whole thing at one go (since full_update will still have to be there for rollback).

[nandosola]

a) Go on with make_X / add_X b) There should be a single repository per aggregate root; ie. Repository.for(Child) should return ParentRepository

[mcamou]

With the requirement for supporting inheritance in children, what do you think of this:

class Parent < BaseEntity
  children Child
end

class Child < BaseEntity
  attribute :foo, String
end

class BarChild < Child
  attribute :bar, String
end

class BazChild < Child
  attribute :baz, String
end

Parent.build do |p|
  p.make_child(BarChild) do |c|
    c.bar = 'bar'
  end

  p.make_child(BazChild) do |c|
    c.baz = 'baz'
  end
end

If you don't pass a class to make_X, it would use the declared class of the child (Child in this example). Of course, we'd raise an exception if the passed class isn't a subclass of the child type.

WDYT?

[nandosola]

Makes sense to me.

[mcamou]

Almost finished, just one failing test, but I bumped up against a problem with fetch_by_id/fetch_all.

Currently, fetch_by_id returns nil when an entity has active:false, which makes fetch_all return an Array with nils in it. We mentioned that fetch_* should fetch everything (active or not). However, changing this breaks a couple of other tests (having to do with Transaction#register_deleted) . So, final call. Should we:

1. Have fetch_by_id return any object, active or not
2. Leave fetch_by_id as is and change fetch_all to include all objects, active or not
3. Change fetch_all to filter out nils?

My feeling is option 1 is more correct since active is more of a view thing, but we do manage active inside Dilithium (since register_deleted only sets active to false).

[nandosola]

I vote for "1". Cheers!

[mcamou]

Done. Please have a look at the PR (esp. the tests). Can you take care of integration? -Mario — I want to change the world, but they won’t give me the source code

On 11 Apr 2014, at 14:44, Nando Sola notifications@github.com wrote:

I vote for "1". Cheers!

— Reply to this email directly or view it on GitHub.

[nandosola]

Gracias amigo! Will do! Next up: #22