Issue Details (XML | Word | Printable)

Key: NUCEXCEL-36
Type: New Feature New Feature
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Andy Jefferson
Reporter: Guido Anzuoni
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
DataNucleus Store Excel

Support of embedded PC (1-1) objects

Created: 03/May/10 11:23 AM   Updated: 17/Nov/10 09:27 PM   Resolved: 13/May/10 11:48 AM
Component/s: Persistence
Affects Version/s: None
Fix Version/s: 2.2.0.m3

File Attachments: 1. Text File store_excel_emb.patch (41 kB)
2. Text File store_excel_emb_branch_2_1.patch (42 kB)



 Description  « Hide
Excel Store Manager should support embedded (even nested) objects

Sort Order: Ascending order - Click to sort in descending order
Guido Anzuoni added a comment - 03/May/10 11:24 AM
The patch is created starting at src directory

Andy Jefferson added a comment - 03/May/10 03:22 PM
Thx for the patch Guido. I applied it but now all tests in test.jdo.spreadsheet are broken ("mvn clean test -Pexcel"), so not committed to SVN trunk

Guido Anzuoni added a comment - 03/May/10 05:09 PM
Andy,
is there a way to post any stacktrace from your run ?

Andy Jefferson added a comment - 03/May/10 05:14 PM
java.lang.NullPointerException
        at org.datanucleus.ExecutionContextImpl.newObjectProviderForMember(ExecutionContextImpl.java:166)
        at org.datanucleus.store.excel.fieldmanager.StoreFieldManager.storeObjectField(StoreFieldManager.java:93)
        at org.datanucleus.state.AbstractStateManager.providedObjectField(AbstractStateManager.java:1031)
        at org.jpox.samples.models.company.Employee.jdoProvideField(Employee.java)
        at org.jpox.samples.models.company.Person.jdoProvideFields(Person.java)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.provideFields(JDOStateManagerImpl.java:2791)
        at org.datanucleus.state.ObjectProviderImpl.provideFields(ObjectProviderImpl.java:64)
        at org.datanucleus.store.excel.ExcelPersistenceHandler.insertObject(ExcelPersistenceHandler.java:155)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.internalMakePersistent(JDOStateManagerImpl.java:3261)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.makePersistent(JDOStateManagerImpl.java:3237)
        at org.datanucleus.ObjectManagerImpl.persistObjectInternal(ObjectManagerImpl.java:1452)
        at org.datanucleus.ObjectManagerImpl.persistObject(ObjectManagerImpl.java:1279)
        at org.datanucleus.jdo.JDOPersistenceManager.jdoMakePersistent(JDOPersistenceManager.java:656)
        at org.datanucleus.jdo.JDOPersistenceManager.makePersistent(JDOPersistenceManager.java:681)
        at org.datanucleus.tests.PersistenceTest.setUp(PersistenceTest.java:71)

Guido Anzuoni added a comment - 04/May/10 08:13 AM
Andy, I am relying on AbstractMemberMetaData.isEmbedded() to manage the embedded PC case.
I realize that isn't sufficient so I have followed what is done in ExecutionContextImpl for the purpose.
The new patch follows.

Andy Jefferson added a comment - 04/May/10 07:01 PM
But isEmbedded() replies true when the field is a non persistable too. What if a field is Long, or Date, or something else ? You can't send those through
efop = ec.newObjectProviderForMember(mmd, embcmd);

so it NPEs (even with this patch).
java.lang.NullPointerException
        at org.datanucleus.ExecutionContextImpl.newObjectProviderForMember(ExecutionContextImpl.java:166)
        at org.datanucleus.store.excel.fieldmanager.StoreFieldManager.storeObjectField(StoreFieldManager.java:93)
        at org.datanucleus.state.AbstractStateManager.providedObjectField(AbstractStateManager.java:1031)
        at org.jpox.samples.models.company.Employee.jdoProvideField(Employee.java)
        at org.jpox.samples.models.company.Person.jdoProvideFields(Person.java)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.provideFields(JDOStateManagerImpl.java:2791)
        at org.datanucleus.state.ObjectProviderImpl.provideFields(ObjectProviderImpl.java:64)
        at org.datanucleus.store.excel.ExcelPersistenceHandler.insertObject(ExcelPersistenceHandler.java:155)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.internalMakePersistent(JDOStateManagerImpl.java:3261)
        at org.datanucleus.jdo.state.JDOStateManagerImpl.makePersistent(JDOStateManagerImpl.java:3237)
        at org.datanucleus.ObjectManagerImpl.persistObjectInternal(ObjectManagerImpl.java:1452)
        at org.datanucleus.ObjectManagerImpl.persistObject(ObjectManagerImpl.java:1279)
        at org.datanucleus.jdo.JDOPersistenceManager.jdoMakePersistent(JDOPersistenceManager.java:656)
        at org.datanucleus.jdo.JDOPersistenceManager.makePersistent(JDOPersistenceManager.java:681)
        at org.datanucleus.tests.PersistenceTest.setUp(PersistenceTest.java:71)

