From e2a799696d22c2beebc846a08ed59328754942fd Mon Sep 17 00:00:00 2001 From: Eric Roberts Date: Fri, 8 Jun 2018 15:07:04 -0400 Subject: [PATCH] Refactor test suite (close #17) --- README.md | 29 ++-- build.sbt | 2 +- project/Dependencies.scala | 10 +- .../IpLookups.scala | 139 +++++++++--------- .../LruMap.scala | 93 ++++++++++++ .../SpecializedReader.scala | 9 +- .../IpLookupsTest.scala | 11 +- 7 files changed, 197 insertions(+), 96 deletions(-) create mode 100644 src/main/scala/com.snowplowanalytics.maxmind.iplookups/LruMap.scala diff --git a/README.md b/README.md index de168aa..bfc8661 100644 --- a/README.md +++ b/README.md @@ -41,17 +41,28 @@ Here is a simple usage example, performing just a geographic lookup and not the connection type lookups: ```scala +import cats.effect.IO import com.snowplowanalytics.maxmind.iplookups.IpLookups -val ipLookups = IpLookups(geoFile = Some("/opt/maxmind/GeoLite2-City.mmdb"), ispFile = None, - domainFile = None, connectionTypeFile = None, memCache = false, lruCache = 20000) - -ipLookups.performLookups("213.52.50.8").ipLocation match { - case Right(loc) => - println(loc.countryCode) // => "NO" - println(loc.countryName) // => "Norway" - case Left(f) => - println(f) +val result = (for { + ipLookups <- IpLookups.createFromFilenames[IO]( + geoFile = Some("/opt/maxmind/GeoLite2-City.mmdb") + ispFile = None, + domainFile = None, + connectionTypeFile = None, + memCache = false, + lruCacheSize = 20000 + ) + + lookup <- ipLookups.performLookups[IO]("175.16.199.0") +} yield lookup).unsafeRunSync() + +result.ipLocation match { + case Some(Right(loc)) => + println(loc.countryCode) // => "CN" + println(loc.countryName) // => "China" + case _ => + println("Lookup failed") } ``` diff --git a/build.sbt b/build.sbt index db0e1b0..ef1029d 100644 --- a/build.sbt +++ b/build.sbt @@ -31,7 +31,7 @@ lazy val root = project .settings( libraryDependencies ++= Seq( Dependencies.maxmind, - Dependencies.collUtils, + Dependencies.catsEffect, Dependencies.cats, Dependencies.scalaz, Dependencies.specs2, diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 83b5957..c34d7e6 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -13,10 +13,8 @@ import sbt._ object Dependencies { - val maxmind = "com.maxmind.geoip2" % "geoip2" % "2.11.0" - val collUtils = "com.twitter" %% "util-collection" % "18.2.0" - val scalaz = "org.scalaz" %% "scalaz-core" % "7.0.9" - val catsEffect = "org.typelevel" %% "cats-effect" % "0.10.1" - val cats = "org.typelevel" %% "cats-core" % "1.1.0" - val specs2 = "org.specs2" %% "specs2-core" % "4.0.3" % "test" + val maxmind = "com.maxmind.geoip2" % "geoip2" % "2.11.0" + val catsEffect = "org.typelevel" %% "cats-effect" % "0.10.1" + val cats = "org.typelevel" %% "cats-core" % "1.1.0" + val specs2 = "org.specs2" %% "specs2-core" % "4.0.3" % "test" } diff --git a/src/main/scala/com.snowplowanalytics.maxmind.iplookups/IpLookups.scala b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/IpLookups.scala index 789699a..59dcf1a 100644 --- a/src/main/scala/com.snowplowanalytics.maxmind.iplookups/IpLookups.scala +++ b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/IpLookups.scala @@ -14,14 +14,15 @@ package com.snowplowanalytics.maxmind.iplookups import java.io.File import java.net.InetAddress +import java.util.{Collections, Map} import com.maxmind.db.CHMCache import com.maxmind.geoip2.model.CityResponse import com.maxmind.geoip2.DatabaseReader -import com.twitter.util.SynchronizedLruMap +import cats.effect.Sync import cats.syntax.either._ -import cats.effect.IO -import scalaz._ +import cats.syntax.flatMap._ +import cats.syntax.functor._ import model._ @@ -36,25 +37,31 @@ object IpLookups { * @param domainFile Domain lookup database file * @param connectionTypeFile Connection type lookup database file * @param memCache Whether to use MaxMind's CHMCache - * @param lruCache Maximum size of SynchronizedLruMap cache + * @param lruCacheSize Maximum size of LruMap cache */ - def createFromFiles( + def createFromFiles[F[_]: Sync]( geoFile: Option[File] = None, ispFile: Option[File] = None, domainFile: Option[File] = None, connectionTypeFile: Option[File] = None, memCache: Boolean = true, - lruCache: Int = 10000 - ): IO[IpLookups] = IO { - new IpLookups( - geoFile, - ispFile, - domainFile, - connectionTypeFile, - memCache, - lruCache - ) - } + lruCacheSize: Int = 10000 + ): F[IpLookups] = + ( + if (lruCacheSize >= 0) + Sync[F].map(LruMap.create[F, String, IpLookupResult](lruCacheSize))(Some(_)) + else Sync[F].pure(None) + ).flatMap((lruCache) => + Sync[F].delay { + new IpLookups( + geoFile, + ispFile, + domainFile, + connectionTypeFile, + memCache, + lruCache + ) + }) /** * Alternative constructor taking Strings rather than Files @@ -64,23 +71,23 @@ object IpLookups { * @param domainFile Domain lookup database filepath * @param connectionTypeFile Connection type lookup database filepath * @param memCache Whether to use MaxMind's CHMCache - * @param lruCache Maximum size of SynchronizedLruMap cache + * @param lruCacheSize Maximum size of LruMap cache */ - def createFromFilenames( + def createFromFilenames[F[_]: Sync]( geoFile: Option[String] = None, ispFile: Option[String] = None, domainFile: Option[String] = None, connectionTypeFile: Option[String] = None, memCache: Boolean = true, - lruCache: Int = 10000 - ): IO[IpLookups] = + lruCacheSize: Int = 10000 + ): F[IpLookups] = IpLookups.createFromFiles( geoFile.map(new File(_)), ispFile.map(new File(_)), domainFile.map(new File(_)), connectionTypeFile.map(new File(_)), memCache, - lruCache + lruCacheSize ) } @@ -98,18 +105,16 @@ object IpLookups { * https://github.com/jt6211/hadoop-dns-mining/blob/master/src/main/java/io/covert/dns/geo/IpLookups.java */ class IpLookups private ( - geoFile: Option[File] = None, - ispFile: Option[File] = None, - domainFile: Option[File] = None, - connectionTypeFile: Option[File] = None, - memCache: Boolean = true, - lruCache: Int = 10000 + geoFile: Option[File], + ispFile: Option[File], + domainFile: Option[File], + connectionTypeFile: Option[File], + memCache: Boolean, + lruCache: Option[LruMap[String, IpLookupResult]] ) { // Initialise the cache - private val lru = - if (lruCache > 0) Some(new SynchronizedLruMap[String, IpLookupResult](lruCache)) - else None // Of type mutable.Map[String, LookupData] + private val lru = lruCache // Configure the lookup services private val geoService = getService(geoFile) @@ -140,17 +145,17 @@ class IpLookups private ( * @param service ISP, domain or connection type LookupService * @return the result of the lookup */ - private def getLookup( - ipAddress: Validation[Throwable, InetAddress], + private def getLookup[F[_]: Sync]( + ipAddress: Either[Throwable, InetAddress], service: Option[SpecializedReader] - ): IO[Option[Validation[Throwable, String]]] = + ): F[Option[Either[Throwable, String]]] = (ipAddress, service) match { - case (Success(ipA), Some(svc)) => - svc.getValue(ipA).map(Some(_)) - case (Failure(f), _) => - IO.pure(Some(Failure(f))) + case (Right(ipA), Some(svc)) => + Sync[F].map(svc.getValue(ipA))(Some(_)) + case (Left(f), _) => + Sync[F].pure(Some(Left(f))) case _ => - IO.pure(None) + Sync[F].pure(None) } /** @@ -158,20 +163,20 @@ class IpLookups private ( * as an IpLocation, or None if MaxMind cannot find * the location. */ - val performLookups: String => IO[IpLookupResult] = (s: String) => + def performLookups[F[_]: Sync](s: String): F[IpLookupResult] = lru .map(performLookupsWithLruCache(_, s)) .getOrElse(performLookupsWithoutLruCache(s)) - private def getLocationLookup( - ipAddress: Validation[Throwable, InetAddress] - ): IO[Option[Validation[Throwable, IpLocation]]] = (ipAddress, geoService) match { - case (Success(ipA), Some(gs)) => - (getCityResponse(gs, ipA)).map( + private def getLocationLookup[F[_]: Sync]( + ipAddress: Either[Throwable, InetAddress] + ): F[Option[Either[Throwable, IpLocation]]] = (ipAddress, geoService) match { + case (Right(ipA), Some(gs)) => + Sync[F].map(getCityResponse(gs, ipA))( (loc) => Some(loc.map(IpLocation(_))) ) - case (Failure(f), _) => IO.pure(Some(Failure(f))) - case _ => IO.pure(None) + case (Left(f), _) => Sync[F].pure(Some(Left(f))) + case _ => Sync[F].pure(None) } /** @@ -184,7 +189,7 @@ class IpLookups private ( * @return Tuple containing the results of the * LookupServices */ - private def performLookupsWithoutLruCache(ip: String): IO[IpLookupResult] = { + private def performLookupsWithoutLruCache[F[_]: Sync](ip: String): F[IpLookupResult] = for { ipAddress <- getIpAddress(ip) @@ -194,7 +199,6 @@ class IpLookups private ( domain <- getLookup(ipAddress, domainService) connectionType <- getLookup(ipAddress, connectionTypeService) } yield IpLookupResult(ipLocation, isp, org, domain, connectionType) - } /** * Returns the MaxMind location for this IP address @@ -207,33 +211,28 @@ class IpLookups private ( * cache entry could be found), versus an extant cache entry * containing None (meaning that the IP address is unknown). */ - private def performLookupsWithLruCache( - lru: SynchronizedLruMap[String, IpLookupResult], + private def performLookupsWithLruCache[F[_]: Sync]( + lru: LruMap[String, IpLookupResult], ip: String - ): IO[IpLookupResult] = { - val lookupAndCache = for { - result <- performLookupsWithoutLruCache(ip) - _ <- putFromMap(lru, ip, result) - } yield result - - getFromMap(lru, ip) - .map(_.map(IO.pure(_))) + ): F[IpLookupResult] = { + val lookupAndCache = + performLookupsWithoutLruCache(ip).flatMap(result => { + LruMap.put(lru, ip, result).map(_ => result) + }) + + LruMap + .get(lru, ip) + .map(_.map(Sync[F].pure(_))) .flatMap(_.getOrElse(lookupAndCache)) } - /** Transforms a String into an Validation[Throwable, InetAddress] */ - private def getIpAddress(ip: String): IO[Validation[Throwable, InetAddress]] = - IO { Validation.fromTryCatch(InetAddress.getByName(ip)) } + /** Transforms a String into an Either[Throwable, InetAddress] */ + private def getIpAddress[F[_]: Sync](ip: String): F[Either[Throwable, InetAddress]] = + Sync[F].delay { Either.catchNonFatal(InetAddress.getByName(ip)) } - private def getCityResponse( + private def getCityResponse[F[_]: Sync]( gs: DatabaseReader, ipAddress: InetAddress - ): IO[Validation[Throwable, CityResponse]] = - IO { Validation.fromTryCatch(gs.city(ipAddress)) } - - private def getFromMap[K, V](map: SynchronizedLruMap[K, V], k: K): IO[Option[V]] = - IO { map.get(k) } - - private def putFromMap[K, V](map: SynchronizedLruMap[K, V], k: K, v: V): IO[Unit] = - IO { map.put(k, v) } + ): F[Either[Throwable, CityResponse]] = + Sync[F].delay { Either.catchNonFatal(gs.city(ipAddress)) } } diff --git a/src/main/scala/com.snowplowanalytics.maxmind.iplookups/LruMap.scala b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/LruMap.scala new file mode 100644 index 0000000..20c743d --- /dev/null +++ b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/LruMap.scala @@ -0,0 +1,93 @@ +/* + * Copyright (c) 2012-2018 Snowplow Analytics Ltd. All rights reserved. + * + * This program is licensed to you under the Apache License Version 2.0, + * and you may not use this file except in compliance with the Apache License Version 2.0. + * You may obtain a copy of the Apache License Version 2.0 at http://www.apache.org/licenses/LICENSE-2.0. + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the Apache License Version 2.0 is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the Apache License Version 2.0 for the specific language governing permissions and limitations there under. + */ +package com.snowplowanalytics.maxmind.iplookups + +import cats.effect.Sync +import java.{util => ju} +import java.util.LinkedHashMap +import scala.collection.JavaConverters._ +import scala.collection.mutable.{Map, MapLike, SynchronizedMap} + +// Based on com.twitter.util.LruMap +// https://github.com/twitter/util/blob/develop/util-collection/src/main/scala/com/twitter/util/LruMap.scala + +/** + * A wrapper trait for java.util.Map implementations to make them behave as scala Maps. + * This is useful if you want to have more specifically-typed wrapped objects instead + * of the generic maps returned by JavaConverters + */ +trait JMapWrapperLike[A, B, +Repr <: MapLike[A, B, Repr] with Map[A, B]] + extends Map[A, B] + with MapLike[A, B, Repr] { + def underlying: ju.Map[A, B] + + override def size = underlying.size + + override def get(k: A) = underlying.asScala.get(k) + + override def +=(kv: (A, B)): this.type = { underlying.put(kv._1, kv._2); this } + override def -=(key: A): this.type = { underlying remove key; this } + + override def put(k: A, v: B): Option[B] = underlying.asScala.put(k, v) + + override def update(k: A, v: B): Unit = underlying.put(k, v) + + override def remove(k: A): Option[B] = underlying.asScala.remove(k) + + override def clear() = underlying.clear() + + override def empty: Repr = null.asInstanceOf[Repr] + + override def iterator = underlying.asScala.iterator +} + +object LruMap { + def create[F[_]: Sync, K, V]( + size: Int + ): F[LruMap[K, V]] = implicitly[Sync[F]].delay { + new LruMap[K, V](size) + } + + def put[F[_]: Sync, K, V]( + lruMap: LruMap[K, V], + key: K, + value: V + ): F[Unit] = implicitly[Sync[F]].delay { + lruMap.put(key, value) + } + + def get[F[_]: Sync, K, V](lruMap: LruMap[K, V], key: K): F[Option[V]] = + implicitly[Sync[F]].delay { + lruMap.get(key) + } + + // initial capacity and load factor are the normal defaults for LinkedHashMap + def makeUnderlying[K, V](maxSize: Int): ju.Map[K, V] = + new LinkedHashMap[K, V]( + 16, /* initial capacity */ + 0.75f, /* load factor */ + true /* access order (as opposed to insertion order) */ + ) { + override protected def removeEldestEntry(eldest: ju.Map.Entry[K, V]): Boolean = + this.size() > maxSize + } +} + +/** + * A scala `Map` backed by a [[java.util.LinkedHashMap]] + */ +class LruMap[K, V](val maxSize: Int, val underlying: ju.Map[K, V]) + extends JMapWrapperLike[K, V, LruMap[K, V]] { + override def empty: LruMap[K, V] = new LruMap[K, V](maxSize) + def this(maxSize: Int) = this(maxSize, LruMap.makeUnderlying(maxSize)) +} diff --git a/src/main/scala/com.snowplowanalytics.maxmind.iplookups/SpecializedReader.scala b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/SpecializedReader.scala index cde9c9e..9968030 100644 --- a/src/main/scala/com.snowplowanalytics.maxmind.iplookups/SpecializedReader.scala +++ b/src/main/scala/com.snowplowanalytics.maxmind.iplookups/SpecializedReader.scala @@ -18,13 +18,12 @@ import com.maxmind.geoip2.DatabaseReader import cats.syntax.either._ import com.snowplowanalytics.maxmind.iplookups.ReaderFunctions.ReaderFunction -import cats.effect.IO -import scalaz._ +import cats.effect.Sync +import cats.syntax.either._ final case class SpecializedReader(db: DatabaseReader, f: ReaderFunction) { - def getValue(ip: InetAddress): IO[Validation[Throwable, String]] = - IO { Validation.fromTryCatch(f(db, ip)) } -} + def getValue[F[_]: Sync](ip: InetAddress): F[Either[Throwable, String]] = + Sync[F].delay { Either.catchNonFatal(f(db, ip)) } object ReaderFunctions { type ReaderFunction = (DatabaseReader, InetAddress) => String diff --git a/src/test/scala/com.snowplowanalytics.maxmind.iplookups/IpLookupsTest.scala b/src/test/scala/com.snowplowanalytics.maxmind.iplookups/IpLookupsTest.scala index e6acd59..03512b0 100644 --- a/src/test/scala/com.snowplowanalytics.maxmind.iplookups/IpLookupsTest.scala +++ b/src/test/scala/com.snowplowanalytics.maxmind.iplookups/IpLookupsTest.scala @@ -18,6 +18,7 @@ import com.maxmind.geoip2.exception.AddressNotFoundException import org.specs2.mutable.Specification import cats.syntax.either._ import cats.syntax.option._ +import cats.effect.IO import model._ @@ -30,7 +31,7 @@ object IpLookupsTest { val connectionTypeFile = getClass.getResource("GeoIP2-Connection-Type-Test.mmdb").getFile IpLookups - .createFromFilenames( + .createFromFilenames[IO]( Some(geoFile), Some(ispFile), Some(domainFile), @@ -134,7 +135,7 @@ class IpLookupsTest extends Specification { testData foreach { case (ip, expected) => formatter(ip, memCache, lruCache) should { - val actual = ipLookups.performLookups(ip).unsafeRunSync() + val actual = ipLookups.performLookups[IO](ip).unsafeRunSync() matchIpLookupResult(actual, expected) } } @@ -149,14 +150,14 @@ class IpLookupsTest extends Specification { new UnknownHostException("not: Name or service not known").asLeft.some, new UnknownHostException("not: Name or service not known").asLeft.some ) - val actual = ipLookups.performLookups("not").unsafeRunSync + val actual = ipLookups.performLookups[IO]("not").unsafeRunSync matchIpLookupResult(actual, expected) } "providing no files should return Nones" in { val actual = (for { - ipLookups <- IpLookups.createFromFiles(None, None, None, None, true, 0) - res <- ipLookups.performLookups("67.43.156.0") + ipLookups <- IpLookups.createFromFiles[IO](None, None, None, None, true, 0) + res <- ipLookups.performLookups[IO]("67.43.156.0") } yield res).unsafeRunSync val expected = IpLookupResult(None, None, None, None, None) matchIpLookupResult(actual, expected)