tzaeschke / zoodb

ZooDB Object Database
Apache License 2.0
57 stars 9 forks source link

DataSerializer fails on empty arrays #98

Closed slavap closed 7 years ago

slavap commented 7 years ago

The following code fails with NPE exception:

public abstract class Content extends ZooPC {
    Property[] properties;
}

public class Folder extends Content {
    String name;
    public Folder() {}
}

public class Property extends ZooPC {
}

public class StringProperty extends Property {
    String value;
    public StringProperty() {}
}

{
    pm.currentTransaction().begin();
    Folder f = new Folder();
    f.name = "abc";
    f.properies = new Property[] {};
    f.zooActivateWrite();
    pm.makePersistent(f);
    pm.currentTransaction().commit();
}

The problem in DataSerializer.writeClassInfo(Class<?> cls, Object val) method:

        //for persistent classes, store oid of schema. Fetching it should be fast, it should
        //be in the local cache.
        if (isPersistentCapable(cls)) {
            out.writeByte(SerializerTools.REF_PERS_ID);
            if (val != null) {
                long soid = ((ZooPC)val).jdoZooGetClassDef().getOid();
                out.writeLong(soid);
            } else {
                long soid = cache.getSchema(cls, node).getOid(); <<< HERE if getSchema() returns null then out.writeLong(SerializerTools.REF_NULL_ID) should be used, or perhaps getSchema() is not needed at all and REF_NULL_ID should be always written.
                out.writeLong(soid);
            }
            return;
        }
tzaeschke commented 7 years ago

Could you give me some more information, such as

So far I tested it with devTZ_main branch, and I get either no Exception (predefined schema) or a JDOUserException (auto-create schema). I don't see an NPE at the moment.

Also, I noticed that you are missing zooActivateWrite() before setting the fields. This is usually done in the setter() methods. However, adding/removing zooActivateWrite() doesn't seem to make a difference in terms of Exceptions or not...

slavap commented 7 years ago

Yes, I'm using schema-auto-creation. I will create test project for this bug. I'm working with zoodb 0.4.9 And I was under impression that it doesn't matter when f.zooActivateWrite() is called, so in my understanding before or after are doing the same thing.

slavap commented 7 years ago

Unfortunatelly, I cannot attach zip file in comments, so I've renamed zip to png. Download, rename back to zip, and execute runme.cmd, then you'll get NPE. Let me know if you need any additional info. zoodbbugs

tzaeschke commented 7 years ago

Thanks, I can reproduce it now. I'll have to investigate why it doesn't happen in my normal test environment.

zooActivateWrite() has several purposes. On new objects, it probably does not matter when it is called. However, for example, for objects coming from the database (when their state is 'hollow'), calling zooActivateWrite() will read the object from the database and overwrite any changes to the object that may have occurred since the last commit or rollback. The save option is to call zooActivateWrite() in the beginning of each method that modifies an object, for example in every setter method. The method has very little overhead once the object is loaded and flagged as 'dirty'.

tzaeschke commented 7 years ago

Just in case, a simple workaround is to define the schema manually, at least for the Property class by doing the following before the main transaction:

        pm.currentTransaction().begin();
        ZooSchema schema = ZooJdoHelper.schema(pm);
        schema.addClass(Property.class);
        pm.currentTransaction().commit();

Also, with schema.getClass(...) allows you to check whether a schema is already defined.

This qualifies for the ongoing bug bounty. If you are interested, could you contact me via zoodb(AT)gmx(DOT)de to organize the bounty?

slavap commented 7 years ago

@tzaeschke Thanks. I've workaround this problem by just storing fake StringProperty with my main/root folder, after that following empty Property arrays are stored just fine. But your workaround looks good to me as well. Will send you email for organizing the bounty, really surprised by this fact.

tzaeschke commented 7 years ago

Should be fixed now on devTZ_main branch. I'll look at the other issue tomorrow. Thanks for reporting!