obtainable using "test.jdo.spreadsheet" with -Pexcel

Guido Anzuoni added a comment - 04/May/10 10:04 PM
Yes, I realize it.
And so what?
How may I detect that a field refers to PC class that is embedded ?
Is this enough ?
        AbstractMemberMetaData mmd =
            sm.getClassMetaData().getMetaDataForManagedMemberAtAbsolutePosition(fieldNumber);

        if (mmd.getEmbeddedMetaData() != null) {

or should I check this too ?
        if (mmd.getEmbeddedMetaData() != null) {
         Class embcls = mmd.getType();
         AbstractClassMetaData embcmd = ec.getMetaDataManager().getMetaDataForClass(embcls, ec.getClassLoaderResolver());
             if (embcmd != null) {
         ObjectProvider efop = ec.newObjectProviderForMember(mmd, embcmd);
         FieldManager ffm = new FetchEmbeddedFieldManager(efop, sheet, row, col, mmd);
         efop.replaceFields(embcmd.getAllMemberPositions(), ffm);
         return efop.getObject();
             }
        }


Guido Anzuoni added a comment - 04/May/10 11:44 PM
The latter option in previous comment "seems" to be OK.

Andy Jefferson added a comment - 05/May/10 09:03 AM
There are many ways of separating out if something is a PC object and embedded. The one I would favour would be
api.isPersistable(mmd.getType()) && mmd.isEmbedded()
but yours works too (at least for the samples in those tests we currently have).

Andy Jefferson added a comment - 05/May/10 07:16 PM
SVN trunk has this patch (with one or two changes so it fits in with the current structure of the XXXFieldManager methods, using relationType). Untested and not added to docs yet

Guido Anzuoni added a comment - 06/May/10 11:42 AM - edited
Here the patch for branch 2_1 (for DN 2.0.4)
Beware that I have backported the fix in ExcelCandidateList (well, as from the comment, the fix should go in superclass)
even if the issue does not appears in DN 2.0.4

Andy Jefferson added a comment - 06/May/10 04:05 PM
Not applied any change to core superclass or ExcelCandidateList since don't understand it and can't see the problem in any testcase. Either way doesn't seem related to embedded PC fields, which is what this issue is

Guido Anzuoni added a comment - 06/May/10 06:58 PM - edited
The issue appears in my my excel sheet that has 341 rows and appears in 2.0.4 too.
The implementation of toArray(Object []a) in org.datanucleus.store.query.AbstractLazyLoadList is

        Object[] array = a;
        if (a.length < size())
        {
            array = new Object[size()];
        }
--->>>>>> for (int i=0;i<array.length;i++)
        {
            if (itemsByIndex != null && itemsByIndex.containsKey(i))
            {
                array[i] = itemsByIndex.get(i);
            }
            else
            {
                array[i] = retrieveObjectForIndex(i);
            }
        }
Now, array.length may be larger that size() in which case retrieveObjectForIndex(i) fails.
The above code should become:
        Object[] array = a;
        if (a.length < size())
        {
            array = new Object[size()];
        }
        int count = size();
        for (int i=0;i<count;i++)
        {
            if (itemsByIndex != null && itemsByIndex.containsKey(i))
            {
                array[i] = itemsByIndex.get(i);
            }
            else
            {
                array[i] = retrieveObjectForIndex(i);
            }
        }

If you check the source code of java.util.ArrayList.ctor(Collection c) you will suddenly see the issue.

Obivously if the fix is in the superclass you can drop the workaround in excel store

Andy Jefferson added a comment - 07/May/10 08:19 AM
Yes, but we don't have a testcase for it, and its nothing to do with Embedded support for Excel (IMHO), which is what this issue is. So it is a JIRA on core, with some way of seeing it (e.g testcase for excel).

Guido Anzuoni added a comment - 07/May/10 08:38 AM
Yes, this has nothing to do with excel and embedded support.
For what concerns the testcase, it requires to setup DN test env and, frankly, these days, I have no time for that.
On the other hand, I don't think you have a test case for any private method in any DN class ;-)
You can fix the bug independently from this patch

Andy Jefferson added a comment - 13/May/10 11:48 AM
SVN trunk has this; tested on simple 1-1 embedded PC - requires user to specify the "index" of each column otherwise falls apart (i.e it allows 1 column for a PC field yet the embedded object may have multiple, so doesn't calculate the indexes correctly).

Not applied to SVN branches/2.1 since lack of time and higher priorities on other things.