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

Remove auto-derivation enabled by default #657

Open
atnoya opened this issue May 27, 2021 · 6 comments
Open

Remove auto-derivation enabled by default #657

atnoya opened this issue May 27, 2021 · 6 comments
Milestone

Comments

@atnoya
Copy link

atnoya commented May 27, 2021

Hi there,

This might be a bit controversial, but auto-derivation is bringing quite a few non-small problems to us, from increased compilation times, to inadvertently, codifying case class that weren't expected to be there.

We are defining the Codecs/SchemaFor explicitly, and that helps, but it's inconvenient to not have the help of the compiler to tell you which one is missing because the missing ones are auto generated.

I would rather have explicit imports for the required behaviour, like circe or zio-json:

  • import com.sksamuel.avro4s.derivation.auto._
  • import com.sksamuel.avro4s.derivation.semi._

I am aware if accepted this is a breaking change, but If accepted, I would be willing to put forward a PR.

@atnoya atnoya changed the title Remove auto-generate enabled by default Remove auto-derivation enabled by default May 27, 2021
@sksamuel sksamuel added this to the 5.0 milestone Jun 2, 2021
@sksamuel
Copy link
Owner

sksamuel commented Jun 2, 2021

I think this is a good idea but the focus should go into 5.0 which will be for Scala 3.
4.x is really just in bug fix mode as I focus on writing a scala 3 version.

@atnoya
Copy link
Author

atnoya commented Jun 2, 2021

Given the nature of the change being a breaking change, I think it is fair :) Thanks! I haven't become familiar with scala 3 yet, but If I can help let me know!

@sksamuel
Copy link
Owner

sksamuel commented Jun 6, 2021

So, just to make sure I'm on the same page.
semi auto derivation is where you go

val encoder = Encoder[Foo].

And auto is when you just request Encoder[F] and get one for everyone.

@atnoya
Copy link
Author

atnoya commented Jun 8, 2021

Yeah, pretty much, I would say:

Semiauto is where you define your typeclass instances in the companion object (usually) by calling the gen macro.

And at the calling site, e.g.:

def useMyEncoder[A: Encoder]: Unit = ???

//If encoder defined in the companion object use that. 
//If you wish to use auto derivation, just include:
//import com.sksamuel.avro4s.derivation.auto._
useMyEncoder[A]

//If you don't define an instance in the companion object of A (or include a relevant import where the Encoder is defined), or add the `auto` import, compilation will not be able to find a suitable instance, and hence, failing.

@jeffmay
Copy link

jeffmay commented Oct 7, 2021

Yea, this one bit me too. I spent days trying to figure out why the FieldMapper wasn't working. It turns out that the macro looks at the local scope at every point where the Decoder is requested. This means that you cannot define the correct FieldMapper for the model in the companion object where the schema is generated (as you do for most type-class based codecs). This is especially a problem when some models use snake_case and others do not as you cannot mix the two in a generic situation as you cannot know which type needs which FieldMapper. The fact that auto-derivation is the default and the field mapper is assumed based on local scope effectively makes the FieldMapper unusable for us.

@chollinger93
Copy link

I could like to +1 this too.

I had a use case where the schema and derived case classes did not match (think metadata w/o a shared superclass or typeclass, where some fields belong to the case class and some come from elsewhere) and the only way I saw to implement what is basically def encode(msg: A, metadata: Metadata): Record was to let avro4s generate a RecordEncoder, extract the ctx: magnolia1.CaseClass[Encoder, T] (via reflection) and handing that to a custom Encoder instance, which inherits RecordEncoder. Which feels wrong.

Perhaps there is a better way I'm not seeing, but since MagnoliaDerivedEncoder is implemented by the Encoder object, I did not see a way to simply skip that whole spiel, use magnolia (or, better, some avro4s function) directly to just grab the CaseClass[Encoder, T] and just build my own encoder. And since I'm already getting an encoder with an expensive derived magnolia object, whether I like it or not, I might as well re-use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants