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

Jump to bottom

newtype collection and item ids for consistent URL encoding / decoding #206

Open
jisantuc opened this issue Jun 26, 2020 · 1 comment
Open

newtype collection and item ids for consistent URL encoding / decoding #206

jisantuc opened this issue Jun 26, 2020 · 1 comment

Comments

@jisantuc
Copy link
Contributor

jisantuc commented Jun 26, 2020 •
edited
Loading

Improvement

currently Franklin has to ask tapir to decode all of our collection IDs and item IDs from urls as strings. this is fine, but not super descriptive (can they be empty? apparently yes! can we use item ids for collection ids? sure!)

We have the tools with newtype + refined to make sure that these values are more specific than "literally any string" and also that we can't use them interchangeably by accident. Then in Franklin, with tapir we can also define the url decoding behavior as part of request parsing.

Notes + Context

Follow up to azavea/franklin#286 because there are so many small changes
@jisantuc
Copy link
Contributor Author

jisantuc commented Jun 26, 2020 •
edited
Loading

Here's a start of what this looked like in Franklin-land, though the diff won't apply in this repo:

diff --git a/application/src/main/scala/com/azavea/franklin/api/endpoints/CollectionItemEndpoints.scala b/application/src/main/scala/com/azavea/franklin/api/endpoints/CollectionItemEndpoints.scala
index 026ca8d..71e5d52 100644
--- a/application/src/main/scala/com/azavea/franklin/api/endpoints/CollectionItemEndpoints.scala
+++ b/application/src/main/scala/com/azavea/franklin/api/endpoints/CollectionItemEndpoints.scala
@@ -1,7 +1,7 @@
 package com.azavea.franklin.api.endpoints
 
 import com.azavea.franklin.api.schemas._
-import com.azavea.franklin.datamodel.PaginationToken
+import com.azavea.franklin.datamodel.{PaginationToken, UrlSafeCollectionId, UrlSafeItemId}
 import com.azavea.franklin.error.{
   CrudError,
   InvalidPatch,
@@ -28,13 +28,13 @@ class CollectionItemEndpoints(
   val base = endpoint.in("collections")
 
   val collectionItemsList: Endpoint[
-    (String, Option[PaginationToken], Option[NonNegInt], List[ExtensionName]),
+    (UrlSafeCollectionId, Option[PaginationToken], Option[NonNegInt], List[ExtensionName]),
     Unit,
     Json,
     Nothing
   ] =
     base.get
-      .in(path[String])
+      .in(path[UrlSafeCollectionId])
       .in("items")
       .in(
         query[Option[PaginationToken]]("next")
@@ -52,27 +52,30 @@ class CollectionItemEndpoints(
       .description("A feature collection of collection items")
       .name("collectionItems")
 
-  val collectionItemsUnique: Endpoint[(String, String), NotFound, (Json, String), Nothing] =
+  val collectionItemsUnique
+      : Endpoint[(UrlSafeCollectionId, UrlSafeItemId), NotFound, (Json, String), Nothing] =
     base.get
-      .in(path[String] / "items" / path[String])
+      .in(path[UrlSafeCollectionId] / "items" / path[UrlSafeItemId])
       .out(jsonBody[Json])
       .errorOut(oneOf(statusMapping(NF, jsonBody[NotFound].description("not found"))))
       .out(header[String]("ETag"))
       .description("A single feature")
       .name("collectionItemUnique")
 
-  val collectionItemTiles: Endpoint[(String, String), NotFound, (Json, String), Nothing] =
+  val collectionItemTiles
+      : Endpoint[(UrlSafeCollectionId, UrlSafeItemId), NotFound, (Json, String), Nothing] =
     base.get
-      .in(path[String] / "items" / path[String] / "tiles")
+      .in(path[UrlSafeCollectionId] / "items" / path[UrlSafeItemId] / "tiles")
       .out(jsonBody[Json])
       .errorOut(oneOf(statusMapping(NF, jsonBody[NotFound].description("not found"))))
       .out(header[String]("ETag"))
       .description("An item's tile endpoints")
       .name("collectionItemTiles")
 
-  val postItem: Endpoint[(String, StacItem), ValidationError, (Json, String), Nothing] =
+  val postItem
+      : Endpoint[(UrlSafeCollectionId, StacItem), ValidationError, (Json, String), Nothing] =
     base.post
-      .in(path[String] / "items")
+      .in(path[UrlSafeCollectionId] / "items")
       .in(jsonBody[StacItem])
       .out(jsonBody[Json])
       .out(header[String]("ETag"))
@@ -88,9 +91,14 @@ class CollectionItemEndpoints(
       .description("Create a new feature in a collection")
       .name("postItem")
 
-  val putItem: Endpoint[(String, String, StacItem, String), CrudError, (Json, String), Nothing] =
+  val putItem: Endpoint[
+    (UrlSafeCollectionId, UrlSafeItemId, StacItem, String),
+    CrudError,
+    (Json, String),
+    Nothing
+  ] =
     base.put
-      .in(path[String] / "items" / path[String])
+      .in(path[UrlSafeCollectionId] / "items" / path[UrlSafeItemId])
       .in(jsonBody[StacItem])
       .in(header[String]("If-Match"))
       .out(jsonBody[Json])
@@ -114,15 +122,20 @@ class CollectionItemEndpoints(
         )
       )
 
-  val deleteItem: Endpoint[(String, String), Unit, Unit, Nothing] =
+  val deleteItem: Endpoint[(UrlSafeCollectionId, UrlSafeItemId), Unit, Unit, Nothing] =
     base.delete
-      .in(path[String] / "items" / path[String])
+      .in(path[UrlSafeCollectionId] / "items" / path[UrlSafeItemId])
       .out(emptyOutput)
       .out(statusCode(StatusCode.NoContent))
 
-  val patchItem: Endpoint[(String, String, Json, String), CrudError, (Json, String), Nothing] =
+  val patchItem: Endpoint[
+    (UrlSafeCollectionId, UrlSafeItemId, Json, String),
+    CrudError,
+    (Json, String),
+    Nothing
+  ] =
     base.patch
-      .in(path[String] / "items" / path[String])
+      .in(path[UrlSafeCollectionId] / "items" / path[UrlSafeItemId])
       .in(jsonBody[Json])
       .in(header[String]("If-Match"))
       .out(jsonBody[Json])
diff --git a/application/src/main/scala/com/azavea/franklin/api/implicits/package.scala b/application/src/main/scala/com/azavea/franklin/api/implicits/package.scala
index 7185e97..ace0f64 100644
--- a/application/src/main/scala/com/azavea/franklin/api/implicits/package.scala
+++ b/application/src/main/scala/com/azavea/franklin/api/implicits/package.scala
@@ -6,6 +6,7 @@ import eu.timepit.refined.types.string.NonEmptyString
 
 import java.net.URLEncoder
 import java.nio.charset.StandardCharsets
+import com.azavea.franklin.datamodel.{UrlSafeCollectionId, UrlSafeItemId}
 
 package object implicits {
 
@@ -23,7 +24,7 @@ package object implicits {
       item.copy(links = updatedLinks)
     }
 
-    def addTilesLink(apiHost: String, collectionId: String, itemId: String) = {
+    def addTilesLink(apiHost: String, collectionId: UrlSafeCollectionId, itemId: UrlSafeItemId) = {
       val cogAsset = item.assets.values.exists { asset =>
         asset._type match {
           case Some(`image/cog`) => true
@@ -32,10 +33,8 @@ package object implicits {
       }
       val updatedLinks = cogAsset match {
         case true => {
-          val encodedItemId       = URLEncoder.encode(itemId, StandardCharsets.UTF_8.toString)
-          val encodedCollectionId = URLEncoder.encode(collectionId, StandardCharsets.UTF_8.toString)
           val tileLink: StacLink = StacLink(
-            s"$apiHost/collections/$encodedCollectionId/items/$encodedItemId/tiles",
+            s"$apiHost/collections/${collectionId.encoded}/items/${itemId.encoded}/tiles",
             StacLinkType.VendorLinkType("tiles"),
             Some(`application/json`),
             Some("Tile URLs for Item")
diff --git a/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala b/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala
index 6100865..ed419a9 100644
--- a/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala
+++ b/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala
@@ -2,7 +2,7 @@ package com.azavea.franklin.api
 
 import cats.implicits._
 import com.azavea.franklin.database.{temporalExtentFromString, temporalExtentToString}
-import com.azavea.franklin.datamodel.PaginationToken
+import com.azavea.franklin.datamodel.{PaginationToken, UrlSafeCollectionId, UrlSafeItemId}
 import com.azavea.franklin.error.InvalidPatch
 import com.azavea.franklin.extensions.validation.ExtensionName
 import com.azavea.stac4s._
@@ -13,6 +13,8 @@ import sttp.tapir.json.circe._
 import sttp.tapir.{Codec, DecodeResult, Schema}
 
 import scala.util.Try
+import java.net.{URLDecoder, URLEncoder}
+import java.nio.charset.StandardCharsets
 
 package object schemas {
 
@@ -83,4 +85,19 @@ package object schemas {
 
   implicit val codecPaginationToken: Codec.PlainCodec[PaginationToken] =
     Codec.string.mapDecode(PaginationToken.decPaginationToken)(PaginationToken.encPaginationToken)
+
+  implicit val codecURLSafeCollectionId: Codec.PlainCodec[UrlSafeCollectionId] =
+    Codec.string.mapDecode(s =>
+      DecodeResult.Value(
+        UrlSafeCollectionId(URLDecoder.decode(s, StandardCharsets.UTF_8.toString))
+      )
+    )(collectionId => URLEncoder.encode(collectionId.underlying, StandardCharsets.UTF_8.toString))
+
+  implicit val codecURLSafeItemId: Codec.PlainCodec[UrlSafeItemId] =
+    Codec.string.mapDecode(s =>
+      DecodeResult.Value(
+        UrlSafeItemId(URLDecoder.decode(s, StandardCharsets.UTF_8.toString))
+      )
+    )(itemId => URLEncoder.encode(itemId.underlying, StandardCharsets.UTF_8.toString))
+
 }
diff --git a/application/src/main/scala/com/azavea/franklin/api/services/CollectionItemsService.scala b/application/src/main/scala/com/azavea/franklin/api/services/CollectionItemsService.scala
index e2500f7..75de0de 100644
--- a/application/src/main/scala/com/azavea/franklin/api/services/CollectionItemsService.scala
+++ b/application/src/main/scala/com/azavea/franklin/api/services/CollectionItemsService.scala
@@ -49,15 +49,14 @@ class CollectionItemsService[F[_]: Sync](
   val enableTiles        = apiConfig.enableTiles
 
   def listCollectionItems(
-      collectionId: String,
+      collectionId: UrlSafeCollectionId,
       token: Option[PaginationToken],
       limit: Option[NonNegInt],
       extensions: List[ExtensionName]
   ): F[Either[Unit, Json]] = {
-    val decodedId = URLDecoder.decode(collectionId, StandardCharsets.UTF_8.toString)
     for {
       items <- StacItemDao.query
-        .filter(StacItemDao.collectionFilter(decodedId))
+        .filter(StacItemDao.collectionFilter(collectionId.underlying))
         .filter(extensions)
         .page(Page(limit getOrElse defaultLimit, token))
         .transact(xa)
@@ -67,7 +66,7 @@ class CollectionItemsService[F[_]: Sync](
     } yield {
       val updatedItems = enableTiles match {
         case true =>
-          items map { item => item.addTilesLink(apiHost, collectionId, item.id) }
+          items map { item => item.addTilesLink(apiHost, collectionId, UrlSafeItemId(item.id)) }
         case _ => items
       }
       val withValidatedLinks =
@@ -92,14 +91,14 @@ class CollectionItemsService[F[_]: Sync](
   }
 
   def getCollectionItemUnique(
-      rawCollectionId: String,
-      rawItemId: String
+      collectionId: UrlSafeCollectionId,
+      itemId: UrlSafeItemId
   ): F[Either[NF, (Json, String)]] = {
-    val itemId       = URLDecoder.decode(rawItemId, StandardCharsets.UTF_8.toString)
-    val collectionId = URLDecoder.decode(rawCollectionId, StandardCharsets.UTF_8.toString)
 
     for {
-      itemOption <- StacItemDao.getCollectionItem(collectionId, itemId).transact(xa)
+      itemOption <- StacItemDao
+        .getCollectionItem(collectionId, itemId)
+        .transact(xa)
     } yield {
       Either.fromOption(
         itemOption map { item =>
@@ -116,12 +115,9 @@ class CollectionItemsService[F[_]: Sync](
   }
 
   def getCollectionItemTileInfo(
-      rawCollectionId: String,
-      rawItemId: String
+      collectionId: UrlSafeCollectionId,
+      itemId: UrlSafeItemId
   ): F[Either[NF, (Json, String)]] = {
-    val itemId       = URLDecoder.decode(rawItemId, StandardCharsets.UTF_8.toString)
-    val collectionId = URLDecoder.decode(rawCollectionId, StandardCharsets.UTF_8.toString)
-
     for {
       itemOption <- StacItemDao.getCollectionItem(collectionId, itemId).transact(xa)
     } yield {
diff --git a/application/src/main/scala/com/azavea/franklin/database/StacItemDao.scala b/application/src/main/scala/com/azavea/franklin/database/StacItemDao.scala
index 853d863..06eea43 100644
--- a/application/src/main/scala/com/azavea/franklin/database/StacItemDao.scala
+++ b/application/src/main/scala/com/azavea/franklin/database/StacItemDao.scala
@@ -2,7 +2,14 @@ package com.azavea.franklin.database
 
 import cats.data.{EitherT, OptionT}
 import cats.implicits._
-import com.azavea.franklin.datamodel.{Context, PaginationToken, SearchMethod, StacSearchCollection}
+import com.azavea.franklin.datamodel.{
+  Context,
+  PaginationToken,
+  SearchMethod,
+  StacSearchCollection,
+  UrlSafeCollectionId,
+  UrlSafeItemId
+}
 import com.azavea.franklin.extensions.paging.PagingLinkExtension
 import com.azavea.stac4s._
 import com.azavea.stac4s.syntax._
@@ -124,27 +131,30 @@ object StacItemDao extends Dao[StacItem] {
       .withUniqueGeneratedKeys[StacItem]("item")
   }
 
-  def getCollectionItem(collectionId: String, itemId: String): ConnectionIO[Option[StacItem]] =
+  def getCollectionItem(
+      collectionId: UrlSafeCollectionId,
+      itemId: UrlSafeItemId
+  ): ConnectionIO[Option[StacItem]] =
     StacItemDao.query
-      .filter(fr"id = $itemId")
-      .filter(collectionFilter(collectionId))
+      .filter(fr"id = ${itemId.underlying}")
+      .filter(collectionFilter(collectionId.underlying))
       .selectOption
 
-  private def doUpdate(itemId: String, item: StacItem): ConnectionIO[StacItem] = {
+  private def doUpdate(itemId: UrlSafeItemId, item: StacItem): ConnectionIO[StacItem] = {
     val itemExtent = Projected(item.geometry.getEnvelope, 4326)
     val fragment   = fr"""
       UPDATE collection_items
       SET
         extent = $itemExtent,
         item = $item
-      WHERE id = $itemId
+      WHERE id = ${itemId.underlying}
     """
     fragment.update.withUniqueGeneratedKeys[StacItem]("item")
   }
 
   def updateStacItem(
-      collectionId: String,
-      itemId: String,
+      collectionId: UrlSafeCollectionId,
+      itemId: UrlSafeItemId,
       item: StacItem,
       etag: String
   ): ConnectionIO[Either[StacItemDaoError, StacItem]] =
@@ -162,8 +172,8 @@ object StacItemDao extends Dao[StacItem] {
     } yield update).value
 
   def patchItem(
-      collectionId: String,
-      itemId: String,
+      collectionId: UrlSafeCollectionId,
+      itemId: UrlSafeItemId,
       jsonPatch: Json,
       etag: String
   ): ConnectionIO[Option[Either[StacItemDaoError, StacItem]]] = {
diff --git a/application/src/main/scala/com/azavea/franklin/datamodel/package.scala b/application/src/main/scala/com/azavea/franklin/datamodel/package.scala
index 014abe8..eddd926 100644
--- a/application/src/main/scala/com/azavea/franklin/datamodel/package.scala
+++ b/application/src/main/scala/com/azavea/franklin/datamodel/package.scala
@@ -6,6 +6,9 @@ import eu.timepit.refined.numeric._
 import eu.timepit.refined.types.string.NonEmptyString
 import io.circe.refined._
 import io.circe.{Decoder, Encoder}
+import io.estatico.newtype.macros.newtype
+import java.net.URLEncoder
+import java.nio.charset.StandardCharsets
 
 package object datamodel {
   implicit val decoderNonEmptyString: Decoder[NonEmptyString] = refinedDecoder
@@ -14,4 +17,12 @@ package object datamodel {
   type Quantile = Int Refined Interval.Closed[W.`0`.T, W.`100`.T]
   object Quantile extends RefinedTypeOps[Quantile, Int]
 
+  @newtype case class UrlSafeCollectionId(underlying: String) {
+    def encoded: String = URLEncoder.encode(underlying, StandardCharsets.UTF_8.toString)
+  }
+
+  @newtype case class UrlSafeItemId(underlying: String) {
+    def encoded: String = URLEncoder.encode(underlying, StandardCharsets.UTF_8.toString)
+  }
+
 }
diff --git a/build.sbt b/build.sbt
index b9b18e8..ee6e63b 100644
--- a/build.sbt
+++ b/build.sbt
@@ -117,6 +117,7 @@ lazy val applicationDependencies = Seq(
   "io.circe"                    %% "circe-parser"             % Versions.CirceVersion,
   "io.circe"                    %% "circe-refined"            % Versions.CirceVersion,
   "io.circe"                    %% "circe-testing"            % Versions.CirceVersion % Test,
+  "io.estatico"                 %% "newtype"                  % Versions.NewtypeVersion,
   "net.postgis"                 % "postgis-geometry"          % Versions.Postgis,
   "net.postgis"                 % "postgis-jdbc"              % Versions.Postgis,
   "org.flywaydb"                % "flyway-core"               % Versions.Flyway,
diff --git a/project/Versions.scala b/project/Versions.scala
index 4486985..1752af1 100644
--- a/project/Versions.scala
+++ b/project/Versions.scala
@@ -21,6 +21,7 @@ object Versions {
   val MagnoliaVersion         = "0.16.0"
   val MamlVersion             = "0.6.1"
   val MonocleVersion          = "2.0.1"
+  val NewtypeVersion          = "0.4.4"
   val Postgis                 = "2.5.0"
   val PureConfig              = "0.12.1"
   val Refined                 = "0.9.14"

@jisantuc jisantuc transferred this issue from azavea/franklin Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jisantuc