Add connection details to listQueries #7928

systay · 2016-09-13T13:44:42Z

This adds connection details to the output of the procedure listQueries.

Depending on how the user is connecting to Neo4j, different outputs will be seen in the result table. Bolt and REST report connection address, whereas an embedded session will just be listed as embedded.

MishaDemianenko

@systay @boggle first round of review completed, couple of small comments

MishaDemianenko · 2016-09-14T22:51:19Z

community/server/src/main/java/org/neo4j/server/rest/web/InternalJettyServletRequest.java

+                    req.getLocalName(),
+                    req.getLocalAddr(),
+                    req.getLocalPort(),
+                    req.getAuthType()


please note that AuthType can be null

Yes, but this seems not to be even used. Possibly there's a need for some large clean-ups here, but I don't think we should perform that as a part of this PR. Essentially any null-Strings gotten from the Request will simply be passed on transparently to InternalJettyServletRequest users.

representing it as an optional will help a lot for example, printing AuthType: default is much better then print AuthType: null

MishaDemianenko · 2016-09-14T22:52:48Z

community/server/src/main/java/org/neo4j/server/rest/web/InternalJettyServletRequest.java

+        {
+            return new RequestData(
+                    req.getRemoteAddr(),
+                    req.getRemoteHost(),


please note that in case if implementation of servlet api will try to resolve fully qualified name
that can take significant time

MishaDemianenko · 2016-09-14T22:53:36Z

community/server/src/test/java/org/neo4j/server/rest/batch/BatchOperationsTest.java

        assertEquals( "127.0.0.1", req.getRemoteAddr());
        assertEquals( "localhost", req.getRemoteHost());
+        assertEquals( true, req.isSecure() );


MishaDemianenko · 2016-09-14T23:01:09Z

community/server/src/main/java/org/neo4j/server/rest/web/ServerQuerySession.java

-               format("server-session\t%s", username) :
-               format("server-session\thttp\t%s\t%s\t%s", request.getRemoteAddr(), request.getRequestURI(), username );
+            new QuerySource( "server-session" ) :
+            new QuerySource( "server-session", "http", request.getRemoteAddr(), request.getRequestURI() );


it looks like it should be request.getScheme() instead of http always

MishaDemianenko · 2016-09-14T23:22:15Z

community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java

@@ -216,6 +219,7 @@ public void init( SPI spi, Config config )
                        .getOrCreateRelationshipIndex( InternalAutoIndexing.RELATIONSHIP_AUTO_INDEX, null ) ),
                spi.autoIndexing().relationships() );
        this.indexManager = new IndexManagerImpl( spi::currentStatement, idxProvider, nodeAutoIndexer, relAutoIndexer );
+        this.contextFactorySupplier = lazySingleton( () -> new Neo4jTransactionalContextFactory( spi, locker ) );


why do we need contextFactory to be supplier at all? why not just create instance of Neo4jTransactionalContextFactory like:

this.contextFactory = new Neo4jTransactionalContextFactory( spi, locker );

and use it?

This lazy instantiation is needed as the spi is not fully configured during the GraphDatabaseFacade.init. Only creating the factory at first use solves this problem with little fuss.

MishaDemianenko · 2016-09-14T23:29:02Z

community/kernel/src/main/java/org/neo4j/kernel/impl/query/Neo4jTransactionalContext.java

@@ -270,4 +267,9 @@ public AccessMode accessMode()
    {
        return mode;
    }
+
+    public static Function<TransactionalContext, String> describe()


i do not think this belongs to this class

MishaDemianenko · 2016-09-14T23:33:30Z

community/kernel/src/main/java/org/neo4j/kernel/impl/query/Neo4jTransactionalContext.java

-                executingQuery.queryText(),
-                executingQuery.queryParameters(),
-                locker
+                    graph,


shouldn't we use newly introduced context factory for this sort of things?

We are using several private field which are more easily accessed here. Renamed the method instead to improved clarity.

MishaDemianenko · 2016-09-14T23:34:36Z

community/kernel/src/test/java/org/neo4j/kernel/impl/query/Neo4jTransactionalContextTest.java

        Neo4jTransactionalContext transactionalContext =
-            new Neo4jTransactionalContext(
-                databaseQueryService, transaction, statement, "", Collections.emptyMap(), propertyContainerLocker );
+//<<<<<<< HEAD


i do not think we should ever resolve conflicts in this way

MishaDemianenko · 2016-09-14T23:42:08Z

...rise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java

