Skip to content

Commit

Permalink
GH-839 - Check for a read only query hint.
Browse files Browse the repository at this point in the history
Allow custom queries to indicate with `/*+ OGM READ_ONLY */` that they are read only. This will allow procedure calls to not flush the session and fix #839.
  • Loading branch information
michael-simons committed Sep 14, 2020
1 parent de89c9e commit 9e332b2
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
*/
public class ExecuteQueriesDelegate extends SessionDelegate {

private static final String OGM_READ_ONLY_HINT = "/*+ OGM READ_ONLY */";
private static final Pattern WRITE_CYPHER_KEYWORDS = Pattern.compile("\\b(CREATE|MERGE|SET|DELETE|REMOVE|DROP|CALL)\\b",
Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE);
// This is using the Neo4jSession on purpose to retrieve the logger.
Expand Down Expand Up @@ -254,7 +255,10 @@ private Long count(CypherQuery query, boolean isRelationshipEntity) {
return Long.parseLong(resultMap.get(resultKey).toString());
}

private static boolean mayBeReadWrite(String cypher) {
static boolean mayBeReadWrite(String cypher) {
if (cypher.contains(OGM_READ_ONLY_HINT)) {
return false;
}
Matcher matcher = WRITE_CYPHER_KEYWORDS.matcher(cypher);
return matcher.find();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright (c) 2002-2020 "Neo4j,"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.neo4j.ogm.session.delegates;

import java.util.Arrays;
import java.util.Collection;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

/**
* Test for new regular expression to determine write queries
*
* @author Torsten Kuhnhenne
* @author Michael J. Simons
*/
@RunWith(Parameterized.class)
public class ExecuteQueriesDelegateTest {

@Parameterized.Parameters
public static Collection<Object[]> parameters() {
return Arrays.asList(new Object[][] {
{ "CREATE (a:Actor) RETURN a", true },
{ "create (a:Actor) return a", true },
// CREATE
{ "CREATE (a:Actor) RETURN a", true }, { "create (a:Actor) return a", true },
// MERGE
{ "MERGE (a:Actor {id : 10}) ON CREATE SET a.created = timestamp() ON MATCH SET a.accessTime = timestamp()",
true },
{ "merge (a:Actor {id : 10}) ON CREATE SET a.created = timestamp() ON MATCH SET a.accessTime = timestamp()",
true },
// SET
{ "MATCH (a:Actor) SET a.age = 45", true },
{ "match (a:Actor) set a.age = 45", true },
// DELETE
{ "MATCH (a:Actor) DELETE a", true },
{ "match (a:Actor) delete a", true },
{ "MATCH (a:Actor) DETACH DELETE a", true },
{ "match (a:Actor) detach delete a", true },
// REMOVE
{ "MATCH (a:Actor) REMOVE a.age", true }, { "match (a:Actor) remove a.age", true },
// DROP
{ "DROP USER test", true },
// CALL
{ "call sp.doSomething()", true },
{ "CALL sp.doSomething()", true },
{ "MATCH (a:Actor) WITH a CALL sp.doSomething(a)", true },
// CALL with misspelled OGM READ_ONLY hint
{ "MATCH (a:Actor) WITH a CALL /*+ OGM_READ_ONLY */ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+ OGM_READ_ONLY */ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+ OGM READ ONLY */ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+ OGM READ _ONLY */ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+OGM READ_ONLY */ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+ OGM READ_ONLY*/ sp.doSomething(a)", true },
{ "MATCH (a:Actor) WITH a CALL /*+OGM READ_ONLY*/ sp.doSomething(a)", true },
{ "call sp.doSomething() yield x \nWITH x MATCH (f:Foo) \nWHERE f.x = x RETURN f ", true },
// Simple match
{ "MATCH (a:Actor) RETURN a", false }, { "match (a:Actor) return a", false },
// CALL with OGM READ_ONLY-hint
{ "call /*+ OGM READ_ONLY */ sp.doSomething()", false },
{ "/*+ OGM READ_ONLY */ call sp.doSomething()", false },
{ "call sp.doSomething() /*+ OGM READ_ONLY */", false }, {
"MATCH (a:Actor) WITH a CALL /*+ OGM READ_ONLY */ sp.doSomething(a)", false },
{ "call sp.doSomething() /*+ OGM READ_ONLY */ yield x \nWITH x MATCH (f:Foo) \nWHERE f.x = x RETURN f ",
false },
{ "call sp.doSomething() yield x \nWITH x MATCH (f:Foo) \nWHERE f.x = x RETURN f /*+ OGM READ_ONLY */",
false },
});
}

private String query;

private boolean isWriteQuery;

public ExecuteQueriesDelegateTest(String query, boolean isWriteQuery) {
this.query = query;
this.isWriteQuery = isWriteQuery;
}

@Test
public void test() {
boolean mayBeReadWrite = ExecuteQueriesDelegate.mayBeReadWrite(query);
if (isWriteQuery) {
Assert.assertTrue(mayBeReadWrite);
} else {
Assert.assertFalse(mayBeReadWrite);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import ch.qos.logback.classic.LoggerContext;

import java.io.IOException;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -38,6 +39,8 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.mockito.Mockito;
import org.neo4j.ogm.context.MappingContext;
import org.neo4j.ogm.domain.cineasts.annotated.Actor;
import org.neo4j.ogm.domain.cineasts.annotated.ExtendedUser;
import org.neo4j.ogm.domain.cineasts.annotated.Movie;
Expand Down Expand Up @@ -147,6 +150,31 @@ public void readOnlyQueryMustBeReadOnly() {
}
}

@Test
public void shouldBeAbleToIndicateSafeCall() throws NoSuchFieldException, IllegalAccessException {

// Don' think too long about that bloody mess…
MappingContext spyOnMappingContext = Mockito.spy(((Neo4jSession) session).context());
Field mappingContextField = Neo4jSession.class.getDeclaredField("mappingContext");
mappingContextField.setAccessible(true);
mappingContextField.set(session, spyOnMappingContext);

Iterable<String> functionNames = session
.query(String.class, "CALL dbms.functions() yield name", Collections.emptyMap());
assertThat(functionNames).isNotEmpty();
functionNames = session
.query(String.class, "CALL dbms.functions() yield name /*+ OGM READ_ONLY */", Collections.emptyMap());
assertThat(functionNames).isNotEmpty();
functionNames = session
.query(String.class, "CALL dbms.functions() yield name \n/*+ OGM READ_ONLY */", Collections.emptyMap());
assertThat(functionNames).isNotEmpty();
functionNames = session
.query(String.class, "CALL /*+ OGM READ_ONLY */ dbms.functions() yield name", Collections.emptyMap());
assertThat(functionNames).isNotEmpty();

Mockito.verify(spyOnMappingContext, Mockito.atMost(1)).clear();
}

@Test // DATAGRAPH-697
public void modifyingQueryShouldReturnStatistics() {
session.save(new Actor("Jeff"));
Expand Down

0 comments on commit 9e332b2

Please sign in to comment.