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.

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

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

Open
notxcain opened this issue Sep 19, 2024 · 10 comments
Open

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

notxcain opened this issue Sep 19, 2024 · 10 comments

Comments

@notxcain
Copy link

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
Copy link
Author

notxcain commented Sep 19, 2024

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

@jatcwang
Copy link
Collaborator

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
Copy link
Author

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

@sideeffffect
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

Thank you @jatcwang for all your hard work ❤️ Hang in there and good luck! 🍀

@jatcwang
Copy link
Collaborator

jatcwang commented Sep 28, 2024

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.. 🔨

@jsoref
Copy link

jsoref commented Oct 8, 2024

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
Copy link
Collaborator

jatcwang commented Oct 8, 2024

Good idea thanks. Done :)

@jatcwang
Copy link
Collaborator

Just a quick update. The rework on Write/Read seems to have panned out and there's a clear path forward, I'm working on having it it all working across Scala 2 & 3 + better test coverage of the edge cases

jatcwang added a commit that referenced this issue Nov 12, 2024
Fix both semiauto and automatic derivation to use custom defined Read/Write instances (e.g. in companion objects).

A complete rework of Read and Write is unfortunately necessary because with the previous implementation, we cannot simply derive a `Read[Option[A]]` given a `Read[A]` - we'd need to derive `Option[A]` from scratch by resolving `Read[Option[X]]` instances for each of `A`'s columns.

After the rework, both `Read` and `Write` are now sealed traits, each with 3 subclasses:
- Single: Wrapper over a `Get/Put`
- SingleOpt: Wrapper over a `Get/Put`, but is nullable i.e. `Read[Option[A]]`, `Write[Option[A]]`
- Composite: A composite of `Read/Write` instances

Apart from enabling proper semiauto and automatic derivation, the rework also brings these benefits:

- Derivation rules are much simpler (which also translates to faster compile times). In particular,
 given a `Read/Write[A]` we can trivially derive `Read/Write[Option[A]]`.
- We now correctly handle optionality for `Option[CaseClassWithOptionalAndNonOptionalFields]`. More tests will be added for this in a follow up PR to demonstrate

Other notes:
- Semiauto and Auto derivation of unary product type (e.g. 1 element case class) are removed due to it causing auto derivation to pick the wrong path. It seems unnecessary since
  Write/Read derivation will yield the same behaviour anyway?

Fixes #1054, #2104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants