Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make use of new database setting for determining conf directory #719

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Make use of new database setting for determining conf directory #719

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04a3239
Moving to database setting
JoelBergstrand Jan 8, 2025
4856577
Fixed tests
JoelBergstrand Jan 8, 2025
afa7b73
Update build.gradle
JoelBergstrand Jan 8, 2025
59d39bb
Updated comment
JoelBergstrand Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ apply from: "licenses-source-header.gradle"

ext {
publicDir = "${project.rootDir}"
neo4jVersionEffective = project.hasProperty("neo4jVersionOverride") ? project.getProperty("neo4jVersionOverride") : "2025.01.0"
neo4jVersionEffective = project.hasProperty("neo4jVersionOverride") ? project.getProperty("neo4jVersionOverride") : "2025.01.0-SNAPSHOT"
JoelBergstrand marked this conversation as resolved.
Show resolved Hide resolved
testContainersVersion = '1.20.2'
apacheArrowVersion = '15.0.0'
}
27 changes: 2 additions & 25 deletions common/src/main/java/apoc/ApocConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.neo4j.configuration.BootloaderSettings.lib_directory;
import static org.neo4j.configuration.BootloaderSettings.run_directory;
import static org.neo4j.configuration.GraphDatabaseSettings.SYSTEM_DATABASE_NAME;
import static org.neo4j.configuration.GraphDatabaseSettings.configuration_directory;
import static org.neo4j.configuration.GraphDatabaseSettings.data_directory;
import static org.neo4j.configuration.GraphDatabaseSettings.load_csv_file_url_root;
import static org.neo4j.configuration.GraphDatabaseSettings.logs_directory;
Expand All @@ -43,7 +44,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;
import org.apache.commons.configuration2.CombinedConfiguration;
import org.apache.commons.configuration2.Configuration;
import org.apache.commons.configuration2.EnvironmentConfiguration;
Expand Down Expand Up @@ -188,30 +188,7 @@ public void init() {
}

protected String determineNeo4jConfFolder() {
String defaultPath = neo4jConfig
.get(neo4j_home)
.resolve(Config.DEFAULT_CONFIG_DIR_NAME)
.toString();
String command = System.getProperty(SUN_JAVA_COMMAND);
if (command == null) {
log.warn(
"system property %s is not set, assuming %s as conf dir. This might cause `apoc.conf` not getting loaded.",
SUN_JAVA_COMMAND, defaultPath);
return defaultPath;
} else {
final String neo4jConfFolder = Stream.of(command.split("--"))
.map(String::trim)
.filter(s -> s.startsWith(CONFIG_DIR))
.map(s -> s.substring(CONFIG_DIR.length()))
.findFirst()
.orElse(defaultPath);
if (defaultPath.equals(neo4jConfFolder)) {
log.info("cannot determine conf folder from sys property %s, assuming %s", command, defaultPath);
} else {
log.info("from system properties: NEO4J_CONF=%s", neo4jConfFolder);
}
return neo4jConfFolder;
}
return neo4jConfig.get(configuration_directory).toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment directly on the correct line, but where this is used now has an incorrect comment, can you fix that up? :) also the log info is wrong now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the log say something like this?

Suggested change
return neo4jConfig.get(configuration_directory).toString();
String neo4jConfFolder = neo4jConfig.get(configuration_directory).toString();
log.info("from database setting: server.directories.configuration=%s", neo4jConfFolder);
return neo4jConfFolder;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually, that line is about setting a system property. Do we even need that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the loging any more. Omitting it and updating the comment.

}

