typelevel / doobie

Functional JDBC layer for Scala.
MIT License
2.17k stars 356 forks source link

1.0.0-RC6: `import doobie.implicits._` is not source compatible. #2104

Open notxcain opened 1 month ago

notxcain commented 1 month ago

It looks like the imported auto-derivation takes precedence over explicitly defined instances available in scope when Option is involved.

Reproducer:

Scastie

case class Person(
  id: Int,
  name: String,
  address: Option[Address]
)

case class Address(
  city: String,
  street: String,
  zip: Int
)

trait Codecs {
  import doobie._

  // Ignore the correctness
  implicit val addressGet: Get[Address] = 
    Get[String].map(_ => Address("", "", 1))
}

import doobie._
import doobie.implicits._

object Dao extends Codecs {
  val autoderived: Read[Person] = Read[Person]
}

assert(Dao.autoderived.length == 3)

// Dao.autoderived.length is 5
notxcain commented 1 month ago

Managed to write an even shorter reproducer. The issue is with the combination of a field wrapped in Option and auto-derivation.

jatcwang commented 1 month ago

Thank you for the report & repro @notxcain! Someone else have the same issue too https://discord.com/channels/632277896739946517/632727524434247691/1286285929690169415

I think I know what needs to be done to fix this 🔨

notxcain commented 1 month ago

Awesome! Thanks for the enormous effort with the RC6 🤝🏼

sideeffffect commented 3 weeks ago

Hello @jatcwang , at our company, we're loving users of Doobie. We were considering to update to RC6, but it seems like it's better to wait for RC7. Do you think RC7 will be released sometime this or next week?

jatcwang commented 3 weeks ago

Hey @sideeffffect I'm knee deep in implicit prioritization land right now trying to sort it out 😄 Yes I think waiting for RC7 is the better idea unless you're not dependent on any custom Meta/Read/Write instances.

I'm currently trying a variation of Circe's Exported to fix this. Can't promise a timeline though sorry - These things either work or don't. Will push some changes for ideas/review if I'm stuck.

sideeffffect commented 3 weeks ago

Thank you @jatcwang for all your hard work :heart: Hang in there and good luck! :four_leaf_clover:

jatcwang commented 3 weeks ago

So..the good news is the general approach works. However, while testing I discovered that because doobie has always relied on auto-derivation. You can not simply take a Read/Write[A] instance and produce Read/Write[Option[A]]. The latter have to be completely derived from scratch. That's something we can change but involves changing the implementation of Write / Read.. :hammer:

jsoref commented 1 week ago

Would you consider marking 1.0.0-RC5 as the latest release as another way to discourage people from looking at 1.0.0-RC6?

https://github.com/typelevel/doobie/releases/edit/v1.0.0-RC5

image
jatcwang commented 1 week ago

Good idea thanks. Done :)