google-code-export / morphia

Automatically exported from code.google.com/p/morphia
1 stars 0 forks source link

Add @Polymorphic #22

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Add support for storing (and searching on) the whole super-class tree and the 
interfaces implemented 
on any of them. Most likely this will be stored as an array of strings called 
super-classes 
(including the current class-name).

Something like this should be supported:

@Polymorphic
public class Shape {
}

public class Circle extends Shape {
}

public class Square extends Shape {
}

Datastore ds = ...;
List<Shape> shapes = ds.find(Shape.class).asList()
for(Shape shape : shapes)
  print(shape.getClass().getName());

> Circle
> Square
> Shape
> Circle
> Circle

Original issue reported on code.google.com by scotthernandez on 31 Mar 2010 at 4:32

GoogleCodeExporter commented 9 years ago
I've started working on the @Polymorphic annotation, and only storing the class 
name in the Mongo object if it is 
present.

Original comment by oli.ga...@gmail.com on 18 Apr 2010 at 4:53

GoogleCodeExporter commented 9 years ago
Great. I checked in code yesterday to remove the class-name for all but the top 
level 
entities, when possible. 

We can remove the top-level ones too if we add some logic to the mapped-classes 
collection. It would require sending in the collection name when converting 
from the 
dbObject to the entity, or vice-versa. 

Right now the collection name is not unique in the mapped classes. This 
presents an 
issue. In previous projects we have made it a unique restriction and required 
that 
the class be mapped before it could be used (queried/saved/etc). I think we can 
do 
away with that and just take the first one we find, and throw a warning if 
there are 
more. It will then be up to programmer to guarantee the behavior is what they 
want. 

Original comment by scotthernandez on 18 Apr 2010 at 5:05

GoogleCodeExporter commented 9 years ago
It will be very usefull feature, but be carefull to store full class names in 
objects.

Let's imagine situation on working production database, using polymorphic object
hierarchy.
Now your boss order you to do some refactor on model classes, and you have to 
change
package names or class names.
In that situation, model and database state become inconsistent.
To chandle that situations, in JPA spec (J2EE) there is a few annotations, for
example @DiscriminatorValue.

Maybe store some discriminator value instead of storing class name.
For example:

@Polymorphic(discriminator="animal")
@Entity
public class Animal {...}

@Polymorphic(discriminator="dog")
@Entity
public class Dog extends Animal {...}

What do you think ?

Original comment by kalondar on 6 May 2010 at 1:25

GoogleCodeExporter commented 9 years ago

Original comment by scotthernandez on 20 Oct 2010 at 6:32

GoogleCodeExporter commented 9 years ago
Yep, we should do @Polymorphic([discriminator="...", 
alterAndCreateIndexes=true]) and also follow the format as they have done here: 
http://github.com/samus/mongodb-csharp/wiki/Inheritance

It means dynamically creating indexes based on this as well. And any queries 
will have to include the discr. value.

This could be rather invasive to do seamlessly but it should just work.

Original comment by scotthernandez on 30 Oct 2010 at 3:13

GoogleCodeExporter commented 9 years ago
I think that creating indexes dynamically is much too invasive. It would be 
better to add @DiscriminatorField annotation so that developer can decide on 
indexes on his own.

@Polymorphic(discriminator="animal")
@Entity
public class Animal {
@Indexed
@DiscriminatorColumn
String discriminator;
...
}

@Polymorphic(discriminator="dog")
@Entity
public class Dog extends Animal {
@Indexed
@DiscriminatorColumn
String discriminator;
...
}

This way we can achieve all we need and full polymorphism (including super type 
querying) can be achieved on single collection and collection per type. Of 
course it would need some more validation during mapping.

Is anything being done with this issue?

Original comment by MomotDam...@gmail.com on 11 Apr 2011 at 2:58

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
IMO the solution should be implemented via a Strategy pattern - the Mapper 
class should have a user-replaceable DiscriminationStrategy that implements two 
methods:

writeClass(DBObject dbo, Object entity);
getClass(DBObject dbo);

This way you can have a FQCNDiscriminationStrategy (current implementation), an 
AnnotationBasedDiscriminationStrategy and whatever other strategy the user may 
choose to implement.

Original comment by shai.yal...@gmail.com on 29 May 2011 at 1:47

GoogleCodeExporter commented 9 years ago
I took the liberty of performing this refactoring. Attached is a patch with the 
new interface, a default implementation and another implementation which omits 
a base class name, allowing for a smaller footprint discriminator.

I've made as little change as possible to existing code in order to preserve 
backwards compatibility wherever possible. Work is needed to remove the 
noClassName field in the @Entity annotation (I renamed it to noDiscriminator).

If you could apply this patch it would be great; meanwhile we're working with a 
forked version, local to our maven repo.

Original comment by shai.yal...@gmail.com on 30 May 2011 at 11:42

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks for taking a pass at implementing this but this patch does not include 
any of the things described in the requirements. There is no support for 
polymorphic queries nor a stored array of the classnames/discriminators in the 
object/document.

This patch will not be applied in its current form. If you would like to 
resubmit with full support for the requirements that would be great.

I only took a quick scan of the patch, but what functionality does it add? It 
seems like you have mostly reformatted code, moved things around into new 
classes (your strategies), and renamed an attribute? Is it just support for 
removing a pre-defined package name from the classname, to make it a 
"discriminator", which you have added?

Here is some more background reading:
http://groups.google.com/group/morphia/browse_thread/thread/d9015a648db2864a
http://groups.google.com/group/morphia/browse_thread/thread/14e775e0a29a5bb6
http://groups.google.com/group/morphia/browse_thread/thread/a964383c044b120
http://groups.google.com/group/morphia/browse_thread/thread/c88c594612849beb

Original comment by scotthernandez on 30 May 2011 at 3:21

GoogleCodeExporter commented 9 years ago
I should've been more specific, perhaps :)

I didn't implement the @Polymorphic annotation; I implemented the ability to 
control the way the discriminator property is written into the DBObject. This 
is mainly to solve the issue where an object tree with lots of (polymorphic or 
not) entities in an embedded list consume lots of storage space because of the 
className field.

The second strategy I implemented indeed supports just this - removing a 
predefined prefix from the class name, to conserve storage space, something 
that our Mongo DBA was very strict about :)

I think that in order to automatically filter the query by the discriminator 
field, all that's needed is to add a "filter" method to the 
DiscriminationStrategy interface and implement it accordingly in different 
strategies; this way, @Polymorphic can be implemented using an 
AnnotationBasedDiscriminationStrategy...

Original comment by shai.yal...@gmail.com on 30 May 2011 at 3:30

GoogleCodeExporter commented 9 years ago
There is another issue for just the discriminator:
http://code.google.com/p/morphia/issues/detail?id=82

We could also create a new issue for just your patch, or the issue you are 
trying to address.

While your solution works for you, I'm not sure it is a good general solution. 
I understand that part of this is to allow the user to describe the 
discriminator rules and I think that part might make sense, but a good 
default/general solution might work as well. I was imagining a different 
requirement for reducing the size of the discriminator by using the mapping of 
the (entity) names to classes and back. It would require registering/mapping 
the classes before use, but will work much more generically and without the 
need to define a package name, or be limited to one. It would work hand in hard 
with @Polymorphic as described above.

Original comment by scotthernandez on 30 May 2011 at 3:41

GoogleCodeExporter commented 9 years ago
My point is that the means by which Morphia discriminates the classes are a 
separate concern from the mapping process, which is why I intorduced the 
strategy interface. Currently the default implementation of 
DiscriminationStrategy falls back to the default behavior, saving the class 
name. It should be trivial to write another implementation which uses the 
entity name instead.

Do you want me to resubmit the patch to issue #82?

Original comment by shai.yal...@gmail.com on 31 May 2011 at 5:29