From a23e84c2ea41defbe09b5eba20354b19319f4ceb Mon Sep 17 00:00:00 2001 From: vga91 Date: Tue, 14 Mar 2023 11:39:35 +0100 Subject: [PATCH] [madSHaLz] APOC triggers aren't updated after a user deletes a database --- .../java/apoc/ApocConfigExtensionFactory.java | 5 +- .../main/java/apoc/ApocExtensionFactory.java | 2 + .../java/apoc/CoreApocGlobalComponents.java | 6 +- .../java/apoc/cypher/CypherInitializer.java | 77 ++++++++++++++++++- .../java/apoc/trigger/TriggerRestartTest.java | 55 +++++++++++++ .../core/TriggerEnterpriseFeaturesTest.java | 10 ++- 6 files changed, 151 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/apoc/ApocConfigExtensionFactory.java b/common/src/main/java/apoc/ApocConfigExtensionFactory.java index 4dd02fe0d..d072ca6c2 100644 --- a/common/src/main/java/apoc/ApocConfigExtensionFactory.java +++ b/common/src/main/java/apoc/ApocConfigExtensionFactory.java @@ -8,6 +8,7 @@ import org.neo4j.kernel.extension.ExtensionType; import org.neo4j.kernel.extension.context.ExtensionContext; import org.neo4j.kernel.lifecycle.Lifecycle; +import org.neo4j.kernel.monitoring.DatabaseEventListeners; import org.neo4j.logging.internal.LogService; /** @@ -22,6 +23,7 @@ public interface Dependencies { Config config(); GlobalProcedures globalProceduresRegistry(); DatabaseManagementService databaseManagementService(); + DatabaseEventListeners databaseEventListeners(); } public ApocConfigExtensionFactory() { @@ -30,7 +32,8 @@ public ApocConfigExtensionFactory() { @Override public Lifecycle newInstance(ExtensionContext context, Dependencies dependencies) { - return new ApocConfig(dependencies.config(), dependencies.log(), dependencies.globalProceduresRegistry(), dependencies.databaseManagementService()); + return new ApocConfig(dependencies.config(), dependencies.log(), dependencies.globalProceduresRegistry(), dependencies.databaseManagementService(), + dependencies.databaseEventListeners()); } } diff --git a/common/src/main/java/apoc/ApocExtensionFactory.java b/common/src/main/java/apoc/ApocExtensionFactory.java index c20a73e49..937a6905e 100644 --- a/common/src/main/java/apoc/ApocExtensionFactory.java +++ b/common/src/main/java/apoc/ApocExtensionFactory.java @@ -12,6 +12,7 @@ import org.neo4j.kernel.internal.GraphDatabaseAPI; import org.neo4j.kernel.lifecycle.Lifecycle; import org.neo4j.kernel.lifecycle.LifecycleAdapter; +import org.neo4j.kernel.monitoring.DatabaseEventListeners; import org.neo4j.logging.Log; import org.neo4j.logging.internal.LogService; import org.neo4j.scheduler.JobScheduler; @@ -43,6 +44,7 @@ public interface Dependencies { AvailabilityGuard availabilityGuard(); DatabaseManagementService databaseManagementService(); ApocConfig apocConfig(); + DatabaseEventListeners databaseEventListeners(); @SuppressWarnings("unused") // used from extended GlobalProcedures globalProceduresRegistry(); RegisterComponentFactory.RegisterComponentLifecycle registerComponentLifecycle(); diff --git a/core/src/main/java/apoc/CoreApocGlobalComponents.java b/core/src/main/java/apoc/CoreApocGlobalComponents.java index 10d29fc06..89def84f3 100644 --- a/core/src/main/java/apoc/CoreApocGlobalComponents.java +++ b/core/src/main/java/apoc/CoreApocGlobalComponents.java @@ -33,6 +33,10 @@ public Collection getContextClasses() { @Override public Iterable getListeners(GraphDatabaseAPI db, ApocExtensionFactory.Dependencies dependencies) { - return Collections.singleton(new CypherInitializer(db, dependencies.log().getUserLog(CypherInitializer.class))); + return Collections.singleton(new CypherInitializer(db, + dependencies.log().getUserLog(CypherInitializer.class), + dependencies.databaseManagementService(), + dependencies.databaseEventListeners()) + ); } } diff --git a/core/src/main/java/apoc/cypher/CypherInitializer.java b/core/src/main/java/apoc/cypher/CypherInitializer.java index 8b6aa915e..ac4ab7fa5 100644 --- a/core/src/main/java/apoc/cypher/CypherInitializer.java +++ b/core/src/main/java/apoc/cypher/CypherInitializer.java @@ -1,6 +1,7 @@ package apoc.cypher; import apoc.ApocConfig; +import apoc.SystemLabels; import apoc.util.Util; import apoc.util.collection.Iterators; import apoc.version.Version; @@ -8,16 +9,27 @@ import org.apache.commons.lang3.StringUtils; import org.neo4j.common.DependencyResolver; import org.neo4j.configuration.GraphDatabaseSettings; +import org.neo4j.dbms.api.DatabaseManagementService; +import org.neo4j.graphdb.Label; +import org.neo4j.graphdb.Node; +import org.neo4j.graphdb.Transaction; +import org.neo4j.graphdb.event.DatabaseEventContext; +import org.neo4j.graphdb.event.DatabaseEventListener; import org.neo4j.kernel.api.procedure.GlobalProcedures; import org.neo4j.kernel.availability.AvailabilityListener; import org.neo4j.kernel.internal.GraphDatabaseAPI; +import org.neo4j.kernel.monitoring.DatabaseEventListeners; import org.neo4j.logging.Log; import java.util.Collection; import java.util.ConcurrentModificationException; +import java.util.List; import java.util.Map; import java.util.TreeMap; +import java.util.function.BiConsumer; + +import static apoc.SystemPropertyKeys.database; public class CypherInitializer implements AvailabilityListener { @@ -25,15 +37,21 @@ public class CypherInitializer implements AvailabilityListener { private final Log userLog; private final GlobalProcedures procs; private final DependencyResolver dependencyResolver; + private final DatabaseManagementService databaseManagementService; + private final DatabaseEventListeners databaseEventListeners; /** * indicates the status of the initializer, to be used for tests to ensure initializer operations are already done */ private volatile boolean finished = false; - public CypherInitializer(GraphDatabaseAPI db, Log userLog) { + public CypherInitializer(GraphDatabaseAPI db, Log userLog, + DatabaseManagementService databaseManagementService, + DatabaseEventListeners databaseEventListeners) { this.db = db; this.userLog = userLog; + this.databaseManagementService = databaseManagementService; + this.databaseEventListeners = databaseEventListeners; this.dependencyResolver = db.getDependencyResolver(); this.procs = dependencyResolver.resolveDependency(GlobalProcedures.class); } @@ -70,6 +88,15 @@ public void available() { } catch (Exception ignored) { userLog.info("Cannot check APOC version compatibility because of a transient error. Retrying your request at a later time may succeed"); } + + // this block is already in system db, so we can directly execute system operation + // for backward compatibility: clear system nodes at start-up if the corresponding database name doesn't exist + // placed here instead of e.g. `ApocConfig` because at this point we are sure that all databases are recovered + cleanUpSystemNodes(); + + // create listener for each database + databaseEventListeners.registerDatabaseEventListener(new SystemFunctionalityListener()); + } Configuration config = dependencyResolver.resolveDependency(ApocConfig.class).getConfig(); @@ -147,4 +174,52 @@ private boolean areProceduresRegistered(String procStart) { public void unavailable() { // intentionally empty } + + private void cleanUpSystemNodes() { + + List databases = databaseManagementService.listDatabases(); + + forEachSystemLabel((tx, label) -> { + tx.findNodes(label).forEachRemaining(node -> { + boolean dbNonExistent = node.hasProperty(database.name()) + && !databases.contains( (String) node.getProperty(database.name()) ); + if (dbNonExistent) { + node.delete(); + } + }); + }); + } + + private class SystemFunctionalityListener implements DatabaseEventListener { + + @Override + public void databaseDrop(DatabaseEventContext eventContext) { + + forEachSystemLabel((tx, label) -> { + tx.findNodes(label, database.name(), eventContext.getDatabaseName()) + .forEachRemaining(Node::delete); + }); + } + + @Override + public void databaseStart(DatabaseEventContext eventContext) {} + + @Override + public void databaseShutdown(DatabaseEventContext eventContext) {} + + @Override + public void databasePanic(DatabaseEventContext eventContext) {} + + @Override + public void databaseCreate(DatabaseEventContext eventContext) {} + } + + private void forEachSystemLabel(BiConsumer consumer) { + try (Transaction tx = db.beginTx()) { + for (Label label: SystemLabels.values()) { + consumer.accept(tx, label); + } + tx.commit(); + } + } } diff --git a/core/src/test/java/apoc/trigger/TriggerRestartTest.java b/core/src/test/java/apoc/trigger/TriggerRestartTest.java index 2931fca2d..a4b987801 100644 --- a/core/src/test/java/apoc/trigger/TriggerRestartTest.java +++ b/core/src/test/java/apoc/trigger/TriggerRestartTest.java @@ -1,7 +1,10 @@ package apoc.trigger; import apoc.ApocConfig; +import apoc.SystemLabels; import apoc.util.TestUtil; +import apoc.util.Util; +import org.apache.commons.lang3.tuple.Pair; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -10,19 +13,28 @@ import org.neo4j.configuration.GraphDatabaseSettings; import org.neo4j.dbms.api.DatabaseManagementService; import org.neo4j.graphdb.GraphDatabaseService; +import org.neo4j.graphdb.Node; +import org.neo4j.graphdb.Transaction; import org.neo4j.test.TestDatabaseManagementServiceBuilder; import java.io.File; import java.io.FileWriter; import java.io.IOException; import java.util.Collections; +import java.util.Iterator; import java.util.Map; +import java.util.function.Consumer; import static apoc.ApocConfig.SUN_JAVA_COMMAND; +import static apoc.SystemLabels.ApocTrigger; +import static apoc.SystemPropertyKeys.database; +import static apoc.SystemPropertyKeys.name; import static apoc.trigger.TriggerTestUtil.TRIGGER_DEFAULT_REFRESH; import static apoc.trigger.TriggerTestUtil.awaitTriggerDiscovered; import static apoc.util.TestUtil.waitDbsAvailable; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class TriggerRestartTest { @@ -62,6 +74,49 @@ private void restartDb() { waitDbsAvailable(db, sysDb); } + @Test + public void deleteSystemNodesAtStartupIfDatabaseDoesNotExist() { + String dbName = "notexistent"; + + // create manually 2 system node, the 1st one in "neo4j" and the other in an not-existent db + withSystemDb(tx -> { + Util.mergeNode(tx, ApocTrigger, null, + Pair.of(database.name(), dbName), + Pair.of(name.name(), "mytest")); + + Util.mergeNode(tx, SystemLabels.ApocTrigger, null, + Pair.of(database.name(), db.databaseName()), + Pair.of(name.name(), "mySecondTest")); + }); + + // check that the nodes have been created successfully + withSystemDb(tx -> { + Iterator dbNotExistentNodes = tx.findNodes(ApocTrigger, database.name(), dbName); + assertTrue( dbNotExistentNodes.hasNext() ); + + Iterator neo4jNodes = tx.findNodes(ApocTrigger, database.name(), db.databaseName()); + assertTrue( neo4jNodes.hasNext() ); + }); + + // check that after a restart only "neo4j" node remains + restartDb(); + + withSystemDb(tx -> { + Iterator dbNotExistentNodes = tx.findNodes(ApocTrigger, database.name(), dbName); + assertFalse( dbNotExistentNodes.hasNext() ); + + Iterator neo4jNodes = tx.findNodes(ApocTrigger, database.name(), db.databaseName()); + assertTrue( neo4jNodes.hasNext() ); + }); + } + + private void withSystemDb(Consumer action) { + try (Transaction tx = sysDb.beginTx()) { + action.accept(tx); + tx.commit(); + } + } + @Test public void testTriggerRunsAfterRestart() { final String query = "CALL apoc.trigger.add('myTrigger', 'unwind $createdNodes as n set n.trigger = n.trigger + 1', {phase:'before'})"; diff --git a/it/src/test/java/apoc/it/core/TriggerEnterpriseFeaturesTest.java b/it/src/test/java/apoc/it/core/TriggerEnterpriseFeaturesTest.java index 47579e89c..0ecb6d5fb 100644 --- a/it/src/test/java/apoc/it/core/TriggerEnterpriseFeaturesTest.java +++ b/it/src/test/java/apoc/it/core/TriggerEnterpriseFeaturesTest.java @@ -18,12 +18,14 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Stream; +import static apoc.ApocConfig.APOC_CONFIG_INITIALIZER; import static apoc.ApocConfig.APOC_TRIGGER_ENABLED; import static apoc.trigger.TriggerHandler.TRIGGER_REFRESH; import static apoc.trigger.TriggerTestUtil.TIMEOUT; import static apoc.trigger.TriggerTestUtil.TRIGGER_DEFAULT_REFRESH; import static apoc.util.TestContainerUtil.createEnterpriseDB; import static apoc.util.TestContainerUtil.testCall; +import static apoc.util.TestContainerUtil.testCallEmpty; import static apoc.util.TestContainerUtil.testResult; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -37,6 +39,7 @@ public class TriggerEnterpriseFeaturesTest { private static final String FOO_DB = "foo"; + private static final String INIT_DB = "initdb"; private static final String NO_ADMIN_USER = "nonadmin"; private static final String NO_ADMIN_PWD = "test1234"; @@ -46,10 +49,15 @@ public class TriggerEnterpriseFeaturesTest { @BeforeClass public static void beforeAll() { + final String cypherInitializer = String.format("%s.%s.0", + APOC_CONFIG_INITIALIZER, SYSTEM_DATABASE_NAME); + final String createInitDb = String.format("CREATE DATABASE %s IF NOT EXISTS", INIT_DB); + // We build the project, the artifact will be placed into ./build/libs neo4jContainer = createEnterpriseDB(List.of(TestContainerUtil.ApocPackage.CORE), true) .withEnv(APOC_TRIGGER_ENABLED, "true") - .withEnv(TRIGGER_REFRESH, String.valueOf(TRIGGER_DEFAULT_REFRESH)); + .withEnv(TRIGGER_REFRESH, String.valueOf(TRIGGER_DEFAULT_REFRESH)) + .withEnv(cypherInitializer, createInitDb); neo4jContainer.start(); session = neo4jContainer.getSession();