Closed nandosola closed 10 years ago
Looks good. The Phantom sequence would make defined_by unnecessary except for schemify (unique index) and perhaps error handling, plus we prevent possible performance problems with the length, but I agree it blurs the intent.
In the near future, VO inheritance might be an interesting feature. The use case would be, for instance, a Location
model, with Country
, Province
, and City
as members.
When defining the PKs in Sequel, please do not use the foreign key
keyword:
For simple PKs:
create_table(:foos) do
String :code, :primary_key=>true
String :description
end
create_table(:bars) do
String :code, :primary_key=>true
foreign_key :foo_code, :food, :null=>false
end
For composite PKs
create_table(:items) do
Integer :group_id
Integer :position
primary_key [:group_id, :position], :name=>:items_pk
end
create_table(:wadus) do
...
foreign_key [:item_group_id, :artist_location], :name=>:items_group_id_position_fkey
end
See here for more help.
Hola,
Actualicé
https://github.com/nandosola/dilithium-rb/issues/49
On 18 Mar 2014, at 17:48, Mario Camou notifications@github.com wrote:
We have a problem when defining VO with CTI using multiple identifiers: foreign_key accepts only a single field
— Reply to this email directly or view it on GitHub.
Un saludo, //nando https://github.com/nandosola
—— "It is a mistake to think you can solve any major problems just with potatoes"
Never mind... I deleted the comment since I found out how to do it. Misread on my part.
How should we handle the _version?
That means that every row in the table has a _version_id column referencing the same row in the versions table
- Advantages:
Least work to implement right now
Disadvantages:
For a new value you need to scan the table to see which version to add to the object. This might also lead to architectural dependency problems since
. 2. The value class has the version, the instance's _version method forwards to the class' _version method. This would ensure that all values of the same type have the same version. Here I see 3 options regarding where to store the version reference in the DB:
2.1. Every row has its own _version_id column referencing the same row in the versions table
- Advantages:
Most similar to the way BaseEntity works
Probably the least work to implement right now of the 2.x options
Disadvantages:
Again, to get the current version you need to query the table, what I see as the most complicated part here is defining where to put that query so we don't violate Separation of Responsibilities. From a data coherence perspective it should be done when loading the class (so it always has a valid version) but we need to go down from BaseEntity to the Mapper to get it. Another option would be to load it lazily the first time you read from or write to the DB but then you have the Mapper having to call the entity to set the version (again SoR, plus an inverted flow of control that I don't like at all).
2.2. A single value_versions table, keyed by value table name
- Advantages:
Ensure at the DB level that all rows representing values of the same type reference the same version. This also better expresses the intent that all values have the same version
Avoid table explosion and possible name conflicts (vs. option 2.3)
Disadvantages:
The value data no longer references the version
The value_versions table could become a point of contention
2.3. One
_version table per value class (where is the table name for the value class) and with a single row each *Advantages:
*Same as 2.2, plus we don't have table contention for the single value_versions table
*Disadvantages:
- Explosion of tiny tables in the DB, which complicates schema management
What do you think? To me, 2.1 or 2.2 sound better, but we have the problem mentioned in 2.1 regarding getting the version for a class when the application is loaded or when the first instance is created at runtime (created in memory with probably others already existing in the DB).
I've given some thought to this one and... It turns out a version is not needed at all. Value Objects are Immutable by nature. They can only be created, which doesn't require versioning.
All the locking (well, it's more of an access locking) mechanisms could be implemented by the UoW:
REGISTER_CLEAN
(no prob)REGISTER_DIRTY
(must check if there are any VOs of the same class already registered as DIRTY
) - But this IMHO is just business logic: "hey, I like to batch-load my VOs and then add several new ones" or "nope: my VOs are just added atomically".All in all, IMO we shouldn't meddle with this versioning issues. Let the user decide how she wants to "visualize" her VOs when updating them inserting new ones. Again, language's a bitch.
Of course, we should:
UPDATE
operations on an existing VO row and send a warningDIRTY
. Only as NEW
or CLEAN
.No editing of VOs with the same identifier? What about, i.e., country (or location) name changes (with no corresponding change of their code)?
Honestly, I can't think of such a use case. They're value objects. Just disallow it. Convince the Customer to create a new registry. Let's implement this and wait for complains ;-)
but The same wouldn't go for active/inactive though: think of a customer in (former) Yugoslavia becoming a customer in Montenegro, for instance. IMHO this is more of an emergency db maintenance:
So, I wouldn't even allow DELETE
operations via API.
Currently BaseValue
s are mutable (mutators are automatically created). I see several options:
attach_attribute_accessors(attribute_descriptor, mutable = true)
class ImmutableDomainObject < DomainObject
I like option 2 best but to make a full refactor we should also make the ::Immutable
objects associated to BaseEntities extend this class. We also need the mutator for load_self_attributes. This means that in either case, for this to be complete, we should also refactor it. Or perhaps have the mutators be private, be removed from the class, or throw an exception if they are called after the BaseValue is loaded. This last option is probably the easiest to implement right now.
Go for the easiest now. Refer to this issue in a comment, plz.
:+1:
More food for thought and future work:
Regarding ImmutableDomainObject
and ::Immutable
... Now that I think about, the ::Immutable
s aren't really DomainObjects, so no... they shouldn't inherit from ImmutableDomainObject
I have the ImmutableDomainObject
implemented, waiting for you to review my last PR before making a new one.
Thinking about how to implement the referencing of a BaseValue
from a BaseEntity
I ran into several considerations:
BaseValue
from another BaseValue
, or just from a BaseEntity
? Implementation-wise it's not too different, the basic difference is whether to implement it at the DomainObject
level or at the BaseEntity
level. I'm more interested in how it works designwise.BaseValue
reference when we create the immutable copy of a BaseEntity
? Previously we had decided that an immutable copy would only contain copies of the Generic or Extended Generic attributes, so my feeling is that having a reference there goes a bit against the grain.BaseValue
warrant the creation of a new keyword, or should we allow the attribute
keyword to create BaseValue
s too? If we go the attribute
way, IMHO then copying the reference to the immutable object makes more sense (plus there's less conceptual surface area to address). After all, you don't normally change an attribute's value in-place, instead you replace it with a new value (i.e., Strings, Numbers, etc...) so conceptually they are the same thing. It would also then make more sense to allow a BaseValue
to contain references to other BaseValue
s.So the proposed solution would be something like:
class Planet < Dilithium::BaseValue
attribute :iso2, String
attribute :iso3, String
attribute :name, String
attribute :type, String
end
class Alien < Dilithium::BaseValue
attribute :race, String
attribute :subrace, String
attribute :origin, Planet
end
class Species < Dilithium::BaseEntity
attribute :name, String
attribute :origin, Planet
attribute :leader, Alien
attribute_list :conquered_planets, Planet # <-- This will not be implemented yet
end
WDYT?
I think that we should raise an exception when inserting/updating a BaseEntity
that references a nonpersisted BaseValue
(i.e., one that doesn't exist in the DB). Also, when inserting/updating a BaseEntity
we should NOT follow the BaseValue
reference and update it if it exists. WDYT?
Also, what happens when creating a BaseEntity
with a reference to a BaseValue
? If we pass in the BaseValue
as an object in the in_h
there should be no problem. OTOH, if it's a "full hash" I see a couple of possibilities:
1.1) Go ahead and create an instance of BaseValue
with the provided data. We'll take care of it when the user tries to persist it. The problem here is a possible inconsistency: the object's values might not correspond to the ones stored in the DB.
1.2) Same as 1.1 but raise an exception if the values do not match
2) Discard the BaseValue
reference and get the actual BaseValue
from the repository using its keys. That ensures the in-memory copy matches whatever is in the DB and that the BaseValue
actually exists at creation time, but can introduce unexpected changes for the user (i.e., object identity and having values change from under them).
I think options 1.2 or 2 are more correct but they do introduce a dependency from the mapper to the repository.
This should not be an issue once we tackle the BaseEntity
builders (see #28) but what do we do meanwhile?
Regarding the previous comment minus one (DomainObject
vs. BaseEntity
and attribute
vs. value
), I've gone ahead and implemented it in DomainObject
using the attribute
keyword. I've gone ahead and pushed the beginning of this work to my repo, I'm thinking of not sending you the PR until I'm finished (still need to do the Mapper and Repo stuff). Have a look and tell me WYT.
Regarding the referencing of a BaseValue
from a BaseEntity
:
class Species < Dilithium::BaseEntity
attribute :name, String
value :origin, Planet
value :leader, Alien
end
vs.
class Species < Dilithium::BaseEntity
attribute :name, String
attribute :origin, Planet
attribute :leader, Alien
end
Bear in mind that the attribute
keyword handles BasicAttributes::GenericAttribute
and BasicAttributes::ExtendedGenericAttribute
- Value objects are just a collection of attributes that are referenced by PK instead of embedded (EmbeddableValue
).
However, the BaseEntity::Immutable
is designed so that no references are held inside because the user is interested just in the BaseEntity
itself. But IMO VOs should be the exception. The reason is that VOs could be part of the BaseEntity
(hence this discussion); they aren't attributes, but they have an accessor to a single (immutable class).
OTOH, and by the same reason, VOs could be the exception for adding a non-BasicAttributes::GenericAttribute
to the attribute
keyword, which doesn't "break" the BaseEntity::Immutable
concept.
All in all, I'd say this is kind of an idempotent implementation. Carry on with whatever is simplest.
Now, to the BaseEntity
updates:
1) VOs are not references, so the tsort
wouldn't arrange them. Could the ObjectTracker
be changed so that there's no need to raise an exception? For instance, add a BaseEntity#has_value_objects
helper method and update the ReferenceSorter
implementation accordingly. This is indeed a side-effect of not treating the VOs as special references.
2) The Mapper
should take care of not updating existing VOs. Repository and mapper lie at the same level and are even conceptually merged many times.
3) As for the BaseEntity
initialization hash, for the moment let's just require the VO's PKs in the hash. The same BaseEntity#initialize
could take of querying the value object's repository and return an immutable copy (the VO itself). If there's a missing key, throw an exception. This, IMO, is essentially correct. Repositories are just a mean of encapsulating queries and, more importantly, a way to providing memory-like access to persisted objects.
ValueObject
s are actually treated as a kind of reference (their implementation is very similar: an object that you reference which is persisted via a foreign key). This means that tsort
should work but I have to test that. What I was talking about regarding exceptions, is that a VO should be added explicitly to the transaction and not persisted as a side effect of persisting its reference object. When it gets to update
or insert
, if it's not yet persisted we get an exception back. Anyway, in the use cases we've seen I believe you never persist a VO as part of the same transaction as a BaseEntity
, you have a separate process to add new VO's so by the time we get to persisting the BaseEntity
it should already be there. If we add this restriction then the tsort
wouldn't have to change, just the Mapper#insert
and Mapper#update
methods (the latter should always raise an exception).Mapper#update
with a VO should always raise an exception.Repository
. I like it. The Hash
would look something like (ref. the above Species
Entity):{
name: 'Dalek',
origin: {
iso2: 'SK'
},
leader: {
race: 'Kaled',
subrace: 'Dalek hybrid'
}
}
Any other attributes within leader
and origin
would be ignored and we would get back VOs with the actual values from the DB.
Tell me if I'm missing or misinterpreting something.
VOs are registered either as CLEAN
or as NEW
into the UoW. An exception should be thrown otherwise. There's no problem with clean VOs, but new ones should be taken care of by the tsort
mechanism we already have in place. Just make sure of it, ok?
As for the other issues, carry on!
I assume that we can also register them as DELETED
since we're using soft-deletes? That means that only REGISTER_DIRTY
should raise an exception.
Makes sense. Go ahead
Never mind about the DELETED
, I got confused and thought we were using soft-deletes within the Transaction. The Transaction actually DOES a delete, so I agree with the first proposal that a BaseValue
shouldn't be able to be registered as DELETED
. We should really clear that up at some point.
Never mind about the previous comment! There's a bug in my code :) So, REGISTER_DELETED
remains kosher for BaseValue
s
Regarding the Transaction. We have a problem with the way the tsort
works. If a ´BaseEntity´ references a BaseValue
, even though it hasn't been added to the transaction, the walk through the tree adds it (since tsort_each_node
and tsort_each_child
don't have a global view of the tree). I see a few options:
BaseValue
s that are referenced by an entity have already been added in a previous transaction (since there doesn't seem to currently be a use case where we have to add aBaseValue
in the same transaction where a BaseEntity
that references it).BaseValue
s that are referenced by BaseEntities
to be automatically inserted when the BaseEntity
is added with REGISTER_NEW
.commit
, scan the tree ourselves to find any BaseValue
s that are referenced by BaseEntities
that don't exist and aren't registered as REGISTER_NEW
.There is also another problem. We're currently using the object's ID to find out whether it exists in the DB or not. In the case of BaseValue
s we can't do that, since a BaseValue
has its own identifier fields that should always be set. That means that you can do a REGISTER_NEW
with a BaseValue
which already exists, and you will get a Sequel::UniqueConstraintViolation
on commit
. Choosing option 2 above would also mean that we have to catch that exception and refresh the in-memory values, whereas option 1 allows us to have a better handle on this problem.
WDYT?
Actually another option would be to enforce that, when doing a register_new
on a BaseEntity
, all BaseValue
s have to be previously registered. That would mean not having to walk the whole tree, instead we walk that BaseEntity
's parent/child tree and ensure that any BaseValue
s referenced either already exist in the DB or are marked as register_new
. That would involve doing DB queries for each BaseValue
that isn't registered in the current transaction.
Well, VOs are immutable, so handling them inside a transaction scope is kind of weird. So let's just:
BaseValues
' (composite) PKs into their referencing BaseEntities
as getters. Never leak them through any BaseEntity
's Mapper/Repository.BaseValue
registered into a transaction (UoW) and/or throw an exception.BaseValue
persistence (ie. Mapper::Sequel.mapper_for(Planet).insert(a_planet)
)However, our primary use case is being able to bulk-edit all the instances of any given BaseEntity
at a time, so maybe a SELECT ... FOR UPDATE
(explicit locking) or LOCK may be necessary prior to performing update operations?
Value objects are just a set of immutable attributes (Extended or GENERIC) and methods. No references are allowed (or needed atm). Inheritance is not supported. The definition could be as follows:
BaseEntities
use them in the following way:The database part is the trickiest. It would involve:
All its registers point to the same version (bulk editing)BaseEntity
via FK (non-id) or composite FKs.varchar(32)
at the most, because of db performance.The latter totally ruins the domain logic, so either one of these must be implemented:
varchar
and wait until a problem arises (remember the first rule of performance tuning: don't)BaseValue
serialization and use it to establish references. However, this would harm the intent expressed by thedefined_by
keyword