Skip to content

Commit

Permalink
[bugfix] Collection Triggers were called twice when moving a collection.
Browse files Browse the repository at this point in the history
  • Loading branch information
adamretter committed Oct 4, 2014
1 parent 2bf6d63 commit 421239d
Show file tree
Hide file tree
Showing 3 changed files with 290 additions and 5 deletions.
27 changes: 22 additions & 5 deletions src/org/exist/storage/NativeBroker.java
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,7 @@ public void moveCollection(final Txn transaction, final Collection collection, f
final File fsSourceDir = getCollectionFile(fsDir, collection.getURI(), false);

// Need to move each collection in the source tree individually, so recurse.
moveCollectionRecursive(transaction, trigger, collection, destination, newName);
moveCollectionRecursive(transaction, trigger, collection, destination, newName, false);

// For binary resources, though, just move the top level directory and all descendants come with it.
moveBinaryFork(transaction, fsSourceDir, destination, newName);
Expand Down Expand Up @@ -1271,7 +1271,20 @@ private void moveBinaryFork(final Txn transaction, final File sourceDir, final C
}
}

private void moveCollectionRecursive(final Txn transaction, final CollectionTrigger trigger, final Collection collection, final Collection destination, final XmldbURI newName) throws PermissionDeniedException, IOException, LockException, TriggerException {
//TODO bug the trigger param is reused as this is a recursive method, but in the current design triggers
// are only meant to be called once for each action and then destroyed!
/**
* @param transaction
* @param trigger
* @param collection
* @param destination
* @param newName
* @param fireTrigger Indicates whether the CollectionTrigger should be fired
* on the collection the first time this function is called.
* Triggers will always be fired for recursive calls of this
* function.
*/
private void moveCollectionRecursive(final Txn transaction, final CollectionTrigger trigger, final Collection collection, final Collection destination, final XmldbURI newName, final boolean fireTrigger) throws PermissionDeniedException, IOException, LockException, TriggerException {

final XmldbURI uri = collection.getURI();
final CollectionCache collectionsCache = pool.getCollectionsCache();
Expand All @@ -1280,7 +1293,9 @@ private void moveCollectionRecursive(final Txn transaction, final CollectionTrig
final XmldbURI srcURI = collection.getURI();
final XmldbURI dstURI = destination.getURI().append(newName);

trigger.beforeMoveCollection(this, transaction, collection, dstURI);
if(fireTrigger) {
trigger.beforeMoveCollection(this, transaction, collection, dstURI);
}

final XmldbURI parentName = collection.getParentURI();
final Collection parent = openCollection(parentName, Lock.WRITE_LOCK);
Expand Down Expand Up @@ -1317,7 +1332,9 @@ private void moveCollectionRecursive(final Txn transaction, final CollectionTrig
lock.release(Lock.WRITE_LOCK);
}

trigger.afterMoveCollection(this, transaction, collection, srcURI);
if(fireTrigger) {
trigger.afterMoveCollection(this, transaction, collection, srcURI);
}

for(final Iterator<XmldbURI> i = collection.collectionIterator(this); i.hasNext(); ) {
final XmldbURI childName = i.next();
Expand All @@ -1327,7 +1344,7 @@ private void moveCollectionRecursive(final Txn transaction, final CollectionTrig
LOG.warn("Child collection " + childName + " not found");
} else {
try {
moveCollectionRecursive(transaction, trigger, child, collection, childName);
moveCollectionRecursive(transaction, trigger, child, collection, childName, true);
} finally {
child.release(Lock.WRITE_LOCK);
}
Expand Down
113 changes: 113 additions & 0 deletions test/src/org/exist/collections/triggers/CollectionTriggerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package org.exist.collections.triggers;

import org.exist.EXistException;
import org.exist.TestUtils;
import org.exist.security.PermissionDeniedException;
import org.xmldb.api.base.Collection;
import org.exist.xmldb.CollectionManagementServiceImpl;
import org.exist.xmldb.DatabaseInstanceManager;
import org.exist.xmldb.IndexQueryService;
import org.exist.xmldb.XmldbURI;
import org.junit.*;
import org.xmldb.api.DatabaseManager;
import org.xmldb.api.base.Database;
import org.xmldb.api.base.XMLDBException;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.fail;


public class CollectionTriggerTest {

private final static String TEST_COLLECTION = "testCollectionTrigger";
private static Collection root;
private static Collection testCollection;
private static CollectionManagementServiceImpl rootSrv;


@Test
public void move() throws XMLDBException, EXistException, PermissionDeniedException {

//create /db/testCollectionTrigger/srcCollection
final CollectionManagementServiceImpl colMgmtSrv = (CollectionManagementServiceImpl)testCollection.getService("CollectionManagementService", "1.0");
final Collection srcCollection = colMgmtSrv.createCollection("col1");

final XmldbURI baseUri = XmldbURI.create(testCollection.getName());
final XmldbURI srcUri = XmldbURI.create(srcCollection.getName());
final XmldbURI newDest = XmldbURI.create("moved");

//perform the move
colMgmtSrv.move(srcUri, baseUri, newDest);


//get the trigger and check its count
CountingCollectionTrigger.CountingCollectionTriggerState triggerState = CountingCollectionTrigger.CountingCollectionTriggerState.getInstance();

//trigger move methods should have only been
//invoked once as we only moved one resource
assertEquals(1, triggerState.getBeforeMove());
assertEquals(1, triggerState.getAfterMove());
}

@Before
public void createTestCollection() throws XMLDBException {
//create a test collection
testCollection = rootSrv.createCollection(TEST_COLLECTION);

// configure the test collection with the trigger
IndexQueryService idxConf = (IndexQueryService)testCollection.getService("IndexQueryService", "1.0");
idxConf.configureCollection(COLLECTION_CONFIG);
}

@After
public void removeTestCollection() throws XMLDBException {
rootSrv.removeCollection(XmldbURI.create(testCollection.getName()));
}

/** just start the DB and create the test collection */
@BeforeClass
public static void startDB() {
try {
// initialize driver
Class<?> cl = Class.forName("org.exist.xmldb.DatabaseImpl");
Database database = (Database) cl.newInstance();
database.setProperty("create-database", "true");
DatabaseManager.registerDatabase(database);

root = DatabaseManager.getCollection(XmldbURI.LOCAL_DB, "admin", "");
assertNotNull(root);

rootSrv = (CollectionManagementServiceImpl)root.getService("CollectionManagementService", "1.0");
} catch (ClassNotFoundException e) {
fail(e.getMessage());
} catch (InstantiationException e) {
fail(e.getMessage());
} catch (IllegalAccessException e) {
fail(e.getMessage());
} catch (XMLDBException e) {
e.printStackTrace();
fail(e.getMessage());
}
}

@AfterClass
public static void shutdownDB() {
TestUtils.cleanupDB();
try {
org.xmldb.api.base.Collection root = DatabaseManager.getCollection("xmldb:exist://" + XmldbURI.ROOT_COLLECTION, "admin", "");
DatabaseInstanceManager mgr = (DatabaseInstanceManager) root.getService("DatabaseInstanceManager", "1.0");
mgr.shutdown();
} catch (XMLDBException e) {
e.printStackTrace();
fail(e.getMessage());
}
testCollection = null;
}

private final static String COLLECTION_CONFIG =
"<exist:collection xmlns:exist='http://exist-db.org/collection-config/1.0'>" +
" <exist:triggers>" +
" <exist:trigger class='org.exist.collections.triggers.CountingCollectionTrigger'/>" +
" </exist:triggers>" +
"</exist:collection>";
}
155 changes: 155 additions & 0 deletions test/src/org/exist/collections/triggers/CountingCollectionTrigger.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package org.exist.collections.triggers;

import org.exist.storage.DBBroker;
import org.exist.storage.txn.Txn;
import org.exist.xmldb.XmldbURI;

import java.util.List;
import java.util.Map;

public class CountingCollectionTrigger implements CollectionTrigger {

final CountingCollectionTriggerState state = CountingCollectionTriggerState.getInstance();

@Override
public void configure(DBBroker broker, org.exist.collections.Collection parent, Map<String, List<? extends Object>> parameters) throws TriggerException {
state.incConfigure();
}

@Override
public void beforeCreateCollection(DBBroker broker, Txn txn, XmldbURI uri) throws TriggerException {
state.incBeforeCreate();
}

@Override
public void afterCreateCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection) throws TriggerException {
state.incAfterCreate();
}

@Override
public void beforeCopyCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection, XmldbURI newUri) throws TriggerException {
state.incBeforeCopy();
}

@Override
public void afterCopyCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection, XmldbURI oldUri) throws TriggerException {
state.incAfterCopy();
}

@Override
public void beforeMoveCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection, XmldbURI newUri) throws TriggerException {
state.incBeforeMove();
}

@Override
public void afterMoveCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection, XmldbURI oldUri) throws TriggerException {
state.incAfterMove();
}

@Override
public void beforeDeleteCollection(DBBroker broker, Txn txn, org.exist.collections.Collection collection) throws TriggerException {
state.incBeforeDelete();
}

@Override
public void afterDeleteCollection(DBBroker broker, Txn txn, XmldbURI uri) throws TriggerException {
state.incAfterDelete();
}

//this evil thing is here so that we can share state between the trigger and the test
//we really should re-design triggers to make them easily testable!
//...all I am going to say is that I am not redesigning triggers again right now
//and that `they made me do it`!
public final static class CountingCollectionTriggerState {
private int configure = 0;
private int beforeCreate = 0;
private int afterCreate = 0;
private int beforeCopy = 0;
private int afterCopy = 0;
private int beforeMove = 0;
private int afterMove = 0;
private int beforeDelete = 0;
private int afterDelete = 0;

private final static CountingCollectionTriggerState instance = new CountingCollectionTriggerState();

private CountingCollectionTriggerState() {
}

public final static CountingCollectionTriggerState getInstance() {
return instance;
}

public int getConfigure() {
return configure;
}

public void incConfigure() {
configure++;
}

public int getBeforeCreate() {
return beforeCreate;
}

public void incBeforeCreate() {
beforeCreate++;
}

public int getAfterCreate() {
return afterCreate;
}

public void incAfterCreate() {
afterCreate++;
}

public int getBeforeCopy() {
return beforeCopy;
}

public void incBeforeCopy() {
beforeCopy++;
}

public int getAfterCopy() {
return afterCopy;
}

public void incAfterCopy() {
afterCopy++;
}

public int getBeforeMove() {
return beforeMove;
}

public void incBeforeMove() {
beforeMove++;
}

public int getAfterMove() {
return afterMove;
}

public void incAfterMove() {
afterMove++;
}

public int getBeforeDelete() {
return beforeDelete;
}

public void incBeforeDelete() {
beforeDelete++;
}

public int getAfterDelete() {
return afterDelete;
}

public void incAfterDelete() {
afterDelete++;
}
}
}

0 comments on commit 421239d

Please sign in to comment.