avro-kotlin / avro4k

Avro format support for Kotlin
Apache License 2.0
188 stars 36 forks source link

Polymorphic open support #109

Closed williamboxhall closed 2 years ago

williamboxhall commented 2 years ago

Huge disclaimer: I don't know kotlinx or avro4k internals at all so there's a good chance this implementation is not on the right track. It was also done under delivery time pressure so forgive me for not doing deeper homework on this. Happy to take any pointers on how to improve it.

This PR adds support for open / unsealed / abstract classes, configured like so:

val module = SerializersModule {
   polymorphic(UnsealedPolymorphicRoot::class) {
      subclass(UnsealedChildOne::class)
      subclass(UnsealedChildTwo::class)
   }
}
val schema = Avro(serializersModule = module).schema(UnsealedPolymorphicRoot.serializer())

For now it explicitly doesn't support polymorphic defaults - this is a feature someone could build on top if they want to.

williamboxhall commented 2 years ago

Not sure on the process here but tagging @thake if he has a little bit of time 🙏🏻

thake commented 2 years ago

@williamboxhall thanks for your contribution :) That was fast. I just had a quick look at the code. Can you add 2 more tests to the patch before I go into a deeper review? The first one tests the serialization/deserialization and the second one tests the schema for polymorphic references.

williamboxhall commented 2 years ago

The first one tests the serialization/deserialization

This makes sense, happy to do it. Is there another good test I can use as a reference for this?

the second one tests the schema for polymorphic references

I don't quite follow what you mean by this, would you be able to explain in a little bit more detail please?

thake commented 2 years ago

Thanks for driving this feature :+1:

For serialization/deserialization you can have a look at https://github.com/avro-kotlin/avro4k/blob/master/src/test/kotlin/com/github/avrokotlin/avro4k/io/SealedClassIoTest.kt

I thought about a test where a schema for the class ReferencingPolymorphicRoot in the following code snippet:

@Serializable
abstract class UnsealedPolymorphicRoot

@Serializable
data class UnsealedChildOne(val one: String) : UnsealedPolymorphicRoot()

@Serializable
data class UnsealedChildTwo(val two: String) : UnsealedPolymorphicRoot()

@Serializable
data class ReferencingPolymorphicRoot(
  @Polymorphic
  val root : UnsealedPolymorphicRoot
)
williamboxhall commented 2 years ago

@thake I've just pushed up the additional tests you ask for, and, shamefully, the missing encoder/decoder support for the polymorphic types (my previous code generated the avro schema properly but not the actual encode/decode 🤦🏻‍♂️). I think everything's all good now.

Note that @Polymorphic is not actually required in order for the nested type to serialize properly so I've deliberately omitted it (it's harmless when added, however)

thake commented 2 years ago

@williamboxhall Can I make some adjustments to your PR? I would like to remove some code duplication. Unfortunately, I do not seem to have write access to the PR branch in your fork.

williamboxhall commented 2 years ago

@thake I'll work on getting you added as a contributor to our fork but there's a bit of internal bureaucracy to get people added so it might take a couple of days. Another option could be for you to pull my branch back in under avro-kotlin/avro4k to make a couple of modifications.

williamboxhall commented 2 years ago

Another option could be to add me as a branch contributor to avro-kotlin/avro4k and I can push my branch upstream to there

williamboxhall commented 2 years ago

Update: @thake we've added you as a non-master branch contributor on our fork avro-kotlin/avro4k. You probably have to "accept" the invitation somewhere before you can push upstream though - hopefully you have a notification about it somewhere in github

thake commented 2 years ago

Unfortunately, it still does not work. Have you followed https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork to grant access?

williamboxhall commented 2 years ago

@thake we couldn't follow that process because I am not the organization owner - I think for that to work I'd need to close my PR and get my org owner to open it, then tick that box.

However, my org owner adding you as a contributor should work, and I think the problem is that you haven't "accepted" the invitation yet:

Screen Shot 2021-09-07 at 9 02 06 am

There should be somewhere you can go in github, or you may have received a notification, allowing you to accept this invitation, which should open up access. Let me know if you can't figure it out and I'll just re-fork this in my personal repo and push the changes up there and give you access.

williamboxhall commented 2 years ago

https://github.community/t/where-do-i-see-all-incoming-invitations-collaborator/661

This page says you either need to access from the email notification or navigate to our forked repo (https://github.com/cultureamp/avro4k) in order to accept the invitation.

williamboxhall commented 2 years ago

@thake I've created a new fork in my personal repo so if you can't get things working here, we can just do things over there where I have more control and have already enabled edits my maintainers: https://github.com/avro-kotlin/avro4k/pull/110

thake commented 2 years ago

Continuing in PR #110