Skip to content

Commit

Permalink
Better error message when deleting a node with relationships.
Browse files Browse the repository at this point in the history
It took.. a while. But here is a better error message whenever a node
with relationships gets deleted.

Resolves #1903
  • Loading branch information
jakewins committed Dec 16, 2015
1 parent 666816f commit 7483d39
Show file tree
Hide file tree
Showing 11 changed files with 155 additions and 99 deletions.
Expand Up @@ -240,7 +240,7 @@ public void shouldHandleImplicitCommitFailure() throws Throwable
assertThat( responses.next(), success() ); assertThat( responses.next(), success() );


// But the stop should have failed, since it implicitly triggers commit and thus triggers a failure // But the stop should have failed, since it implicitly triggers commit and thus triggers a failure
assertThat( discarding.next(), failedWith( Status.Transaction.ValidationFailed ) ); assertThat( discarding.next(), failedWith( Status.Schema.ConstraintViolation ) );
} }


@Test @Test
Expand Down
Expand Up @@ -19,7 +19,7 @@
*/ */
package org.neo4j.internal.cypher.acceptance package org.neo4j.internal.cypher.acceptance


import org.neo4j.cypher.{NodeStillHasRelationshipsException, NewPlannerTestSupport, ExecutionEngineFunSuite, QueryStatisticsTestSupport} import org.neo4j.cypher._


