Merge pull request #5394 from lutovich/2.2-rollback-on-client-and-tra…

…nsient-err

Client and transient errors cause transaction rollback

community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/Status.java

@@ -400,13 +400,13 @@ public int hashCode()
     public enum Classification
     {
         /** The Client sent a bad request - changing the request might yield a successful outcome. */
-        ClientError( TransactionEffect.NONE,
+        ClientError( TransactionEffect.ROLLBACK,
                 "The Client sent a bad request - changing the request might yield a successful outcome." ),
         /** The database failed to service the request. */
         DatabaseError( TransactionEffect.ROLLBACK,
                 "The database failed to service the request. " ),
         /** The database cannot service the request right now, retrying later might yield a successful outcome. */
-        TransientError( TransactionEffect.NONE,
+        TransientError( TransactionEffect.ROLLBACK,
                 "The database cannot service the request right now, retrying later might yield a successful outcome. "),
         ;
 

community/server/CHANGES.txt

@@ -1,3 +1,7 @@
+2.2.6
+-----
+o ClientError and TransientError make REST API transactions rollback
+
 2.2.3
 -----
 o Improve force shutdown when stopping an unresponsive Neo4j server

community/server/src/test/java/org/neo4j/server/rest/AbstractRestFunctionalTestBase.java

@@ -37,6 +37,7 @@
 import org.neo4j.test.GraphDescription;
 import org.neo4j.test.GraphHolder;
 import org.neo4j.test.TestData;
+import org.neo4j.test.server.HTTP;
 import org.neo4j.test.server.SharedServerTestBase;
 import org.neo4j.visualization.asciidoc.AsciidocHelper;
 
@@ -90,12 +91,12 @@ public String doCypherRestCall( String endpoint, String scriptTemplate, Status s
                 .description( AsciidocHelper.createAsciiDocSnippet( "cypher", snippet ) );
         return gen().post( endpoint ).entity();
     }
-    
+
     private Long idFor( String name )
     {
         return data.get().get( name ).getId();
     }
-    
+
     private String createParameterString( Pair<String, String>[] params )
     {
         String paramString = "";
@@ -117,7 +118,7 @@ protected String createScript( String template )
         }
         return template;
     }
-    
+
     protected String startGraph( String name )
     {
         return AsciidocHelper.createGraphVizWithNodeId( "Starting Graph", graphdb(), name );
@@ -128,8 +129,8 @@ public GraphDatabaseService graphdb()
     {
         return server().getDatabase().getGraph();
     }
-    
-    protected String getDataUri()
+
+    protected static String getDataUri()
     {
         return "http://localhost:7474/db/data/";
     }
@@ -158,12 +159,34 @@ protected String postNodeIndexUri( String indexName )
     {
         return getDataUri() + PATH_NODE_INDEX + "/" + indexName;
     }
-    
+
     protected String postRelationshipIndexUri( String indexName )
     {
         return getDataUri() + PATH_RELATIONSHIP_INDEX + "/" + indexName;
     }
 
+    protected String txUri()
+    {
+        return getDataUri() + "transaction";
+    }
+
+    protected static String txCommitUri()
+    {
+        return getDataUri() + "transaction/commit";
+    }
+
+    protected String txUri( long txId )
+    {
+        return getDataUri() + "transaction/" + txId;
+    }
+
+    public static long extractTxId( HTTP.Response response )
+    {
+        int lastSlash = response.location().lastIndexOf( "/" );
+        String txIdString = response.location().substring( lastSlash + 1 );
+        return Long.parseLong( txIdString );
+    }
+
     protected Node getNode( String name )
     {
         return data.get().get( name );
@@ -179,7 +202,7 @@ protected Node[] getNodes( String... names )
         }
         return result.toArray(nodes);
     }
-    
+
     public void assertSize( int expectedSize, String entity )
     {
         Collection<?> hits;
@@ -193,7 +216,7 @@ public void assertSize( int expectedSize, String entity )
             throw new RuntimeException( e );
         }
     }
-    
+
     public String getPropertiesUri( Relationship rel )
     {
         return getRelationshipUri(rel)+  "/properties";
@@ -202,17 +225,17 @@ public String getPropertiesUri( Node node )
     {
         return getNodeUri(node)+  "/properties";
     }
-    
+
     public RESTDocsGenerator gen() {
         return gen.get();
     }
-    
+
     public void description( String description )
     {
         gen().description( description );
-        
+
     }
-    
+
     protected String getDocumentationSectionName() {
         return "dev/rest-api";
     }

community/server/src/test/java/org/neo4j/server/rest/transactional/integration/ClientErrorIT.java

@@ -0,0 +1,132 @@
+/*
+ * 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.server.rest.transactional.integration;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.neo4j.kernel.api.exceptions.Status;
+import org.neo4j.server.rest.AbstractRestFunctionalTestBase;
+import org.neo4j.server.rest.domain.JsonParseException;
+import org.neo4j.test.server.HTTP;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+import static org.neo4j.server.rest.transactional.integration.TransactionMatchers.containsNoErrors;
+import static org.neo4j.server.rest.transactional.integration.TransactionMatchers.hasErrors;
+import static org.neo4j.test.server.HTTP.POST;
+import static org.neo4j.test.server.HTTP.RawPayload.quotedJson;
+
+@RunWith( Parameterized.class )
+public class ClientErrorIT extends AbstractRestFunctionalTestBase
+{
+    private static final int UNIQUE_ISBN = 12345;
+
+    @Parameterized.Parameter( 0 )
+    public String query;
+
+    @Parameterized.Parameter( 1 )
+    public Status errorStatus;
+
+    @Parameterized.Parameters( name = "{0} should cause {1}" )
+    public static List<Object[]> queriesWithStatuses()
+    {
+        return Arrays.asList(
+                new Object[]{
+                        "Not a valid query",
+                        Status.Statement.InvalidSyntax
+                },
+                new Object[]{
+                        "RETURN {foo}",
+                        Status.Statement.ParameterMissing
+                },
+                new Object[]{
+                        "MATCH (n) WITH n.prop AS n2 RETURN n2.prop",
+                        Status.Statement.InvalidType
+                },
+                new Object[]{
+                        "CYPHER 1.9 EXPLAIN MATCH n RETURN n",
+                        Status.Statement.InvalidArguments
+                },
+                new Object[]{
+                        "RETURN 10 / 0",
+                        Status.Statement.ArithmeticError
+                },
+                new Object[]{
+                        "CREATE INDEX ON :Person(name)",
+                        Status.Transaction.InvalidType
+                },
+                new Object[]{
+                        "CREATE (n:``)",
+                        Status.Schema.IllegalTokenName
+                },
+                new Object[]{
+                        "CREATE (b:Book {isbn: " + UNIQUE_ISBN + "})",
+                        Status.Schema.ConstraintViolation
+                },
+                new Object[]{
+                        "LOAD CSV FROM 'http://127.0.0.1/null/' AS line CREATE (a {name:line[0]})", // invalid for json
+                        Status.Request.InvalidFormat
+                }
+        );
+    }
+
+    @BeforeClass
+    public static void prepareDatabase()
+    {
+        POST( txCommitUri(), quotedJson(
+                "{'statements': [{'statement': 'CREATE INDEX ON :Book(name)'}]}" ) );
+
+        POST( txCommitUri(), quotedJson(
+                "{'statements': [{'statement': 'CREATE CONSTRAINT ON (b:Book) ASSERT b.isbn IS UNIQUE'}]}" ) );
+
+        POST( txCommitUri(), quotedJson(
+                "{'statements': [{'statement': 'CREATE (b:Book {isbn: " + UNIQUE_ISBN + "})'}]}" ) );
+    }
+
+    @Test
+    public void clientErrorShouldRollbackTheTransaction() throws JsonParseException
+    {
+        // Given
+        HTTP.Response first = POST( txUri(), quotedJson( "{'statements': [{'statement': 'CREATE (n {prop : 1})'}]}" ) );
+        assertThat( first.status(), is( 201 ) );
+        assertThat( first, containsNoErrors() );
+        long txId = extractTxId( first );
+
+        // When
+        HTTP.Response malformed = POST( txUri( txId ),
+                quotedJson( "{'statements': [{'statement': '" + query + "'}]}" ) );
+
+        // Then
+
+        // malformed POST contains expected error
+        assertThat( malformed.status(), is( 200 ) );
+        assertThat( malformed, hasErrors( errorStatus ) );
+
+        // transaction was rolled back on the previous step and we can't commit it
+        HTTP.Response commit = POST( first.stringFromContent( "commit" ) );
+        assertThat( commit.status(), is( 404 ) );
+    }
+}

community/server/src/test/java/org/neo4j/server/rest/transactional/integration/TransactionErrorIT.java

@@ -117,19 +117,8 @@ public void begin_and_execute_periodic_commit_that_fails() throws Exception
         }
     }
 
-    private String txUri()
-    {
-        return getDataUri() + "transaction";
-    }
-
-    private String txCommitUri()
-    {
-        return getDataUri() + "transaction/commit";
-    }
-
     private long countNodes()
     {
         return TransactionMatchers.countNodes( graphdb() );
     }
-
 }

community/server/src/test/java/org/neo4j/server/rest/transactional/integration/TransactionIT.java

@@ -167,17 +167,19 @@ public void begin_and_execute__commit_with_badly_escaped_statement() throws Exce
                       "} ] }";
 
         // begin and execute
+        // given statement is badly escaped and it is a client error, thus tx is rolled back at once
         Response begin = http.POST( "/db/data/transaction", quotedJson( json ) );
 
         String commitResource = begin.stringFromContent( "commit" );
 
-        // commit
+        // commit fails because tx was rolled back on the previous step
         Response commit = http.POST( commitResource );
 
         assertThat( begin.status(), equalTo( 201 ) );
         assertThat( begin, hasErrors( Status.Request.InvalidFormat ) );
-        assertThat( commit.status(), equalTo( 200 ) );
-        assertThat( commit, containsNoErrors() );
+
+        assertThat( commit.status(), equalTo( 404 ) );
+        assertThat( commit, hasErrors( Status.Transaction.UnknownId ) );
 
         assertThat( countNodes(), equalTo( nodesInDatabaseBeforeTransaction ) );
     }