@@ -131,10 +131,6 @@ private Lifecycle createEmptyAdapter()

    static class QueryLogger implements QueryExecutionMonitor
    {
-        private static final MetadataKey<Long> START_TIME = new MetadataKey<>( Long.class, "start time" );
-        private static final MetadataKey<String> QUERY_STRING = new MetadataKey<>( String.class, "query string" );
-        private static final MetadataKey<Map<String,Object>> PARAMS = new MetadataKey<>( paramsClass(), "parameters" );


static method paramsClass is not used anymore and should be deleted

MishaDemianenko · 2016-09-14T23:45:39Z

community/kernel/src/main/java/org/neo4j/kernel/impl/query/QuerySource.java

+    @Override
+    public String toString()
+    {
+        return toString( '\t' );


please use return String.join( "\t", parts ); instead

fickludd · 2016-09-15T09:07:54Z

@MishaDemianenko Thanks for catching some sloppy parts of this code. Addressed most and commented on others.

MishaDemianenko · 2016-09-16T06:31:56Z

community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java

@@ -197,7 +195,7 @@ public GraphDatabaseFacade()
    /**
     * Create a new Core API facade, backed by the given SPI.
     */
-    public void init( SPI spi, Config config )
+    public void initSPI( SPI spi, Config config )


initSPI is a bit confusing imho, since it looks like we initialise SPI here, but actually we just provide facade with dependencies, so i would keep old name instead

MishaDemianenko · 2016-09-16T06:33:14Z

community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java

@@ -197,7 +195,7 @@ public GraphDatabaseFacade()
    /**
     * Create a new Core API facade, backed by the given SPI.
     */
-    public void init( SPI spi, Config config )
+    public void initSPI( SPI spi, Config config )


Will it work if in init method we just provide dependencies - store them as fields and then do al initialisation of fields all together in next method together with context factory?

I tried this, but it failed a lot of tests that do not need the TransactionalContextFactory. It seems like here is work to do, but I would rather not spend more time on this is the scope of this PR.

… field o This aligns procedures better with how Cypher itself handles return columns and probably should have been like this initially

When adding connection details to ExecutingQuery, there was no longer any need to keep QuerySession around. Instead, query logging (the main user of QuerySession) can now get it's data through the TransactionContext which includes an ExecutingQuery.

systay added cypher kernel security 3.1 labels Sep 13, 2016

systay force-pushed the 3.1-list-queries branch from 5052662 to 7eddd37 Compare September 14, 2016 04:23

lutovich assigned MishaDemianenko Sep 14, 2016

systay force-pushed the 3.1-list-queries branch from bf0a849 to 851b19a Compare September 14, 2016 10:37

fickludd mentioned this pull request Sep 14, 2016

Added dbms.setTXMetaData procedure #7943

Merged

MishaDemianenko requested changes Sep 14, 2016

View reviewed changes

fickludd force-pushed the 3.1-list-queries branch from af211bd to 0bac6ab Compare September 15, 2016 08:32

fickludd force-pushed the 3.1-list-queries branch 2 times, most recently from 79b394f to e5bd626 Compare September 15, 2016 14:00

MishaDemianenko reviewed Sep 16, 2016

View reviewed changes

boggle and others added 11 commits September 16, 2016 13:40

Enforce that non-VOID procedures must return at least a single output…

cca93de

… field o This aligns procedures better with how Cypher itself handles return columns and probably should have been like this initially

Add connection details to listQueries

0c6c25f

Fix Birk unit tests

85cec0e

Fixed broken test

93ef5fa

Enabled access to request meta-data after first operation in batch

f107748

Added PR feedback, including lazy remote host lookup

f70712e

Renamed TransactionalContext.provideContext to getOrBeginNewIfClosed

22acac6

Fixed broken import

afb14d8

Moved Neo4jTransactionalContextFactory creation out of DataSourceModule

ef25a31

Adapted manual to changes in Query APIs

3389ff3

fickludd force-pushed the 3.1-list-queries branch from e5bd626 to 3389ff3 Compare September 16, 2016 11:40

craigtaverner merged commit 3389ff3 into neo4j:3.1 Sep 16, 2016

craigtaverner deleted the 3.1-list-queries branch September 16, 2016 16:05

tinwelint added the team-kernel label Aug 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection details to listQueries #7928

Add connection details to listQueries #7928

systay commented Sep 13, 2016 •

edited

Loading

MishaDemianenko left a comment

MishaDemianenko Sep 14, 2016

fickludd Sep 15, 2016

MishaDemianenko Sep 15, 2016

MishaDemianenko Sep 14, 2016

MishaDemianenko Sep 14, 2016

MishaDemianenko Sep 14, 2016

MishaDemianenko Sep 14, 2016

fickludd Sep 15, 2016

MishaDemianenko Sep 14, 2016

MishaDemianenko Sep 14, 2016

fickludd Sep 15, 2016

MishaDemianenko Sep 14, 2016

MishaDemianenko Sep 14, 2016

fickludd Sep 15, 2016

MishaDemianenko Sep 14, 2016

fickludd commented Sep 15, 2016

MishaDemianenko Sep 16, 2016

MishaDemianenko Sep 16, 2016

fickludd Sep 16, 2016

Add connection details to listQueries #7928

Add connection details to listQueries #7928

Conversation

systay commented Sep 13, 2016 • edited Loading

MishaDemianenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fickludd commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

systay commented Sep 13, 2016 •

edited

Loading