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.

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

Scala Stream Collector: provide a way to specify custom path mappings (closes #4087) #4111

Merged
merged 4 commits into from Jul 3, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Scala Stream Collector: provide a way to specify custom path mappings (

…closes #4087)
  • Loading branch information...
dilyand committed Jun 24, 2019
commit aee002a8ab5d4bd3d969371a779d71503db819ee
@@ -14,12 +14,11 @@
*/
package com.snowplowanalytics.snowplow.collectors.scalastream

import akka.http.scaladsl.model.{ContentType, HttpResponse, StatusCode, StatusCodes}
import akka.http.scaladsl.model._
import akka.http.scaladsl.model.headers.HttpCookiePair
import akka.http.scaladsl.server.{Directive1, Route}
import akka.http.scaladsl.server.Directives._
import com.snowplowanalytics.snowplow.collectors.scalastream.model.DntCookieMatcher

import monitoring.BeanRegistry

trait CollectorRoute {
@@ -43,7 +42,7 @@ trait CollectorRoute {
extractors { (host, ip, request) =>
// get the adapter vendor and version from the path
path(Segment / Segment) { (vendor, version) =>
val path = s"/$vendor/$version"
val path = collectorService.determinePath(vendor, version)

This comment has been minimized.

Copy link
@chuwy

chuwy Jun 24, 2019

Member

So, does it mean we can rewrite only two-segmented URIs with two-segmented URIs? /com.acme/foo/bar won't be supported neither as a source nor destinatioin?

This comment has been minimized.

Copy link
@dilyand

dilyand Jun 24, 2019

Author Collaborator

Yes. This is the current behaviour as well.

This comment has been minimized.

Copy link
@BenFradet

BenFradet Jul 1, 2019

Contributor

did you look into Segments in the end to allow for arbitrary incoming paths?

post {
extractContentType { ct =>
entity(as[String]) { body =>
@@ -57,6 +57,7 @@ trait Service {
): (HttpResponse, List[Array[Byte]])
def cookieName: Option[String]
def doNotTrackCookie: Option[DntCookieMatcher]
def determinePath(vendor: String, version: String): String
}

object CollectorService {
@@ -78,6 +79,16 @@ class CollectorService(
override val cookieName = config.cookieName
override val doNotTrackCookie = config.doNotTrackHttpCookie

/**
* Determines the path to be used in the response,
* based on whether a mapping can be found in the config for the original request path.
*/
override def determinePath(vendor: String, version: String): String = {
val original = s"$vendor/$version"
val mappingOrOriginal = config.paths.getOrElse(original, original)
This conversation was marked as resolved by BenFradet

This comment has been minimized.

Copy link
@chuwy

chuwy Jun 24, 2019

Member

Is there a reason why we don't have a slash in the beginning in the mapping?

This comment has been minimized.

Copy link
@dilyand

dilyand Jun 24, 2019

Author Collaborator

Do you mean in the config? I think it's more user-friendly and less error-prone without it. Compare:

paths {
  "/com.acme/track" -> "/com.snowplowanalytics.snowplow/tp2
}

vs

paths {
  "com.acme/track" -> "com.snowplowanalytics.snowplow/tp2
}

This comment has been minimized.

Copy link
@chuwy

chuwy Jun 25, 2019

Member

That's arguable. For me /com.acme/track looks explicit in the sense that is a root-level URL. Though it is a nit, I don't have strong opinion about it.

s"/$mappingOrOriginal"
}

override def cookie(
queryString: Option[String],
body: Option[String],
@@ -127,6 +127,7 @@ package model {
final case class CollectorConfig(
interface: String,
port: Int,
paths: Map[String, String] = Map.empty[String, String],
This conversation was marked as resolved by chuwy

This comment was marked as resolved.

Copy link
@chuwy

chuwy Jun 24, 2019

Member

Default parameters generally considered a bad practice in Scala https://blog.ssanj.net/posts/2019-05-01-why-are-default-parameter-values-considered-bad-in-scala.html. Especially if they're not in the end.

This comment has been minimized.

Copy link
@dilyand

dilyand Jun 24, 2019

Author Collaborator

Changed it to Option[Map[String, String]].

This comment has been minimized.

Copy link
@chuwy

chuwy Jun 25, 2019

Member

But None is not different from empty Map?

This comment has been minimized.

Copy link
@dilyand

dilyand Jun 25, 2019

Author Collaborator

Sure, but then:

val mappings = config.paths.getOrElse(Map.empty[String, String])

This comment has been minimized.

Copy link
@chuwy

chuwy Jun 25, 2019

Member

Exactly. And we're ending up with empty map by default anyways, unless I'm missing something very obvious.

This comment has been minimized.

Copy link
@dilyand

dilyand Jun 25, 2019

Author Collaborator

Maybe I am...

Your note was about default parameters, especially when not last in the list. We are no longer using them. The paths parameter is an optional map. You cannot look for a value by key in a None, so if the map is not provided, we use an empty map instead.

Do you still have any concerns?

p3p: P3PConfig,
crossDomain: CrossDomainConfig,
cookie: CookieConfig,
@@ -44,6 +44,7 @@ class CollectorRouteSpec extends Specification with Specs2RouteTest {
): (HttpResponse, List[Array[Byte]]) = (HttpResponse(200, entity = s"cookie"), List.empty)
def cookieName: Option[String] = Some("name")
def doNotTrackCookie: Option[DntCookieMatcher] = None
def determinePath(vendor: String, version: String): String = "/p1/p2"
}
}

@@ -472,5 +472,36 @@ class CollectorServiceSpec extends Specification {
service.validMatch(invalidHost2, domain) shouldEqual false
}
}

"updateRequestPath" in {
val vendor = "com.acme"
val version1 = "track"
val version2 = "redirect"
val version3 = "iglu"

"should correctly replace the path in the request if a mapping is provided" in {
val expected1 = "/com.snowplowanalytics.snowplow/tp2"
val expected2 = "/r/tp2"
val expected3 = "/com.snowplowanalytics.iglu/v1"

service.determinePath(vendor, version1) shouldEqual expected1
service.determinePath(vendor, version2) shouldEqual expected2
service.determinePath(vendor, version3) shouldEqual expected3
}

"should pass on the original path if no mapping for it can be found" in {
val service = new CollectorService(
TestUtils.testConf.copy(paths = Map.empty[String, String]),
CollectorSinks(new TestSink, new TestSink)
)
val expected1 = "/com.acme/track"
val expected2 = "/com.acme/redirect"
val expected3 = "/com.acme/iglu"

service.determinePath(vendor, version1) shouldEqual expected1
service.determinePath(vendor, version2) shouldEqual expected2
service.determinePath(vendor, version3) shouldEqual expected3
}
}
}
}
@@ -22,6 +22,7 @@ object TestUtils {
val testConf = CollectorConfig(
interface = "0.0.0.0",
port = 8080,
paths = Map("com.acme/track" -> "com.snowplowanalytics.snowplow/tp2", "com.acme/redirect" -> "r/tp2", "com.acme/iglu" -> "com.snowplowanalytics.iglu/v1"),
p3p = P3PConfig("/w3c/p3p.xml", "NOI DSP COR NID PSA OUR IND COM NAV STA"),
CrossDomainConfig(enabled = true, List("*"), secure = false),
cookie = CookieConfig(true, "sp", 365.days, None, None),
@@ -24,6 +24,22 @@ collector {
port = {{collectorPort}}
port = ${?COLLECTOR_PORT}

# The collector responds with a cookie to requests with a path that matches the 'vendor/version' protocol.
# The expected values are:
# - com.snowplowanalytics.snowplow/tp2 for Tracker Protocol 2
# - r/tp2 for redirects
# - com.snowplowanalytics.iglu/v1 for the Iglu Webhook
# Any path that matches the 'vendor/version' protocol will result in a cookie response, for use by custom webhooks
# downstream of the collector.
# But you can optionally also map any valid (i.e. two-segment) path to one of the three defaults.
# Your custom path must be the key and the value must be one of the corresponding default paths.
# Uncomment to use. Defaults to empty map.
#paths {
# "com.acme/track" = "com.snowplowanalytics.snowplow/tp2"
This conversation was marked as resolved by BenFradet

This comment has been minimized.

Copy link
@BenFradet

BenFradet Jul 1, 2019

Contributor

I tend to agree with Anton, we're better off with fully qualified paths

This comment has been minimized.

Copy link
@dilyand

dilyand Jul 1, 2019

Author Collaborator

I've changed it to the way you guys advise.

# "com.acme/redirect" = "r/tp2"
# "com.acme/iglu" = "com.snowplowanalytics.iglu/v1"
#}

# Configure the P3P policy header.
p3p {
policyRef = "/w3c/p3p.xml"
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.