class DeleteAcceptanceTest extends ExecutionEngineFunSuite with QueryStatisticsTestSupport with NewPlannerTestSupport { class DeleteAcceptanceTest extends ExecutionEngineFunSuite with QueryStatisticsTestSupport with NewPlannerTestSupport {
test("should be able to delete nodes") { test("should be able to delete nodes") {
Expand Down Expand Up @@ -61,7 +61,7 @@ class DeleteAcceptanceTest extends ExecutionEngineFunSuite with QueryStatisticsT
relate(x, createNode()) relate(x, createNode())
relate(x, createNode()) relate(x, createNode())


a [NodeStillHasRelationshipsException] should be thrownBy a [ConstraintValidationException] should be thrownBy
executeWithCostPlannerOnly(s"match (n:X) delete n") executeWithCostPlannerOnly(s"match (n:X) delete n")
} }


Expand Down
Expand Up @@ -91,29 +91,8 @@ class ClosingIterator(inner: Iterator[collection.Map[String, Any]],
close(success = true) close(success = true)
} }


def close(success: Boolean) = translateException { def close(success: Boolean) = decoratedCypherException({
closer.close(success) closer.close(success)
}

private def translateException[U](f: => U): U = decoratedCypherException({
try {
f
} catch {
case e: TransactionFailureException =>
e.getCause match {
case exception: org.neo4j.kernel.api.exceptions.TransactionFailureException =>
val status = exception.status()
if (status == Status.Transaction.ValidationFailed) {
exception.getMessage match {
case still_has_relationships(id) => throw new NodeStillHasRelationshipsException(id.toLong, e)
case _ => throw e
}
}
case _ =>
}

throw e
}
}) })


private def failIfThrows[U](f: => U): U = decoratedCypherException({ private def failIfThrows[U](f: => U): U = decoratedCypherException({
Expand Down
Expand Up @@ -180,7 +180,7 @@ class MutatingIntegrationTest extends ExecutionEngineFunSuite with Assertions wi
relate(a, b, "HATES") relate(a, b, "HATES")
relate(a, b, "LOVES") relate(a, b, "LOVES")


intercept[NodeStillHasRelationshipsException](executeWithRulePlanner("match (n) where id(n) = 0 match (n)-[r:HATES]->() delete n,r")) intercept[ConstraintValidationException](executeWithRulePlanner("match (n) where id(n) = 0 match (n)-[r:HATES]->() delete n,r"))
} }


test("delete and return") { test("delete and return") {
Expand Down
Expand Up @@ -27,7 +27,6 @@
import org.neo4j.graphdb.TransientFailureException; import org.neo4j.graphdb.TransientFailureException;
import org.neo4j.graphdb.TransientTransactionFailureException; import org.neo4j.graphdb.TransientTransactionFailureException;
import org.neo4j.kernel.api.KernelTransaction; import org.neo4j.kernel.api.KernelTransaction;
import org.neo4j.kernel.api.KernelTransaction.CloseListener;
import org.neo4j.kernel.api.exceptions.ConstraintViolationTransactionFailureException; import org.neo4j.kernel.api.exceptions.ConstraintViolationTransactionFailureException;
import org.neo4j.kernel.api.exceptions.KernelException; import org.neo4j.kernel.api.exceptions.KernelException;
import org.neo4j.kernel.api.exceptions.Status.Classification; import org.neo4j.kernel.api.exceptions.Status.Classification;
Expand All @@ -51,14 +50,7 @@ public TopLevelTransaction( KernelTransaction transaction,
{ {
this.transaction = transaction; this.transaction = transaction;
this.stmtProvider = stmtProvider; this.stmtProvider = stmtProvider;
this.transaction.registerCloseListener( new CloseListener() this.transaction.registerCloseListener( success -> stmtProvider.unbindTransactionFromCurrentThread() );
{
@Override
public void notify( boolean success )
{
stmtProvider.unbindTransactionFromCurrentThread();
}
} );
} }


@Override @Override
Expand Down
Expand Up @@ -30,4 +30,9 @@ public ConstraintViolationTransactionFailureException( String msg, KernelExcepti
{ {
super( Status.Schema.ConstraintViolation, msg, cause ); super( Status.Schema.ConstraintViolation, msg, cause );
} }

public ConstraintViolationTransactionFailureException( String msg )
{
this( msg, null );
}
} }
Expand Up @@ -19,6 +19,7 @@
*/ */
package org.neo4j.kernel.impl.transaction.state; package org.neo4j.kernel.impl.transaction.state;


import org.neo4j.kernel.api.exceptions.ConstraintViolationTransactionFailureException;
import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.api.exceptions.TransactionFailureException;
import org.neo4j.kernel.api.exceptions.index.IndexNotFoundKernelException; import org.neo4j.kernel.api.exceptions.index.IndexNotFoundKernelException;
Expand Down Expand Up @@ -49,8 +50,9 @@ public void validateNodeRecord( NodeRecord record ) throws TransactionFailureExc
{ {
if ( !record.inUse() && record.getNextRel() != Record.NO_NEXT_RELATIONSHIP.intValue() ) if ( !record.inUse() && record.getNextRel() != Record.NO_NEXT_RELATIONSHIP.intValue() )
{ {
throw new TransactionFailureException( Status.Transaction.ValidationFailed, throw new ConstraintViolationTransactionFailureException(
"Node record " + record + " still has relationships" ); "Cannot delete node<" + record.getId() + ">, because it still has relationships. " +
"To delete this node, you must first delete its relationships." );
} }
} }


Expand Down
@@ -0,0 +1,64 @@
/*
* Copyright (c) 2002-2015 "Neo Technology,"
* Network Engine for Objects in Lund AB [http://neotechnology.com]
*
* This file is part of Neo4j.
*
* Neo4j is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package org.neo4j.graphdb;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import org.neo4j.test.ImpermanentDatabaseRule;

public class DeleteNodeWithRelsIT
{
@Rule
public ImpermanentDatabaseRule db = new ImpermanentDatabaseRule();

@Rule
public ExpectedException exception = ExpectedException.none();

@Test
public void shouldGiveHelpfulExceptionWhenDeletingNodeWithRels() throws Exception
{
// Given
GraphDatabaseService db = this.db.getGraphDatabaseService();

Node node;
try( Transaction tx = db.beginTx() )
{
node = db.createNode();
node.createRelationshipTo( db.createNode(), DynamicRelationshipType.withName( "MAYOR_OF" ) );
tx.success();
}

// And given a transaction deleting just the node
Transaction tx = db.beginTx();
node.delete();
tx.success();

// Expect
exception.expect( ConstraintViolationException.class );
exception.expectMessage( "Cannot delete node<"+node.getId()+">, because it still has relationships. " +
"To delete this node, you must first delete its relationships." );

// When I commit
tx.close();
}

}
Expand Up @@ -19,23 +19,23 @@
*/ */
package org.neo4j.kernel.impl.core; package org.neo4j.kernel.impl.core;


import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import java.lang.Thread.State; import java.lang.Thread.State;
import java.util.Iterator; import java.util.Iterator;
import java.util.Map; import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicBoolean;


import org.junit.Assert; import org.neo4j.graphdb.ConstraintViolationException;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;

import org.neo4j.graphdb.GraphDatabaseService; import org.neo4j.graphdb.GraphDatabaseService;
import org.neo4j.graphdb.Node; import org.neo4j.graphdb.Node;
import org.neo4j.graphdb.NotFoundException; import org.neo4j.graphdb.NotFoundException;
import org.neo4j.graphdb.PropertyContainer; import org.neo4j.graphdb.PropertyContainer;
import org.neo4j.graphdb.RelationshipType; import org.neo4j.graphdb.RelationshipType;
import org.neo4j.graphdb.Transaction; import org.neo4j.graphdb.Transaction;
import org.neo4j.graphdb.TransactionFailureException;
import org.neo4j.test.DatabaseRule; import org.neo4j.test.DatabaseRule;
import org.neo4j.test.GraphTransactionRule; import org.neo4j.test.GraphTransactionRule;
import org.neo4j.test.ImpermanentDatabaseRule; import org.neo4j.test.ImpermanentDatabaseRule;
Expand All @@ -44,7 +44,6 @@
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;

import static org.neo4j.helpers.Exceptions.launderedException; import static org.neo4j.helpers.Exceptions.launderedException;


public class NodeTest public class NodeTest
Expand All @@ -55,31 +54,31 @@ public class NodeTest
@Rule @Rule
public GraphTransactionRule tx = new GraphTransactionRule( db ); public GraphTransactionRule tx = new GraphTransactionRule( db );


@Rule
public ExpectedException exception = ExpectedException.none();

@Test @Test
public void givenNodeWithRelationshipWhenDeleteNodeThenThrowExceptionOnCommit() throws Exception public void shouldGiveHelpfulExceptionWhenDeletingNodeWithRels() throws Exception
{ {
// Given // Given
Node node1 = getGraphDb().createNode(); Node node;
Node node2 = getGraphDb().createNode();
node1.createRelationshipTo( node2, RelationshipType.withName( "KNOWS" ) );


node = db.createNode();
Node node2 = db.createNode();
node.createRelationshipTo( node2, RelationshipType.withName( "MAYOR_OF" ) );
tx.success(); tx.success();


// When // And given a transaction deleting just the node
tx.begin(); tx.begin();
node1.delete(); node.delete();


try // Expect
{ exception.expect( ConstraintViolationException.class );
tx.success(); exception.expectMessage( "Cannot delete node<"+node.getId()+">, because it still has relationships. " +
"To delete this node, you must first delete its relationships." );


// Then // When I commit
Assert.fail(); tx.success();
}
catch ( TransactionFailureException e )
{
Assert.assertNotEquals( e.getCause().getMessage().indexOf( "still has relationships" ), -1 );
}
} }


@Test @Test
Expand Down
Expand Up @@ -19,31 +19,37 @@
*/ */
package org.neo4j.kernel.impl.core; package org.neo4j.kernel.impl.core;


import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import java.util.Arrays; import java.util.Arrays;
import java.util.HashSet; import java.util.HashSet;
import java.util.Iterator; import java.util.Iterator;
import java.util.Set; import java.util.Set;


import org.junit.Test; import org.neo4j.graphdb.ConstraintViolationException;

import org.neo4j.graphdb.Direction; import org.neo4j.graphdb.Direction;
import org.neo4j.graphdb.DynamicRelationshipType;
import org.neo4j.graphdb.GraphDatabaseService;
import org.neo4j.graphdb.Node; import org.neo4j.graphdb.Node;
import org.neo4j.graphdb.NotFoundException; import org.neo4j.graphdb.NotFoundException;
import org.neo4j.graphdb.Relationship; import org.neo4j.graphdb.Relationship;
import org.neo4j.graphdb.TransactionFailureException; import org.neo4j.graphdb.Transaction;
import org.neo4j.helpers.collection.PrefetchingIterator; import org.neo4j.helpers.collection.PrefetchingIterator;
import org.neo4j.kernel.impl.AbstractNeo4jTestCase; import org.neo4j.kernel.impl.AbstractNeo4jTestCase;


import static java.util.Arrays.asList; import static java.util.Arrays.asList;

import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;

import static org.neo4j.kernel.impl.MyRelTypes.TEST; import static org.neo4j.kernel.impl.MyRelTypes.TEST;


public class TestLoopRelationships extends AbstractNeo4jTestCase public class TestLoopRelationships extends AbstractNeo4jTestCase
{ {
@Rule
public ExpectedException exception = ExpectedException.none();

@Test @Test
public void canCreateRelationshipBetweenTwoNodesWithLoopsThenDeleteOneOfTheNodesAndItsRelationships() public void canCreateRelationshipBetweenTwoNodesWithLoopsThenDeleteOneOfTheNodesAndItsRelationships()
throws Exception throws Exception
Expand Down Expand Up @@ -192,18 +198,28 @@ public void getSingleRelationshipOnNodeWithOneLoopOnly() throws Exception
@Test @Test
public void cannotDeleteNodeWithLoopStillAttached() throws Exception public void cannotDeleteNodeWithLoopStillAttached() throws Exception
{ {
Node node = getGraphDb().createNode(); // Given
node.createRelationshipTo( node, TEST ); GraphDatabaseService db = getGraphDb();
newTransaction(); Node node;
node.delete(); try( Transaction tx = db.beginTx() )
try
{ {
commit(); node = db.createNode();
fail( "Shouldn't be able to delete a node which still has a loop relationship attached" ); node.createRelationshipTo( node, DynamicRelationshipType.withName( "MAYOR_OF" ) );
} tx.success();
catch ( TransactionFailureException e )
{ // Good
} }

// And given a transaction deleting just the node
Transaction tx = newTransaction();
node.delete();
tx.success();

// Expect
exception.expect( ConstraintViolationException.class );
exception.expectMessage( "Cannot delete node<"+node.getId()+">, because it still has relationships. " +
"To delete this node, you must first delete its relationships." );

// When I commit
tx.close();
} }


@Test @Test
Expand Down

0 comments on commit 7483d39

Please sign in to comment.