avro-kotlin / avro4k

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

Add type-safe for AvroDefault #158

Open Chuckame opened 10 months ago

Chuckame commented 10 months ago

After testing both json and JSON, the @Language is sadly not detected by IntelliJ, surely because it limits the scope of language injection scanning to the project sources only.

image

The only way of making it works is to have //language=json on the previous line (that is not really convenient since it will apply for all the next line):

image

Since it's not working for the moment, let's remove it to not let users try to fix for nothing 😄

When create kotlin classes from an avro schema, then the json for defaults and props is easy: Just copy paste from the schema and voilà.

But when using avro4k only in kotlin without generating schema, then having props and defaults is more complicated since we cannot copy/paste. It also makes the evolution more error prone with values that can change

An idea of having a type-safe value in annotations would be a supplier to retrieve the value, then convert it to json:

// user code
@AvroTypeSafeProp("prop", MyDataAdditionalProp::class)
data class MyData(
        @AvroTypeSafeDefault(DefaultFieldValue::class)
        val someField: String = DefaultFieldValue.value,
)

object DefaultFieldValue : AvroTypeSafeSupplier<String>("text")
object MyDataAdditionalProp : AvroTypeSafeSupplier<ComplexObject>(ComplexObject("a field", 123, "other data", NestedData(nestedList = listOf("a", "b", "c"))))

// avro4k code
@Target(AnnotationTarget.CLASS, AnnotationTarget.PROPERTY)
annotation class AvroTypeSafeDefault(
        val valueSupplier: KClass<out AvroTypeSafeSupplier<*>>,
)

abstract class AvroTypeSafeSupplier<T>(val value: T) // abstract as it is not intended to be used directly

The added value for defaults and props is that we now have a pure kotlin declaration (no more cross-language). The other added value for defaults is that we centralise the default value for both @AvroTypeSafeDefault and class' field.

EDIT: Removed the abstract serializer field as we don't need it anymore

thake commented 10 months ago

Great idea! Thanks for this detailed description of this enhancement. I really like it!

A few things to note:

Chuckame commented 10 months ago
thake commented 10 months ago

@Chuckame, regarding performance issues: I think we should create an additional enhancement to introduce schema caching into avro4k. As the schemas are always generated based on the classes loaded within the JVM, the size of the schema cache should be relatively small. So IMHO the performance gain outweighs the additional needed memory multiple times.

Chuckame commented 10 months ago

This is exactly what apache avro library is doing, caching all the schemas depending on the class. Let's do it!

What do you prefer ?

  1. implementing caching with this ticket
  2. create another ticket to implement caching separately
thake commented 10 months ago

Opened up #159.

Chuckame commented 1 month ago

When google/ksp#1868 is done, we could be able of creating a plugin that extract all the constant defaults from optional data class properties.