Skip to content

JSON in types #46

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

Merged
merged 1 commit into from
May 26, 2018
Merged

JSON in types #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
dist: trusty
language: scala

scala:
Expand All @@ -7,6 +8,9 @@ scala:
jdk:
- oraclejdk8

addons:
postgresql: "9.5"

services:
- postgresql

Expand Down
3 changes: 2 additions & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ val baseSettings = Seq(
"com.twitter" %% "finagle-netty3" % "18.2.0",
"org.scalatest" %% "scalatest" % "3.0.4" % "test,it",
"org.scalacheck" %% "scalacheck" % "1.13.5" % "test,it",
"org.scalamock" %% "scalamock-scalatest-support" % "3.4.2" % "test,it"
"org.scalamock" %% "scalamock-scalatest-support" % "3.4.2" % "test,it",
"io.circe" %% "circe-testing" % "0.8.0" % "test,it"
)
)

Expand Down
3 changes: 2 additions & 1 deletion src/main/scala/com/twitter/finagle/postgres/PostgresClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ object PostgresClient {

case class TypeSpecifier(receiveFunction: String, typeName: String, elemOid: Long = 0)

private[finagle] val defaultTypes = Map(
val defaultTypes = Map(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm making it public to make things slightly easier for the next person who finds that one of the types they use isn't part of defaultTypes and who doesn't want to use the fatally-bugged dynamic-type-map-building feature. Right now, if you want to use withCustomTypes, chances are that you'll have to copy this definition of defaultTypes into your code so you can have a Map that adds the mappings you need to pass to withCustomTypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I say it's "fatally-bugged" because the Future here caches any failure to pull the typeMap at startup forever, thus borking the client)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading these comments again, I think my tone may be slightly abrasive. Sorry about that!

Types.BOOL -> TypeSpecifier("boolrecv", "bool"),
Types.BYTE_A -> TypeSpecifier("bytearecv", "bytea"),
Types.CHAR -> TypeSpecifier("charrecv", "char"),
Expand All @@ -95,6 +95,7 @@ object PostgresClient {
Types.TID -> TypeSpecifier("tidrecv", "tid"),
Types.XID -> TypeSpecifier("xidrecv", "xid"),
Types.CID -> TypeSpecifier("cidrecv", "cid"),
Types.JSON -> TypeSpecifier("json_recv", "json"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I've tried to only include types in the default map that are baked-in to postgres without installing an extension. I think that's now the case for json so this should be OK. Can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON was added as a baked-in feature in Postgres 9.2 (released in 2012, so it's pretty likely that most people are on at least that version at this point).

Types.XML -> TypeSpecifier("xml_recv", "xml"),
Types.POINT -> TypeSpecifier("point_recv", "point"),
Types.L_SEG -> TypeSpecifier("lseg_recv", "lseg"),
Expand Down
1 change: 1 addition & 0 deletions src/main/scala/com/twitter/finagle/postgres/values/Types.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ object Types {
val TID = 27
val XID = 28
val CID = 29
val JSON = 114
val XML = 142
val POINT = 600
val L_SEG = 601
Expand Down
15 changes: 14 additions & 1 deletion src/test/scala/com/twitter/finagle/postgres/values/ValuesSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ import com.twitter.finagle.postgres.messages.DataRow
import com.twitter.finagle.postgres.messages.Field
import com.twitter.util.Await
import org.jboss.netty.buffer.ChannelBuffers
import org.scalacheck.Arbitrary
import org.scalacheck.{Arbitrary, Gen}
import Arbitrary.arbitrary
import org.scalatest.prop.GeneratorDrivenPropertyChecks
import io.circe.testing.instances.arbitraryJson

class ValuesSpec extends Spec with GeneratorDrivenPropertyChecks {

Expand Down Expand Up @@ -156,6 +158,17 @@ class ValuesSpec extends Spec with GeneratorDrivenPropertyChecks {
val decoded = ValueDecoder[JSONB].decodeBinary("", createBuffer(), Charset.defaultCharset()).get()
JSONB.stringify(decoded) must equal(json)
}

"parse json" in test(ValueDecoder.string, ValueEncoder.string)("json_send", "json")(
Arbitrary(
Gen.oneOf(
arbitraryJson.arbitrary.map(_.noSpaces),
arbitraryJson.arbitrary.map(_.spaces4),
arbitraryJson.arbitrary.map(_.spaces2)
).map(_.replace("\u0000", "\\u0000"))
),
client
)
}
}
}