From fb2089b27686ce9188afc408217eeaf002953b4c Mon Sep 17 00:00:00 2001 From: Tobias Lindaaker Date: Tue, 31 Jan 2017 13:21:07 +0100 Subject: [PATCH] Ensure that shell closes the query result This is required in order to log the query. --- .../org/neo4j/shell/impl/SystemOutput.java | 7 +- .../neo4j/shell/kernel/apps/cypher/Start.java | 1 + enterprise/query-logging/pom.xml | 7 + .../kernel/impl/query/QueryLoggerIT.java | 8 +- .../impl/query/ShellQueryLoggingIT.java | 190 ++++++++++++++++++ 5 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/ShellQueryLoggingIT.java diff --git a/community/shell/src/main/java/org/neo4j/shell/impl/SystemOutput.java b/community/shell/src/main/java/org/neo4j/shell/impl/SystemOutput.java index c66f88bc37ce7..8892cb71675f0 100644 --- a/community/shell/src/main/java/org/neo4j/shell/impl/SystemOutput.java +++ b/community/shell/src/main/java/org/neo4j/shell/impl/SystemOutput.java @@ -40,9 +40,14 @@ public SystemOutput() this( System.out ); } + public SystemOutput( PrintWriter out ) + { + this.out = out; + } + public SystemOutput( OutputStream out ) { - this.out = new PrintWriter( new OutputStreamWriter( out, StandardCharsets.UTF_8 ) ); + this( new PrintWriter( new OutputStreamWriter( out, StandardCharsets.UTF_8 ) ) ); } public void print( Serializable object ) diff --git a/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java b/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java index 7f682533d0b37..ad4e23836cb2f 100644 --- a/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java +++ b/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java @@ -130,6 +130,7 @@ protected void handleResult( Output out, Result result, long startTime ) throws RemoteException, ShellException { printResult( out, result, startTime ); + result.close(); } private void printResult( Output out, Result result, long startTime ) throws RemoteException diff --git a/enterprise/query-logging/pom.xml b/enterprise/query-logging/pom.xml index 8bef8dd1e2b7c..064d74da6b13e 100644 --- a/enterprise/query-logging/pom.xml +++ b/enterprise/query-logging/pom.xml @@ -75,6 +75,13 @@ test + + org.neo4j + neo4j-shell + ${project.version} + test + + org.neo4j neo4j-io diff --git a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java index a79749c023826..357011941b2ec 100644 --- a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java +++ b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java @@ -40,6 +40,7 @@ import org.neo4j.graphdb.Result; import org.neo4j.graphdb.factory.GraphDatabaseBuilder; import org.neo4j.graphdb.factory.GraphDatabaseSettings; +import org.neo4j.io.fs.FileSystemAbstraction; import org.neo4j.kernel.configuration.Settings; import org.neo4j.logging.AssertableLogProvider; import org.neo4j.test.EphemeralFileSystemRule; @@ -191,10 +192,15 @@ private void executeQueryAndShutdown( GraphDatabaseService database, String quer } private List readAllLines( File logFilename ) throws IOException + { + return readAllLines( fileSystem.get(), logFilename ); + } + + public static List readAllLines( FileSystemAbstraction fs, File logFilename ) throws IOException { List logLines = new ArrayList<>(); try ( BufferedReader reader = new BufferedReader( - fileSystem.get().openAsReader( logFilename, StandardCharsets.UTF_8 ) ) ) + fs.openAsReader( logFilename, StandardCharsets.UTF_8 ) ) ) { for ( String line; (line = reader.readLine()) != null; ) { diff --git a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/ShellQueryLoggingIT.java b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/ShellQueryLoggingIT.java new file mode 100644 index 0000000000000..6f1266cf08724 --- /dev/null +++ b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/ShellQueryLoggingIT.java @@ -0,0 +1,190 @@ +/* + * Copyright (c) 2002-2017 "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 Affero 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 Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.impl.query; + +import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.HashMap; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestRule; +import org.junit.runners.model.Statement; + +import org.neo4j.graphdb.Result; +import org.neo4j.graphdb.Transaction; +import org.neo4j.graphdb.factory.GraphDatabaseBuilder; +import org.neo4j.graphdb.factory.GraphDatabaseFactory; +import org.neo4j.graphdb.factory.GraphDatabaseSettings; +import org.neo4j.kernel.configuration.Settings; +import org.neo4j.shell.ShellClient; +import org.neo4j.shell.ShellException; +import org.neo4j.shell.ShellLobby; +import org.neo4j.shell.impl.SystemOutput; +import org.neo4j.shell.kernel.GraphDatabaseShellServer; +import org.neo4j.test.DatabaseRule; +import org.neo4j.test.EphemeralFileSystemRule; +import org.neo4j.test.ImpermanentDatabaseRule; +import org.neo4j.test.TargetDirectory; +import org.neo4j.test.TestGraphDatabaseFactory; + +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertThat; +import static org.junit.rules.RuleChain.outerRule; +import static org.neo4j.kernel.impl.query.QueryLoggerIT.readAllLines; +import static org.neo4j.test.TargetDirectory.testDirForTest; + +public class ShellQueryLoggingIT +{ + private final EphemeralFileSystemRule fs = new EphemeralFileSystemRule(); + private final TargetDirectory.TestDirectory dir = testDirForTest( getClass() ); + private final DatabaseRule db = new ImpermanentDatabaseRule() + { + @Override + protected void configure( GraphDatabaseFactory factory ) + { + ((TestGraphDatabaseFactory) factory).setFileSystem( fs.get() ); + } + + @Override + protected void configure( GraphDatabaseBuilder builder ) + { + builder.setConfig( GraphDatabaseSettings.log_queries, Settings.TRUE ); + builder.setConfig( GraphDatabaseSettings.logs_directory, logsDirectory().getPath() ); + } + }; + @Rule + public final TestRule order = outerRule( dir ).around( fs ).around( db ) + .around( ( base, description ) -> new Statement() + { + @Override + public void evaluate() throws Throwable + { + try + { + base.evaluate(); + } + catch ( Throwable e ) + { + System.err.println( "" ); + System.err.print( out ); + System.err.println( "" ); + throw e; + } + } + } ); + private final StringWriter out = new StringWriter(); + private GraphDatabaseShellServer server; + private ShellClient client; + + @Before + public void setup() throws Exception + { + server = new GraphDatabaseShellServer( db.getGraphDatabaseAPI() ); + SystemOutput output = new SystemOutput( new PrintWriter( out ) ); + client = ShellLobby.newClient( server, new HashMap<>(), output, action -> () -> + { + // we don't need any handling of CTRL+C + } ); + out.getBuffer().setLength( 0 ); // clear the output (remove the welcome message) + } + + @After + public void shutdown() throws Exception + { + client.shutdown(); + server.shutdown(); + } + + @Test + public void shouldLogQueryWhenExecutingDirectly() throws Exception + { + // given + String query = "CREATE (foo:Foo{bar:'baz'}) RETURN foo.bar"; + + // when + try ( Result result = db.execute( query ); + PrintWriter out = new PrintWriter( this.out ) ) + { + result.writeAsStringTo( out ); + } + + // then + assertThat( out.toString(), allOf( + containsString( "Nodes created: 1" ), + containsString( "Properties set: 1" ), + containsString( "Labels added: 1" ) ) ); + assertThat( queryLog(), contains( containsString( query ) ) ); + } + + @Test + public void shouldLogReadQuery() throws Exception + { + // given + try ( Transaction tx = db.beginTx() ) + { + db.createNode().setProperty( "hello", "world" ); + + tx.success(); + } + + // then + shouldLogQuery( "MATCH (n) RETURN n" ); + } + + @Test + public void shouldLogWriteQuery() throws Exception + { + shouldLogQuery( "CREATE (:Foo{bar:'baz'})" ); + } + + private void shouldLogQuery( String query ) throws ShellException, IOException + { + // when + client.evaluate( query + ';' ); + + // then + assertThat( queryLog(), contains( containsString( query ) ) ); + } + + private List queryLog() throws IOException + { + return readAllLines( fs.get(), queryLogFile() ); + } + + private File queryLogFile() + { + return new File( logsDirectory(), "query.log" ); + } + + private File logsDirectory() + { + File logsDir = new File( dir.graphDbDir(), "logs" ); + fs.get().mkdirs( logsDir ); + return logsDir; + } +}