DomainObjectAllocator should verify the object being allocated is a Domain Object

[jcarvalho]

DomainObjectAllocator is currently not verifying whether the object being allocated is really a domain object.

If it is not, strange errors may occur.

[smmf]

IIRC the DomainObjectAllocator is called only from the fromOid() operation. Does it make sense to consider that it would be used for anything other than a domain object? Or are you trying to address the case where someone just gives a random OID (e.g. from an external call) and that happens to get through to the DomainObjectAllocator? This has been the case for a long time. Why is it a problem; can you give a concrete example? Thanks.

[jcarvalho]

A concrete example of an issue is the following:

Module A declares class X in the DML. Module A gets a major refactor, the X entity still exists, but is no longer a domain object.

You now have an entry in the DomainClassInfo, as the java.lang.Class object is valid (just not a Domain Object). When you try to load existing data, an OID for X is found, and the allocator is invoked (as the Class object is not null). What happens next seems to be JVM dependent, I've seen it throw a NoClassDefFoundError and cause a Segmentation Fault.

[jcarvalho]

Then again, perhaps this can be solved in a more efficient manner by ensuring that the DomainClassInfo entry is not inserted if either the class does not exist or does not extend DomainObject.

[smmf]

Your last proposal makes sense. Just a note: probably we'll need still to insert the class in the map, maybe just mark it as "deleted", or something. I'm thinking about what should happen when, after such refactoring (where a domain class is removed), later some outdated client may call a method using an externalId referring to such class. I'm thinking quickly about this, but I recall some similar issue already popping up in the past regarding domain classes that are renamed, which seems somewhat similar.

[jcarvalho]

For the JVSTM/OJB backend, this was fixed in ef709d367d68e3d4ef36dba4068339cbb242390c.

[smmf]

After commenting on commit ef709d367d68e3d4ef36dba4068339cbb242390c, I just realized that in this thread, a bit higher @jcarvalho mentions that we could check for inheritance from DomainObject. I really thing that this is the way to go, as long as there is no need to do a more specific check.

[jcarvalho]

(I'll reply here for both the issue and your comment on my commit).

Regarding the specific class within the JVSTM/OJB backend, at the moment, it is the only one that is usable (in fact, ADO and OBDO haven't been merged due to lack of time).

Regarding the usage of the DomainObject interface for checking, while it is enough to cover the typical case of a class not being a domain object anymore, it doesn't cover the case in which a non-DO class is inserted on purpose, in order to crash the JVM running it, hence my original proposal of making that check at the DomainObjectAllocator, which has access to the concrete backend class in use.

[smmf]

Regarding the specific class within the JVSTM/OJB backend, at the moment, it is the only one that is usable (in fact, ADO and OBDO haven't been merged due to lack of time).

I did not understand the above. Can you explain? Currently pt.ist.fenixframework.backend.jvstmojb.pstm.OneBoxDomainObject is a subclass of pt.ist.fenixframework.core.AbstractDomainObject (with 2 other classes in between). I was proposing to use the latter instead of the former. Why do you say it is not possible?

[Sorry, if I did not understand why ADO cannot be used, and you need to explain it further. I haven't been coding on the FF in a while.]

Regarding the usage of the DomainObject interface for checking, while it is enough to cover the typical case of a class not being a domain object anymore, it doesn't cover the case in which a non-DO class is inserted on purpose, in order to crash the JVM running it, hence my original proposal of making that check at the DomainObjectAllocator, which has access to the concrete backend class in use.

I don't want to be nitpicking and I'll be happy if OBDO can be replaced with ADO, but your expression is odd. You said:

a non-DO class

By definition that means some class that does not implement DomainObject, right? So, such class would fail the new test we're discussing. How, then, would it crash the VM? I remember your comment, but that's without the test. Or did you 'abuse' the language, and you meant some class that implements the interface of DO, but is not a subclass of ADO? If that's the case then, again, I would recommend checking with core.ADO as a solution.

[jcarvalho]

The stream of comments is getting a bit mess, so I'll explain my reasoning for the change.

In a previous version of the Fenix Framework, DomainObjectAllocator was changed, to use a VM-internal API ReflectionFactory. This allows us to instantiate an Object X, only invoking the constructor of a superclass (typically, the backend-specific domain object superclass). This change required DomainObjectAllocator to no longer provide a single static method, and instead, receive upon its construction, said backend-specific superclass. So when the allocateObject is instantiated, a new object of the given class will be instantiated, using only the constructor of the class passed upon the DomainObjectAllocator's construction.

The issue here, is that if the class given to the allocateObject method does not extend that specific backend-specific class, strange errors may occur (I've seen it return null, throw a NoClassDefFoundError and even kill the VM with an OS-level error). As such, checking for DomainObject (or even AbstractDomainObject) would not be enough.

To fully prevent any errors, the allocateObject itself should perform this validation. However, since it is on a critical execution path, that check greatly increases the method's complexity and execution time. As such, a compromise was taken: perform that check when loading entries from the DomainClassInfo, as that would be the time when all possible classes are loaded into the system.

[jcarvalho]

On a sidenote, adding the check to DomainObjectAllocator could prevent crashes when it is maliciously used outside the framework, but nothing really prevents a malicious caller from invoking the underlying API directly :)

[smmf]

Thanks for taking the time. I had forgotten about the change to DomainObjectAllocator to use ReflectionFactory. I'm still a bit bothered by the JVSTM/OJB backend doing a specific test with OBDO, because, as I initially mentioned, it precludes using other generators in that backend, but I understand the need you have for ensuring that the VM does not crash ;-) Anyway, you might consider opening an issue about the fact that this has been left hardwired?! Either way, I'll close this one.