/**
Expand Down
29 changes: 7 additions & 22 deletions common/src/test/java/apoc/ApocConfigCommandExpansionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package apoc;

import static apoc.ApocConfig.APOC_MAX_DECOMPRESSION_RATIO;
import static apoc.ApocConfig.SUN_JAVA_COMMAND;
import static java.nio.file.attribute.PosixFilePermission.GROUP_EXECUTE;
import static java.nio.file.attribute.PosixFilePermission.GROUP_READ;
import static java.nio.file.attribute.PosixFilePermission.GROUP_WRITE;
Expand All @@ -46,7 +45,6 @@
import java.util.Collections;
import java.util.Set;
import org.assertj.core.api.Assertions;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.neo4j.configuration.Config;
Expand Down Expand Up @@ -75,7 +73,6 @@ public void setup() throws Exception {
when(neo4jConfig.get(any())).thenReturn(null);
when(neo4jConfig.get(GraphDatabaseSettings.allow_file_urls)).thenReturn(false);
when(neo4jConfig.expandCommands()).thenReturn(true);
when(neo4jConfig.get(GraphDatabaseSettings.neo4j_home)).thenReturn(Path.of("C:/neo4j/neo4j-enterprise-5.x.0"));

apocConfigCommandExpansionFile = new File(getClass()
.getClassLoader()
Expand All @@ -84,33 +81,23 @@ public void setup() throws Exception {
Files.setPosixFilePermissions(
apocConfigCommandExpansionFile.toPath(), permittedFilePermissionsForCommandExpansion);

when(neo4jConfig.get(GraphDatabaseSettings.configuration_directory))
.thenReturn(Path.of(apocConfigCommandExpansionFile.getParent()));

GlobalProceduresRegistry registry = mock(GlobalProceduresRegistry.class);
DatabaseManagementService databaseManagementService = mock(DatabaseManagementService.class);
apocConfig =
new ApocConfig(neo4jConfig, new SimpleLogService(logProvider), registry, databaseManagementService);
}

@After
public void cleanup() {
System.clearProperty(SUN_JAVA_COMMAND);
}

private void setApocConfigFilePermissions(Set<PosixFilePermission> forbidden) throws Exception {
Files.setPosixFilePermissions(
apocConfigCommandExpansionFile.toPath(),
Sets.union(permittedFilePermissionsForCommandExpansion, forbidden));
}

private void setApocConfigSystemProperty() {
System.setProperty(
SUN_JAVA_COMMAND,
"com.neo4j.server.enterprise.CommercialEntryPoint --home-dir=/home/stefan/neo4j-enterprise-4.0.0-alpha09mr02 --config-dir="
+ apocConfigCommandExpansionFile.getParent());
}

@Test
public void testApocConfWithExpandCommands() {
setApocConfigSystemProperty();
apocConfig.init();

assertEquals("expanded value", apocConfig.getConfig().getString("command.expansion"));
Expand All @@ -120,7 +107,6 @@ public void testApocConfWithExpandCommands() {
public void testApocConfWithInvalidExpandCommands() throws Exception {
String invalidExpandLine = "command.expansion.3=$(fakeCommand 3 + 3)";
addLineToApocConfig(invalidExpandLine);
setApocConfigSystemProperty();

RuntimeException e = assertThrows(RuntimeException.class, apocConfig::init);
String expectedMessage =
Expand All @@ -134,7 +120,6 @@ public void testApocConfWithInvalidExpandCommands() throws Exception {
public void testApocConfWithWrongFilePermissions() throws Exception {
for (PosixFilePermission filePermission : forbiddenFilePermissionsForCommandExpansion) {
setApocConfigFilePermissions(Set.of(filePermission));
setApocConfigSystemProperty();

RuntimeException e = assertThrows(RuntimeException.class, apocConfig::init);
String expectedMessage = "does not have the correct file permissions to evaluate commands.";
Expand All @@ -152,15 +137,14 @@ public void testApocConfWithoutExpandCommands() {
when(neo4jConfig.getDeclaredSettings()).thenReturn(Collections.emptyMap());
when(neo4jConfig.get(any())).thenReturn(null);
when(neo4jConfig.expandCommands()).thenReturn(false);
when(neo4jConfig.get(GraphDatabaseSettings.neo4j_home)).thenReturn(Path.of("C:/neo4j/neo4j-enterprise-5.x.0"));
when(neo4jConfig.get(GraphDatabaseSettings.configuration_directory))
.thenReturn(Path.of(apocConfigCommandExpansionFile.getParent()));

GlobalProceduresRegistry registry = mock(GlobalProceduresRegistry.class);
DatabaseManagementService databaseManagementService = mock(DatabaseManagementService.class);
ApocConfig apocConfig =
new ApocConfig(neo4jConfig, new SimpleLogService(logProvider), registry, databaseManagementService);

setApocConfigSystemProperty();

RuntimeException e = assertThrows(RuntimeException.class, apocConfig::init);
String expectedMessage =
"$(echo \"expanded value\") is a command, but config is not explicitly told to expand it. (Missing --expand-commands argument?)";
Expand All @@ -176,7 +160,8 @@ public void testMaxDecompressionRatioValidation() {
when(neo4jConfig.getDeclaredSettings()).thenReturn(Collections.emptyMap());
when(neo4jConfig.get(any())).thenReturn(null);
when(neo4jConfig.expandCommands()).thenReturn(false);
when(neo4jConfig.get(GraphDatabaseSettings.neo4j_home)).thenReturn(Path.of("C:/neo4j/neo4j-enterprise-5.x.0"));
when(neo4jConfig.get(GraphDatabaseSettings.configuration_directory))
.thenReturn(Path.of("C:/neo4j/neo4j-enterprise-5.x.0/conf"));

GlobalProceduresRegistry registry = mock(GlobalProceduresRegistry.class);
DatabaseManagementService databaseManagementService = mock(DatabaseManagementService.class);
Expand Down
33 changes: 3 additions & 30 deletions common/src/test/java/apoc/ApocConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package apoc;

import static apoc.ApocConfig.SUN_JAVA_COMMAND;
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -50,53 +49,27 @@ public void setup() throws Exception {
when(neo4jConfig.getDeclaredSettings()).thenReturn(Collections.emptyMap());
when(neo4jConfig.get(any())).thenReturn(null);
when(neo4jConfig.get(GraphDatabaseSettings.allow_file_urls)).thenReturn(false);
when(neo4jConfig.get(GraphDatabaseSettings.neo4j_home)).thenReturn(Path.of("C:/neo4j/neo4j-enterprise-5.x.0"));

apocConfigFile =
new File(getClass().getClassLoader().getResource("apoc.conf").toURI());
when(neo4jConfig.get(GraphDatabaseSettings.configuration_directory))
.thenReturn(Path.of(apocConfigFile.getParent()));

GlobalProceduresRegistry registry = mock(GlobalProceduresRegistry.class);
DatabaseManagementService databaseManagementService = mock(DatabaseManagementService.class);
apocConfig =
new ApocConfig(neo4jConfig, new SimpleLogService(logProvider), registry, databaseManagementService);
}

private void setApocConfigSystemProperty() {
System.setProperty(
SUN_JAVA_COMMAND,
"com.neo4j.server.enterprise.CommercialEntryPoint --home-dir=/home/stefan/neo4j-enterprise-4.0.0-alpha09mr02 --config-dir="
+ apocConfigFile.getParent());
}

@Test
public void testDetermineNeo4jConfFolderDefault() {
System.setProperty(SUN_JAVA_COMMAND, "");
assertEquals("C:/neo4j/neo4j-enterprise-5.x.0/conf", apocConfig.determineNeo4jConfFolder());
}

@Test
public void testDetermineNeo4jConfFolder() {
System.setProperty(
SUN_JAVA_COMMAND,
"com.neo4j.server.enterprise.CommercialEntryPoint --home-dir=/home/stefan/neo4j-enterprise-4.0.0-alpha09mr02 --config-dir=/home/stefan/neo4j-enterprise-4.0.0-alpha09mr02/conf");

assertEquals("/home/stefan/neo4j-enterprise-4.0.0-alpha09mr02/conf", apocConfig.determineNeo4jConfFolder());
assertEquals(apocConfigFile.getParent(), apocConfig.determineNeo4jConfFolder());
}

@Test
public void testApocConfFileBeingLoaded() {
setApocConfigSystemProperty();
apocConfig.init();

assertEquals("bar", apocConfig.getConfig().getString("foo"));
}

@Test
public void testDetermineNeo4jConfFolderWithWhitespaces() {
System.setProperty(
SUN_JAVA_COMMAND,
"com.neo4j.server.enterprise.CommercialEntryPoint --config-dir=/home/stefan/neo4j enterprise-4.0.0-alpha09mr02/conf --home-dir=/home/stefan/neo4j enterprise-4.0.0-alpha09mr02");

assertEquals("/home/stefan/neo4j enterprise-4.0.0-alpha09mr02/conf", apocConfig.determineNeo4jConfFolder());
}
}
Loading