From 1b521e58edfdadd034740fa39497d7a085306a02 Mon Sep 17 00:00:00 2001 From: clintval Date: Thu, 16 May 2019 22:33:19 -0700 Subject: [PATCH 1/3] Optionally check configureExecutable config value path actually exists --- .../dagr/core/config/Configuration.scala | 29 +++++++++++-------- .../dagr/core/config/ConfigurationTest.scala | 5 ++++ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/core/src/main/scala/dagr/core/config/Configuration.scala b/core/src/main/scala/dagr/core/config/Configuration.scala index 36389c81..2d9ebef5 100644 --- a/core/src/main/scala/dagr/core/config/Configuration.scala +++ b/core/src/main/scala/dagr/core/config/Configuration.scala @@ -166,24 +166,29 @@ private[config] trait ConfigurationLike extends LazyLogging { } /** - * Attempts to determine the path to an executable, first by looking it up in config, - * and if that fails, by attempting to locate it on the system path. If both fail then - * an exception is raised. + * Attempts to determine the path to an executable, first by looking it up in config and optionally checking to see + * if the path exists. If that fails, we will then locate the executable on the system path. If both strategies fail, + * then an exception is raised. * * @param path the configuration path to look up * @param executable the default name of the executable - * @return An absolute path to the executable to use + * @param mustExist ensure the configuration path to look up references a file which exists + * @return an absolute path to the executable to use + * @throws Generic when the executable cannot be configured */ - def configureExecutable(path: String, executable: String) : Path = { + def configureExecutable(path: String, executable: String, mustExist: Boolean = false) : Path = { Configuration.RequestedKeys += path - optionallyConfigure[Path](path) match { - case Some(exec) => exec - case None => findInPath(executable) match { - case Some(exec) => exec - case None => throw new Generic(s"Could not configurable executable. Config path '$path' is not defined and executable '$executable' is not in PATH.") - } - } + optionallyConfigure[Path](path) + .filterNot(mustExist && !Files.exists(_)) + .orElse(findInPath(executable)) + .getOrElse( + throw new Generic( + s"Could not configurable executable. " + + s"Config path '$path' is not defined, or the config value references a file which does not exist, " + + s"and executable '$executable' is not in PATH." + ) + ) } /** diff --git a/core/src/test/scala/dagr/core/config/ConfigurationTest.scala b/core/src/test/scala/dagr/core/config/ConfigurationTest.scala index 1556ab23..2bcf315d 100644 --- a/core/src/test/scala/dagr/core/config/ConfigurationTest.scala +++ b/core/src/test/scala/dagr/core/config/ConfigurationTest.scala @@ -142,6 +142,11 @@ class ConfigurationTest extends UnitSpec with CaptureSystemStreams { java.getFileName.toString shouldBe "java" Files.isExecutable(java) shouldBe true + // If the config key is found, but the value, as a path, does not exist, then fallback to the system path. + java = conf.configureExecutable("some-executable", "java", mustExist = true) + java.getFileName.toString shouldBe "java" + Files.isExecutable(java) shouldBe true + // the bin directory should not be found, and so should fall back on the system path, and find the java executable java = conf.configureExecutableFromBinDirectory("path-does-not-exist", "java") java.getFileName.toString shouldBe "java" From 27bfd8631153dbdcbe0343bab5b8a5c012c3e265 Mon Sep 17 00:00:00 2001 From: clintval Date: Thu, 16 May 2019 22:50:30 -0700 Subject: [PATCH 2/3] Typo in execption message --- .../main/scala/dagr/core/config/Configuration.scala | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/dagr/core/config/Configuration.scala b/core/src/main/scala/dagr/core/config/Configuration.scala index 2d9ebef5..c1c9bd9f 100644 --- a/core/src/main/scala/dagr/core/config/Configuration.scala +++ b/core/src/main/scala/dagr/core/config/Configuration.scala @@ -166,9 +166,9 @@ private[config] trait ConfigurationLike extends LazyLogging { } /** - * Attempts to determine the path to an executable, first by looking it up in config and optionally checking to see - * if the path exists. If that fails, we will then locate the executable on the system path. If both strategies fail, - * then an exception is raised. + * Attempts to determine the path to an executable, first by looking it up in the config and optionally checking to + * see if the path exists. If that fails, we will then locate the executable on the system path. If both strategies + * fail, then an exception is raised. * * @param path the configuration path to look up * @param executable the default name of the executable @@ -182,11 +182,12 @@ private[config] trait ConfigurationLike extends LazyLogging { optionallyConfigure[Path](path) .filterNot(mustExist && !Files.exists(_)) .orElse(findInPath(executable)) + .map(_.toAbsolutePath) .getOrElse( throw new Generic( - s"Could not configurable executable. " - + s"Config path '$path' is not defined, or the config value references a file which does not exist, " - + s"and executable '$executable' is not in PATH." + s"Could not configure executable." + + s" Config path '$path' is not defined, or the config value references a file which does not exist," + + s" and executable '$executable' is not in the system path." ) ) } From e49e227689f2331e88bb7bfa650a2d0e618b68e6 Mon Sep 17 00:00:00 2001 From: clintval Date: Tue, 21 May 2019 22:05:44 -0700 Subject: [PATCH 3/3] Suit @nh13 review --- .../dagr/core/config/Configuration.scala | 29 ++++++++++++------- .../dagr/core/config/ConfigurationTest.scala | 2 +- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/core/src/main/scala/dagr/core/config/Configuration.scala b/core/src/main/scala/dagr/core/config/Configuration.scala index c1c9bd9f..dd5a0694 100644 --- a/core/src/main/scala/dagr/core/config/Configuration.scala +++ b/core/src/main/scala/dagr/core/config/Configuration.scala @@ -166,28 +166,37 @@ private[config] trait ConfigurationLike extends LazyLogging { } /** - * Attempts to determine the path to an executable, first by looking it up in the config and optionally checking to - * see if the path exists. If that fails, we will then locate the executable on the system path. If both strategies - * fail, then an exception is raised. + * Attempt to determine the path to an executable. + * + * The following logic describes how we will attempt to determine the path to the executable: + * + * 1. If the key `path` is not present in the config, then we immediately fallback to searching the system path for + * `executable`. + * 2. If the key `path` is present in the config and `allowMissingConfigReference` is `true` then the config value + * is returned without testing if the value references an executable filepath that actually exists. + * 3. If the key `path` is present in the config and `allowMissingConfigReference` is `false` then the config value + * is only returned if the value references an executable filepath that exists. If the executable filepath does + * not exist, then we fallback to searching the system path for `executable`. + * 4. If all above strategies fail, then an exception is raised. * * @param path the configuration path to look up * @param executable the default name of the executable - * @param mustExist ensure the configuration path to look up references a file which exists - * @return an absolute path to the executable to use + * @param allowMissingConfigReference whether we require the executable path in the configuration to exist on disk before falling back to searching the system path + * @return an absolute path to the executable * @throws Generic when the executable cannot be configured */ - def configureExecutable(path: String, executable: String, mustExist: Boolean = false) : Path = { + def configureExecutable(path: String, executable: String, allowMissingConfigReference: Boolean = true) : Path = { Configuration.RequestedKeys += path optionallyConfigure[Path](path) - .filterNot(mustExist && !Files.exists(_)) + .filter(allowMissingConfigReference || Files.exists(_)) .orElse(findInPath(executable)) .map(_.toAbsolutePath) .getOrElse( throw new Generic( - s"Could not configure executable." - + s" Config path '$path' is not defined, or the config value references a file which does not exist," - + s" and executable '$executable' is not in the system path." + s"Could not configure executable. Config path `$path` is not defined " + + (if (allowMissingConfigReference) "" else s"or the config value references a file which does not exist, ") + + s"and executable '$executable' is not on the system path." ) ) } diff --git a/core/src/test/scala/dagr/core/config/ConfigurationTest.scala b/core/src/test/scala/dagr/core/config/ConfigurationTest.scala index 2bcf315d..8f235161 100644 --- a/core/src/test/scala/dagr/core/config/ConfigurationTest.scala +++ b/core/src/test/scala/dagr/core/config/ConfigurationTest.scala @@ -143,7 +143,7 @@ class ConfigurationTest extends UnitSpec with CaptureSystemStreams { Files.isExecutable(java) shouldBe true // If the config key is found, but the value, as a path, does not exist, then fallback to the system path. - java = conf.configureExecutable("some-executable", "java", mustExist = true) + java = conf.configureExecutable("some-executable", "java", allowMissingConfigReference = false) java.getFileName.toString shouldBe "java" Files.isExecutable(java) shouldBe true