Support custom JdbcEncoder when creating sql segment #131
Conversation
| import scala.language.implicitConversions | ||
|
|
||
| package object jdbc { | ||
| package object jdbc extends LowPriorityImplicits1 { |
There was a problem hiding this comment.
It's not great practice to use package objects because of their removal in Scala 3.
I think we can solve this problem in another way:
By using JdbcEncoder, and deleting Setter.
In order to do this, we would enhance JdbcDecoder with two new methods like so:
def unsafeEncode(columIndex: Int, ps: PreparedStatement, value: A): Int
def unsafeEncodeNull(columIndex: Int, ps: PreparedStatement): Intand implement them for all existing JdbcEncoder.
These are essentially Setter#setValue / setNull, except the functions return the output index, because it may require more than 1 index to write the full value.
What do you think?
There was a problem hiding this comment.
I wonder if JdbcEncode and Setter could be combined under one sealed trait.
From what I understand Setter is a bit safer since it uses PreparedStatement.
I will try some experiments today.
EDIT: All right, I get the unsafeEncode now, it's like in Decoder:
trait JdbcDecoder[+A] { self =>
def unsafeDecode(columIndex: Int, rs: ResultSet): (Int, A)
}There was a problem hiding this comment.
All right, I've combined Setter into JdbcDecoder and I think it looks pretty good.
It's not finished - there is some refactoring to do. But you can already check if the idea is fine in your opinion.
Combine Setter into JdbcEncoder
| def encode(value: A): SqlFragment | ||
| val setter: Option[Setter[A]] |
There was a problem hiding this comment.
First, I would inline Setter, as there is no need for a separate type class.
Second, why make it Option? Are there some cases that have no well-defined implementation?
There was a problem hiding this comment.
Well, the one from issue does not:
implicit val byteArrayjdbcEncoder: JdbcEncoder[Array[Byte]] =
value => s"FROM_BASE64('${encoder.encodeToString(value)}')"
I mean - setter can be set as required, but encoder won't be so simple.
Maybe it's better - after all these kind of encoders are open to SQL injection attacks.
There was a problem hiding this comment.
So after seeing support for iterable I had an epiphany how it may work - I hope API is now what you wanted.
…encoder_in_segment # Conflicts: # core/src/main/scala/zio/jdbc/ZConnection.scala
| override def sql(a: B): String = "?" | ||
| } | ||
|
|
||
| def sql(value: A): String |
There was a problem hiding this comment.
I think this should be implemented by calling encode and then rendering that SqlFragment to a SQL string.
| @@ -84,6 +83,8 @@ sealed trait SqlFragment { self => | |||
| def notIn[B](b: B, bs: B*)(implicit encoder: JdbcEncoder[B]): SqlFragment = | |||
| notIn(b +: bs) | |||
|
|
|||
| def and(): SqlFragment = self ++ SqlFragment.and | |||
There was a problem hiding this comment.
| def and(): SqlFragment = self ++ SqlFragment.and | |
| def and: SqlFragment = self ++ SqlFragment.and |
| def apply[A]( | ||
| onEncode: A => SqlFragment, | ||
| onSql: => String, | ||
| onValue: (PreparedStatement, Int, A) => Unit, |
There was a problem hiding this comment.
This is not correct, it must return an Int for the next index, to capture how many placeholders it used in the prepared statement
| tuple => JdbcEncoder[A]().encode(tuple._1) ++ SqlFragment.comma ++ JdbcEncoder[B]().encode(tuple._2) | ||
| override def unsafeSetValue(ps: PreparedStatement, index: Int, value: A): Int = { | ||
| onValue(ps, index, value) | ||
| index + 1 |
There was a problem hiding this comment.
You can see this method falsely claims it uses 1 placeholder, but in fact, onValue could consume many placeholders. Hence the need to change the type of onValue to be accurate.
| ) ++ SqlFragment.comma ++ JdbcEncoder[C]().encode(tuple._3) | ||
| override def unsafeSetNull(ps: PreparedStatement, index: Int): Int = { | ||
| onNull(ps, index) | ||
| index + 1 |
There was a problem hiding this comment.
Again, onNull must return an int, because what if we are talking about a composite JdbEncoder, e.g. Person(name, age), and then when you set that to null, you actually have to set 2 placeholders to null.
| implicit def singleParamEncoder[A: SqlFragment.Setter]: JdbcEncoder[A] = value => sql"$value" | ||
| def apply[A]( | ||
| onEncode: A => SqlFragment, | ||
| onSql: => String, |
There was a problem hiding this comment.
Best to generalize this to take the A:
| onSql: => String, | |
| onSql: A => String, |
There was a problem hiding this comment.
Or better yet, better to derive the implementation and make it final in the trait so users don't have to specify this (because you can go from A to SqlFragment, and then from SqlFragment to String, automatically).
|
@andrzejressel Please re-open if you have the desire to push this one forward, and thanks for your prototyping work here! |
This PR allows JdbcEncoder to be used in sql interpolator using
Param.NestedBecause they are a lot of issues regarding JdbcEncoder/Setter priority, I've added comment for users that want custom encoding for already existing types.
Fixes #83
/claim #83