Encoders and decoders refactoring #614
Conversation
|
Isn't this something that is better solved by addressing the core problem rather than reverting #588 ? If the intention is to revert #588 then we should just revert the commit or discuss the issues there rather than creating a new pull request that is just reverting an older one. I feel this is better solved by detecting why the |
|
@mdedetrich I did not revert changes from your pull request and there was no such intent. I just specified correct types for encoders and decoders back to implicit val stringEncoder: Encoder[String] = encoder[String](SqlTypes.VARCHAR)to implicit val stringEncoder = encoder[String](SqlTypes.VARCHAR)and you'll see that added test is failing. |
|
@mdedetrich If you'll look at implicit val stringEncoder: Encoder[String] = encoder(_.setString, Types.VARCHAR)so I did just the same for |
Nothing wrong, but the intention of the pull request is that its possible to get any SQL primitive type out of an I need to check if this actually effects me with the plugin I am making, it may not be an issue but Quill does do some weird subtyping stuff in this area I am just actually more surprised as to why this is coming up |
|
@mdedetrich You can get SQL primitive type by pattern matching as before because for example type of value assigned to |
|
@mdedetrich Have you checked your plugin? I'm asking because I have two projects that I can not compile because of this issue and it's hard to roll them back to |
|
@mxl I will have time to look at it tonight, is that okay? |
|
@mdedetrich Ok. |
|
@mxl Ok so here is the issue. Assuming that we change implicit val stringDecoder: AsyncDecoder[String] = decoder[String](PartialFunction.empty, SqlTypes.VARCHAR)to implicit val stringDecoder: Decoder[String] = decoder[String](PartialFunction.empty, SqlTypes.VARCHAR)The intention of the val decoder = implicitly[AsyncDecoder[String]]Fails. Now we can just summon the val decoder = implicitly[Decoder[String]]The issue here is that we now can't access the Of course, we can do something like this val decoder = implicitly[Decoder[String]].asInstanceOf[AsyncDecoder]
decoder.sqlTypeHowever that is a bit hacky. I think I am willing to live with this, but I still have a very strong suspicion that something else may be amiss here which is the actual root of the problem |
|
@fwbrasil We discussed this issue with @mdedetrich and we need your help here. implicit def dbTraversableEncoder[T](implicit encoder: AsyncEncoder[T]): AsyncEncoder[Traversable[T]] =
...and because of some driver implementation details I need exactly Actually now I agree that we should solve the core problem. I tried following. In Then in implicit val stringEncoder: Encoder[String] = encoder[String](SqlTypes.VARCHAR)to implicit val stringEncoder = encoder[String](SqlTypes.VARCHAR)Running tests results in:
I searched for And finally I wrote simple case that illustrates the issue. |
|
@mxl Thanks for the stackoverflow link! I was trying to avoid a refactoring of the encoder definitions but it seems necessary. My proposal is:
type Index = Int
type Encoder[T] <: (Index, T, PrepareRow) => PrepareRow
type Decoder[T] <: (Index, ResultRow) => T
case class AsyncEncoder[T](sqlType: SqlTypes.SqlTypes)(implicit encoder: Encoder[T]) extends ((Index, T, PrepareRow) => PrepareRow) {
override def apply(index: Index, value: T, row: PrepareRow) = ...
...
}
type Encoder[T] = AsyncEncoder[T]
// the same for `Decoder`It seems that it should work: https://scalafiddle.io/sf/2N1qodh/11. This also would allow us to remove the ugly hack that requires the contexts to override @mxl @mdedetrich what do you think? |
|
All good from me 👍 |
|
@mxl or @mdedetrich would you be interested in working on it? |
|
@fwbrasil Let me try to finish it in this pull request. |
mxl
changed the title
|
@fwbrasil Done. |
# Conflicts: # quill-core/src/main/scala/io/getquill/dsl/MetaDslMacro.scala
|
|
||
| trait LowPriorityImplicits { | ||
| this: EncodingDsl => | ||
|
|
||
| implicit def materializeEncoder[T]: Encoder[T] = macro EncodingDslMacro.materializeEncoder[T] | ||
| implicit def materializeDecoder[T]: Decoder[T] = macro EncodingDslMacro.materializeDecoder[T] | ||
| implicit def materializeEncoder[T <: AnyVal]: Encoder[T] = macro EncodingDslMacro.materializeEncoder[T] |
There was a problem hiding this comment.
Could you clarify why you needed to add the <: AnyVal constraint? The materializers also work for AnyRefs
There was a problem hiding this comment.
Without <: AnyVal constraint it conflicts with implicit def mappedEncoder when resolving implicits. Try to remove constraint and run tests.
There was a problem hiding this comment.
Are you sure that it works with AnyRefs? It looks like it fails if type is not AnyVal.
|
|
||
| implicit def mappedEncoder[I, O](implicit mapped: MappedEncoding[I, O], encoder: Encoder[O]): Encoder[I] = | ||
| mappedEncoderImpl[I, O] | ||
| def mappedBaseEncoder[I, O](mapped: MappedEncoding[I, O], encoder: BaseEncoder[O]): BaseEncoder[I] = |
There was a problem hiding this comment.
could this be protected?
| def apply(index: Int, value: I, row: PrepareRow) = | ||
| encoder(index, mapped.f(value), row) | ||
| } | ||
| def mappedBaseDecoder[I, O](mapped: MappedEncoding[I, O], decoder: BaseDecoder[I]): BaseDecoder[O] = |
There was a problem hiding this comment.
could this be protected?
Fixes #611
Problem
In #588 encoders and decoders type were changed to
AsyncEncoderandAsyncDecoderso if I have custom typecase class EncodingTestType(value: String)and table with field of that type and I trying to insert values into this table inside singleton object then compilation of this code fails. For a detailed example see test case in this pull request.Solution
Change back types to
EncoderandDecoder.Checklist
README.mdif applicable[WIP]to the pull request title if it's work in progresssbt scalariformFormat test:scalariformFormatto make sure that the source files are formatted@getquill/maintainers