Description Steve Neely 2013-07-17 17:52:19 EDT

In v2.4.2 of EclipseLink the class org.eclipse.persistence.mappings.DirectMapMapping was refactored. It looks like an if-guard was missed in the refactoring.

Looking at method compareForChange in DirectMapMapping in v2.4.0:

    public ChangeRecord compareForChange(Object clone, Object backUp, ObjectChangeSet owner, AbstractSession session) {
	
	// [SNIP]
    
        Map backUpCollection = null;
        if (!owner.isNew()) {
            backUpAttribute = getAttributeValueFromObject(backUp);
            if ((backUpAttribute == null) && (cloneAttribute == null)) {
                return null;
            }
            backUpCollection = (Map)getRealCollectionAttributeValueFromObject(backUp, session);
            Object backUpIter = containerPolicy.iteratorFor(backUpCollection);
            while (containerPolicy.hasNext(backUpIter)) {// Make a lookup of the objects
                Map.Entry entry = (Map.Entry)containerPolicy.nextEntry(backUpIter, session);
                originalKeyValues.put(entry.getKey(), backUpCollection.get(entry.getKey()));
            }
        }
     	// [SNIP]
    }


The code "if (!owner.isNew())" was checking the call to this line:

	Object backUpIter = containerPolicy.iteratorFor(backUpCollection);

This is important because if the OwnerChangeSet is new there will be no backupCollection (it'll be null). Downstream this would cause an NPE.

----------------------------------

Looking at same method (compareForChange in DirectMapMapping) in v2.4.2:

    public ChangeRecord compareForChange(Object clone, Object backUp, ObjectChangeSet owner, AbstractSession session) {

    	// [SNIP]

        Map backUpCollection = null;
        
        if (!owner.isNew()) {
            backUpAttribute = getAttributeValueFromObject(backUp);
            if ((backUpAttribute == null) && (cloneAttribute == null)) {
                return null;
            }
            backUpCollection = (Map)getRealCollectionAttributeValueFromObject(backUp, session);
        }

        DirectMapChangeRecord changeRecord = new DirectMapChangeRecord(owner);
        changeRecord.setAttribute(getAttributeName());
        changeRecord.setMapping(this);
        compareCollectionsForChange(backUpCollection, cloneObjectCollection, changeRecord, session);

    	// [SNIP]
    }


    public void compareCollectionsForChange(Object oldCollection, Object newCollection, ChangeRecord changeRecord, AbstractSession session) {      
        HashMap originalKeyValues = new HashMap(10);
        HashMap cloneKeyValues = new HashMap(10);

        Map backUpCollection = (Map)oldCollection;
        Object backUpIter = containerPolicy.iteratorFor(backUpCollection);
        while (containerPolicy.hasNext(backUpIter)) {// Make a lookup of the objects
            Map.Entry entry = (Map.Entry)containerPolicy.nextEntry(backUpIter, session);
            originalKeyValues.put(entry.getKey(), backUpCollection.get(entry.getKey()));
        }
    	// [SNIP]
    }

The "compareCollectionsForChange" method was introduced in v2.4.2 to abstract the while-loops. The new method contains the following line of code that is *not* guarded:

        Object backUpIter = containerPolicy.iteratorFor(backUpCollection);

If you follow the code from "iteratorFor(backupCollection)" call, it'll null point. The backupCollection value is null and the MapContainerPolicyIterator has that null passed into it as "container":

        public MapContainerPolicyIterator(Map container) {
    	     this.iterator = container.entrySet().iterator();
	}