From 8a77c05b9914b63f8bced86e9e3ea31dab099811 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Thu, 19 Feb 2009 22:02:14 +0000 Subject: [PATCH] Fix for bug introduced in r745976. EscherContainerRecord shouldn't hand out it's private 'child records' field. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@746018 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/ddf/EscherContainerRecord.java | 112 +++++++++--------- .../org/apache/poi/hslf/model/MovieShape.java | 2 +- .../poi/ddf/TestEscherContainerRecord.java | 57 +++++++-- 3 files changed, 103 insertions(+), 68 deletions(-) diff --git a/src/java/org/apache/poi/ddf/EscherContainerRecord.java b/src/java/org/apache/poi/ddf/EscherContainerRecord.java index fe78fbd65..f1785304b 100644 --- a/src/java/org/apache/poi/ddf/EscherContainerRecord.java +++ b/src/java/org/apache/poi/ddf/EscherContainerRecord.java @@ -34,8 +34,7 @@ import java.io.PrintWriter; * * @author Glen Stampoultzis */ -public class EscherContainerRecord extends EscherRecord -{ +public final class EscherContainerRecord extends EscherRecord { public static final short DGG_CONTAINER = (short)0xF000; public static final short BSTORE_CONTAINER = (short)0xF001; public static final short DG_CONTAINER = (short)0xF002; @@ -57,7 +56,7 @@ public class EscherContainerRecord extends EscherRecord bytesWritten += childBytesWritten; offset += childBytesWritten; bytesRemaining -= childBytesWritten; - getChildRecords().add( child ); + addChildRecord(child); if (offset >= data.length && bytesRemaining > 0) { System.out.println("WARNING: " + bytesRemaining + " bytes remaining but no space left"); @@ -73,16 +72,16 @@ public class EscherContainerRecord extends EscherRecord LittleEndian.putShort(data, offset, getOptions()); LittleEndian.putShort(data, offset+2, getRecordId()); int remainingBytes = 0; - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) - { - EscherRecord r = (EscherRecord) iterator.next(); + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); remainingBytes += r.getRecordSize(); } LittleEndian.putInt(data, offset+4, remainingBytes); int pos = offset+8; - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) - { - EscherRecord r = (EscherRecord) iterator.next(); + iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); pos += r.serialize(pos, data, listener ); } @@ -90,12 +89,11 @@ public class EscherContainerRecord extends EscherRecord return pos - offset; } - public int getRecordSize() - { + public int getRecordSize() { int childRecordsSize = 0; - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) - { - EscherRecord r = (EscherRecord) iterator.next(); + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); childRecordsSize += r.getRecordSize(); } return 8 + childRecordsSize; @@ -106,46 +104,51 @@ public class EscherContainerRecord extends EscherRecord * given recordId? */ public boolean hasChildOfType(short recordId) { - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) - { - EscherRecord r = (EscherRecord) iterator.next(); + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); if(r.getRecordId() == recordId) { return true; } } return false; } - + /** - * Returns a list of all the child (escher) records - * of the container. + * @return a copy of the list of all the child records of the container. */ public List getChildRecords() { - return _childRecords; + return new ArrayList(_childRecords); } - + /** + * replaces the internal child list with the contents of the supplied childRecords + */ + public void setChildRecords(List childRecords) { + if (childRecords == _childRecords) { + throw new IllegalStateException("Child records private data member has escaped"); + } + _childRecords.clear(); + _childRecords.addAll(childRecords); + } + + /** * Returns all of our children which are also * EscherContainers (may be 0, 1, or vary rarely * 2 or 3) */ - public List getChildContainers() { - List containers = new ArrayList(); - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) - { - EscherRecord r = (EscherRecord) iterator.next(); + public List getChildContainers() { + List containers = new ArrayList(); + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); if(r instanceof EscherContainerRecord) { - containers.add(r); + containers.add((EscherContainerRecord) r); } } return containers; } - public void setChildRecords(List childRecords) { - _childRecords.clear(); - _childRecords.addAll(childRecords); - } - public String getRecordName() { switch (getRecordId()) { case DGG_CONTAINER: @@ -165,13 +168,12 @@ public class EscherContainerRecord extends EscherRecord } } - public void display( PrintWriter w, int indent ) - { - super.display( w, indent ); - for (Iterator iterator = _childRecords.iterator(); iterator.hasNext();) + public void display(PrintWriter w, int indent) { + super.display(w, indent); + for (Iterator iterator = _childRecords.iterator(); iterator.hasNext();) { - EscherRecord escherRecord = (EscherRecord) iterator.next(); - escherRecord.display( w, indent + 1 ); + EscherRecord escherRecord = iterator.next(); + escherRecord.display(w, indent + 1); } } @@ -188,16 +190,15 @@ public class EscherContainerRecord extends EscherRecord String nl = System.getProperty( "line.separator" ); StringBuffer children = new StringBuffer(); - if ( getChildRecords().size() > 0 ) - { + if (_childRecords.size() > 0) { children.append( " children: " + nl ); int count = 0; - for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); ) + for ( Iterator iterator = _childRecords.iterator(); iterator.hasNext(); ) { String newIndent = indent + " "; - EscherRecord record = (EscherRecord) iterator.next(); + EscherRecord record = iterator.next(); children.append(newIndent + "Child " + count + ":" + nl); if(record instanceof EscherContainerRecord) { @@ -215,18 +216,17 @@ public class EscherContainerRecord extends EscherRecord indent + " isContainer: " + isContainerRecord() + nl + indent + " options: 0x" + HexDump.toHex( getOptions() ) + nl + indent + " recordId: 0x" + HexDump.toHex( getRecordId() ) + nl + - indent + " numchildren: " + getChildRecords().size() + nl + + indent + " numchildren: " + _childRecords.size() + nl + indent + children.toString(); } - public EscherSpRecord getChildById( short recordId ) - { - for ( Iterator iterator = _childRecords.iterator(); iterator.hasNext(); ) - { - EscherRecord escherRecord = (EscherRecord) iterator.next(); - if (escherRecord.getRecordId() == recordId) - return (EscherSpRecord) escherRecord; + public EscherSpRecord getChildById(short recordId) { + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); + if (r.getRecordId() == recordId) + return (EscherSpRecord) r; } return null; } @@ -236,15 +236,15 @@ public class EscherContainerRecord extends EscherRecord * * @param out - list to store found records */ - public void getRecordsById(short recordId, List out){ - for(Iterator it = _childRecords.iterator(); it.hasNext();) { - Object er = it.next(); - EscherRecord r = (EscherRecord)er; + public void getRecordsById(short recordId, List out){ + Iterator iterator = _childRecords.iterator(); + while (iterator.hasNext()) { + EscherRecord r = iterator.next(); if(r instanceof EscherContainerRecord) { EscherContainerRecord c = (EscherContainerRecord)r; c.getRecordsById(recordId, out ); } else if (r.getRecordId() == recordId){ - out.add(er); + out.add(r); } } } diff --git a/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java b/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java index 152433d72..ff4cac0a5 100755 --- a/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java +++ b/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java @@ -83,7 +83,7 @@ public final class MovieShape extends Picture { EscherClientDataRecord cldata = new EscherClientDataRecord(); cldata.setOptions((short)0xF); - _escherContainer.getChildRecords().add(cldata); + _escherContainer.addChildRecord(cldata); OEShapeAtom oe = new OEShapeAtom(); InteractiveInfo info = new InteractiveInfo(); diff --git a/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java b/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java index 490a3d50c..8d12b1c51 100644 --- a/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java +++ b/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java @@ -1,4 +1,3 @@ - /* ==================================================================== Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with @@ -20,14 +19,17 @@ package org.apache.poi.ddf; import java.io.File; import java.io.FileInputStream; +import java.util.List; import junit.framework.TestCase; import org.apache.poi.util.HexRead; import org.apache.poi.util.HexDump; import org.apache.poi.util.IOUtils; -public class TestEscherContainerRecord extends TestCase -{ +/** + * Tests for {@link EscherContainerRecord} + */ +public final class TestEscherContainerRecord extends TestCase { private String ESCHER_DATA_PATH; protected void setUp() { @@ -129,17 +131,18 @@ public class TestEscherContainerRecord extends TestCase " properties:" + nl; assertEquals( expected, r.toString() ); } + + private static final class DummyEscherRecord extends EscherRecord { + public DummyEscherRecord() { } + public int fillFields( byte[] data, int offset, EscherRecordFactory recordFactory ) { return 0; } + public int serialize( int offset, byte[] data, EscherSerializationListener listener ) { return 0; } + public int getRecordSize() { return 10; } + public String getRecordName() { return ""; } + } public void testGetRecordSize() { EscherContainerRecord r = new EscherContainerRecord(); - r.addChildRecord(new EscherRecord() - { - public int fillFields( byte[] data, int offset, EscherRecordFactory recordFactory ) { return 0; } - public int serialize( int offset, byte[] data, EscherSerializationListener listener ) { return 0; } - public int getRecordSize() { return 10; } - public String getRecordName() { return ""; } - } ); - + r.addChildRecord(new DummyEscherRecord()); assertEquals(18, r.getRecordSize()); } @@ -158,4 +161,36 @@ public class TestEscherContainerRecord extends TestCase EscherContainerRecord record = new EscherContainerRecord(); record.fillFields(data, 0, new DefaultEscherRecordFactory()); } + + /** + * Ensure {@link EscherContainerRecord} doesn't spill its guts everywhere + */ + public void testChildren() { + EscherContainerRecord ecr = new EscherContainerRecord(); + List children0 = ecr.getChildRecords(); + assertEquals(0, children0.size()); + + EscherRecord chA = new DummyEscherRecord(); + EscherRecord chB = new DummyEscherRecord(); + EscherRecord chC = new DummyEscherRecord(); + + ecr.addChildRecord(chA); + ecr.addChildRecord(chB); + children0.add(chC); + + List children1 = ecr.getChildRecords(); + assertTrue(children0 != children1); + assertEquals(2, children1.size()); + assertEquals(chA, children1.get(0)); + assertEquals(chB, children1.get(1)); + + assertEquals(1, children0.size()); // first copy unchanged + + ecr.setChildRecords(children0); + ecr.addChildRecord(chA); + List children2 = ecr.getChildRecords(); + assertEquals(2, children2.size()); + assertEquals(chC, children2.get(0)); + assertEquals(chA, children2.get(1)); + } }