Permalink
Browse files

! can, routing, servlet, util: improve *Settings infrastructure

All our Setting model classes follow the same pattern: apart from the default case class constructor we supply two `XxxSettings.apply` overloads:

1. `def apply(system: ActorSystem): XxxSettings`
2. `def apply(config: Config): XxxSettings`

The first one simply calls the second with the ActorSystem's config. However, so far it also selected the respective sub-config for the Settings, e.g. "spray.can.server" for the spray-can `ServerSettings`. The second overload therefore required the correct *sub-config* for the Settings rather than a root-level Config object. Internally it then applied a `withFallback(ConfigFactory.defaultReference(getClass.getClassLoader))` to this sub-config, which was ineffective since the defaultReference supplies a root-level config (we were mixing a root-level config into a sub-config).

This patch fixes this problem by moving sub-config selection from the first overload into the second. It also improves DRYness by factoring out common logic into a new superclass `SettingsCompanion` and adds another convenience `apply` overload for settings overrides given as a String.

Even though the public API in terms of method signatures is not changed by this patch the semantics of the parameter on the second overload change, thereby breaking all code that uses it.
  • Loading branch information...
sirthias committed Jul 12, 2013
1 parent 0b50ddb commit 78d7e4abc11c4de21b6879a6297274c8bf8653fa
@@ -16,9 +16,9 @@
package spray.site
-import com.typesafe.config.{ Config, ConfigFactory }
+import com.typesafe.config.Config
import scala.collection.JavaConverters._
-import akka.actor.ActorSystem
+import spray.util.SettingsCompanion
case class SiteSettings(
interface: String,
@@ -33,19 +33,13 @@ case class SiteSettings(
require(0 < port && port < 65536, "illegal port")
}
-object SiteSettings {
- def apply(system: ActorSystem): SiteSettings =
- apply(system.settings.config getConfig "spray.site")
-
- def apply(config: Config): SiteSettings = {
- val c = config withFallback ConfigFactory.defaultReference(getClass.getClassLoader)
- SiteSettings(
- c getString "interface",
- c getInt "port",
- c getBoolean "dev-mode",
- c getString "repo-dirs" split ':' toList,
- c getString "nightlies-dir",
- c getString "main-version",
- c.getStringList("other-versions").asScala)
- }
+object SiteSettings extends SettingsCompanion[SiteSettings]("spray.site") {
+ def fromSubConfig(c: Config) = apply(
+ c getString "interface",
+ c getInt "port",
+ c getBoolean "dev-mode",
+ c getString "repo-dirs" split ':' toList,
+ c getString "nightlies-dir",
+ c getString "main-version",
+ c.getStringList("other-versions").asScala)
}
@@ -25,7 +25,7 @@ import MediaTypes._
object TestSupport {
- def defaultParserSettings = ParserSettings(ConfigFactory.load() getConfig "spray.can.parsing")
+ def defaultParserSettings = ParserSettings(ConfigFactory.load())
def emptyRawRequest(method: String = "GET") = prep {
"""|%s / HTTP/1.1
@@ -179,7 +179,8 @@ class SprayCanServerSpec extends Specification with NoTimeConversions {
// automatically bind a server
val listener = {
val commander = TestProbe()
- commander.send(IO(Http), Http.Bind(bindHandler.ref, hostname, port))
+ val settings = spray.util.pimpString_(configOverrides).toOption.map(ServerSettings.apply)
+ commander.send(IO(Http), Http.Bind(bindHandler.ref, hostname, port, settings = settings))
commander.expectMsgType[Http.Bound]
commander.sender
}
@@ -16,9 +16,9 @@
package spray.can.client
-import com.typesafe.config.{ ConfigFactory, Config }
+import com.typesafe.config.Config
import scala.concurrent.duration.Duration
-import akka.actor.{ ActorRefFactory, ActorSystem }
+import akka.actor.ActorRefFactory
import spray.can.parsing.ParserSettings
import spray.util._
@@ -43,25 +43,17 @@ case class ClientConnectionSettings(
requirePositiveOrUndefined(connectingTimeout)
}
-object ClientConnectionSettings {
- def apply(system: ActorSystem): ClientConnectionSettings =
- apply(system.settings.config getConfig "spray.can.client")
-
- def apply(config: Config): ClientConnectionSettings = {
- val c = config
- .withFallback(Utils.sprayConfigAdditions)
- .withFallback(ConfigFactory.defaultReference(getClass.getClassLoader))
- ClientConnectionSettings(
- c getString "user-agent-header",
- c getBoolean "ssl-encryption",
- c getDuration "idle-timeout",
- c getDuration "request-timeout",
- c getDuration "reaping-cycle",
- c getBytes "response-chunk-aggregation-limit" toInt,
- c getBytes "request-size-hint" toInt,
- c getDuration "connecting-timeout",
- ParserSettings(c getConfig "parsing"))
- }
+object ClientConnectionSettings extends SettingsCompanion[ClientConnectionSettings]("spray.can.client") {
+ def fromSubConfig(c: Config) = apply(
+ c getString "user-agent-header",
+ c getBoolean "ssl-encryption",
+ c getDuration "idle-timeout",
+ c getDuration "request-timeout",
+ c getDuration "reaping-cycle",
+ c getBytes "response-chunk-aggregation-limit" toInt,
+ c getBytes "request-size-hint" toInt,
+ c getDuration "connecting-timeout",
+ ParserSettings fromSubConfig c.getConfig("parsing"))
def apply(optionalSettings: Option[ClientConnectionSettings])(implicit actorRefFactory: ActorRefFactory): ClientConnectionSettings =
optionalSettings getOrElse apply(actorSystem)
@@ -16,9 +16,8 @@
package spray.can.client
-import com.typesafe.config.{ ConfigFactory, Config }
+import com.typesafe.config.Config
import scala.concurrent.duration.Duration
-import akka.actor.ActorSystem
import spray.util._
case class HostConnectorSettings(
@@ -33,17 +32,11 @@ case class HostConnectorSettings(
requirePositiveOrUndefined(idleTimeout)
}
-object HostConnectorSettings {
- def apply(system: ActorSystem): HostConnectorSettings =
- apply(system.settings.config getConfig "spray.can.host-connector")
-
- def apply(config: Config): HostConnectorSettings = {
- val c = config withFallback ConfigFactory.defaultReference(getClass.getClassLoader)
- HostConnectorSettings(
- c getInt "max-connections",
- c getInt "max-retries",
- c getBoolean "pipelining",
- c getDuration "idle-timeout",
- ClientConnectionSettings(c getConfig "client"))
- }
+object HostConnectorSettings extends SettingsCompanion[HostConnectorSettings]("spray.can.host-connector") {
+ def fromSubConfig(c: Config) = apply(
+ c getInt "max-connections",
+ c getInt "max-retries",
+ c getBoolean "pipelining",
+ c getDuration "idle-timeout",
+ ClientConnectionSettings fromSubConfig c.getConfig("client"))
}
@@ -18,8 +18,8 @@ package spray.can.parsing
import com.typesafe.config.Config
import scala.collection.JavaConverters._
-import akka.actor.ActorSystem
import spray.http.Uri
+import spray.util._
case class ParserSettings(
maxUriLength: Int,
@@ -49,28 +49,20 @@ case class ParserSettings(
headerValueCacheLimits.getOrElse(headerName, defaultHeaderValueCacheLimit)
}
-object ParserSettings {
- def apply(system: ActorSystem): ParserSettings =
- apply(system.settings.config getConfig "spray.can.parsing")
-
- def apply(config: Config): ParserSettings = {
- def bytes(key: String): Int = {
- val value: Long = config getBytes key
- if (value <= Int.MaxValue) value.toInt
- else sys.error(s"ParserSettings config setting $key must not be larger than ${Int.MaxValue}")
- }
- val cacheConfig = config.getConfig("header-cache")
- ParserSettings(
- bytes("max-uri-length"),
- bytes("max-response-reason-length"),
- bytes("max-header-name-length"),
- bytes("max-header-value-length"),
- bytes("max-header-count"),
- bytes("max-content-length"),
- bytes("max-chunk-ext-length"),
- bytes("max-chunk-size"),
- Uri.ParsingMode(config getString "uri-parsing-mode"),
- config getBoolean "illegal-header-warnings",
+object ParserSettings extends SettingsCompanion[ParserSettings]("spray.can.parsing") {
+ def fromSubConfig(c: Config) = {
+ val cacheConfig = c getConfig "header-cache"
+ apply(
+ c getIntBytes "max-uri-length",
+ c getIntBytes "max-response-reason-length",
+ c getIntBytes "max-header-name-length",
+ c getIntBytes "max-header-value-length",
+ c getIntBytes "max-header-count",
+ c getIntBytes "max-content-length",
+ c getIntBytes "max-chunk-ext-length",
+ c getIntBytes "max-chunk-size",
+ Uri.ParsingMode(c getString "uri-parsing-mode"),
+ c getBoolean "illegal-header-warnings",
cacheConfig.entrySet.asScala.map(kvp kvp.getKey -> cacheConfig.getInt(kvp.getKey))(collection.breakOut))
}
}
@@ -16,10 +16,11 @@
package spray.can.server
-import com.typesafe.config.{ ConfigFactory, Config }
+import com.typesafe.config.Config
import scala.concurrent.duration.Duration
-import akka.actor.ActorSystem
import spray.can.parsing.ParserSettings
+import spray.http.parser.HttpParser
+import spray.http.HttpHeaders._
import spray.util._
case class BackpressureSettings(noAckRate: Int, readingLowWatermark: Int)
@@ -63,39 +64,37 @@ case class ServerSettings(
"idle-timeout must be > request-timeout (if the latter is not 'infinite')")
}
-object ServerSettings {
- def apply(system: ActorSystem): ServerSettings =
- apply(system.settings.config getConfig "spray.can.server")
-
- def apply(config: Config): ServerSettings = {
- val c = config
- .withFallback(Utils.sprayConfigAdditions)
- .withFallback(ConfigFactory.defaultReference(getClass.getClassLoader))
- val backpressureSettings =
- Some(BackpressureSettings(
+object ServerSettings extends SettingsCompanion[ServerSettings]("spray.can.server") {
+ def fromSubConfig(c: Config) = apply(
+ c getString "server-header",
+ c getBoolean "ssl-encryption",
+ c.getString("pipelining-limit") match { case "disabled" 0; case _ c getInt "pipelining-limit" },
+ c getDuration "idle-timeout",
+ c getDuration "request-timeout",
+ c getDuration "timeout-timeout",
+ c getDuration "reaping-cycle",
+ c getBoolean "stats-support",
+ c getBoolean "remote-address-header",
+ c getBoolean "transparent-head-requests",
+ c getString "timeout-handler",
+ c getBoolean "chunkless-streaming",
+ c getBoolean "verbose-error-messages",
+ c getBytes "request-chunk-aggregation-limit" toInt,
+ c getBytes "response-size-hint" toInt,
+ c getDuration "bind-timeout",
+ c getDuration "unbind-timeout",
+ c getDuration "registration-timeout",
+ defaultHostHeader =
+ HttpParser.parseHeader(RawHeader("Host", c getString "default-host-header")) match {
+ case Right(x: Host) x
+ case Left(error) sys.error(error.withSummary("Configured `default-host-header` is illegal").formatPretty)
+ case Right(_) throw new IllegalStateException
+ },
+ backpressureSettings =
+ if (c.getBoolean("automatic-back-pressure-handling"))
+ Some(BackpressureSettings(
c getInt "back-pressure.noack-rate",
- c getPossiblyInfiniteInt "back-pressure.reading-low-watermark")).filter(_ c.getBoolean("automatic-back-pressure-handling"))
-
- ServerSettings(
- c getString "server-header",
- c getBoolean "ssl-encryption",
- c.getString("pipelining-limit") match { case "disabled" 0; case _ c getInt "pipelining-limit" },
- c getDuration "idle-timeout",
- c getDuration "request-timeout",
- c getDuration "timeout-timeout",
- c getDuration "reaping-cycle",
- c getBoolean "stats-support",
- c getBoolean "remote-address-header",
- c getBoolean "transparent-head-requests",
- c getString "timeout-handler",
- c getBoolean "chunkless-streaming",
- c getBoolean "verbose-error-messages",
- c getBytes "request-chunk-aggregation-limit" toInt,
- c getBytes "response-size-hint" toInt,
- c getDuration "bind-timeout",
- c getDuration "unbind-timeout",
- c getDuration "registration-timeout",
- backpressureSettings,
- ParserSettings(c getConfig "parsing"))
- }
+ c getPossiblyInfiniteInt "back-pressure.reading-low-watermark"))
+ else None,
+ ParserSettings fromSubConfig c.getConfig("parsing"))
}
@@ -16,8 +16,8 @@
package spray.routing
-import com.typesafe.config.{ ConfigFactory, Config }
-import akka.actor.{ ActorRefFactory, ActorSystem }
+import com.typesafe.config.Config
+import akka.actor.ActorRefFactory
import spray.util._
case class RoutingSettings(
@@ -31,20 +31,14 @@ case class RoutingSettings(
require(fileChunkingChunkSize > 0, "file-chunking-chunk-size must be > 0")
}
-object RoutingSettings {
+object RoutingSettings extends SettingsCompanion[RoutingSettings]("spray.routing") {
+ def fromSubConfig(c: Config) = apply(
+ c getBoolean "verbose-error-messages",
+ c getBytes "file-chunking-threshold-size",
+ c getBytes "file-chunking-chunk-size",
+ c getConfig "users",
+ c getBoolean "render-vanity-footer")
+
implicit def default(implicit refFactory: ActorRefFactory) =
apply(actorSystem)
-
- def apply(system: ActorSystem): RoutingSettings =
- apply(system.settings.config getConfig "spray.routing")
-
- def apply(config: Config): RoutingSettings = {
- val c = config withFallback ConfigFactory.defaultReference(getClass.getClassLoader)
- RoutingSettings(
- c getBoolean "verbose-error-messages",
- c getBytes "file-chunking-threshold-size",
- c getBytes "file-chunking-chunk-size",
- c getConfig "users",
- c getBoolean "render-vanity-footer")
- }
}
@@ -16,9 +16,8 @@
package spray.servlet
-import com.typesafe.config.{ ConfigFactory, Config }
+import com.typesafe.config.Config
import scala.concurrent.duration.Duration
-import akka.actor.ActorSystem
import spray.http.Uri
import spray.util._
@@ -41,20 +40,14 @@ case class ConnectorSettings(
val rootPathCharCount = rootPath.charCount
}
-object ConnectorSettings {
- def apply(system: ActorSystem): ConnectorSettings =
- apply(system.settings.config getConfig "spray.servlet")
-
- def apply(config: Config): ConnectorSettings = {
- val c = config withFallback ConfigFactory.defaultReference(getClass.getClassLoader)
- ConnectorSettings(
- c getString "boot-class",
- c getDuration "request-timeout",
- c getDuration "timeout-timeout",
- c getString "timeout-handler",
- Uri.Path(c getString "root-path"),
- c getBoolean "remote-address-header",
- c getBoolean "verbose-error-messages",
- c getBytes "max-content-length")
- }
+object ConnectorSettings extends SettingsCompanion[ConnectorSettings]("spray.servlet") {

This comment has been minimized.

Show comment
Hide comment
@unoexperto

unoexperto Jul 21, 2013

It looks like this build broke the code. spray.servlet config is already passed to constructor of SettingsCompanion therefore making it to look for spray.servlet.spray.servlet

@unoexperto

unoexperto Jul 21, 2013

It looks like this build broke the code. spray.servlet config is already passed to constructor of SettingsCompanion therefore making it to look for spray.servlet.spray.servlet

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Jul 21, 2013

Member

I don't understand. What is broken? Could you elaborate?

@jrudolph

jrudolph Jul 21, 2013

Member

I don't understand. What is broken? Could you elaborate?

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Jul 21, 2013

Member

Forget it. I just saw your other messages.

@jrudolph

jrudolph Jul 21, 2013

Member

Forget it. I just saw your other messages.

+ def fromSubConfig(c: Config) = apply(
+ c getString "boot-class",
+ c getDuration "request-timeout",
+ c getDuration "timeout-timeout",
+ c getString "timeout-handler",
+ Uri.Path(c getString "root-path"),
+ c getBoolean "remote-address-header",
+ c getBoolean "verbose-error-messages",
+ c getBytes "max-content-length")
}
Oops, something went wrong.

0 comments on commit 78d7e4a

Please sign in to comment.