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

Add type-safe for AvroDefault #158

Open
Chuckame opened this issue Aug 28, 2023 · 6 comments
Open

Add type-safe for AvroDefault #158

Chuckame opened this issue Aug 28, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@Chuckame
Copy link
Contributor

Chuckame commented Aug 28, 2023

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

@Chuckame Chuckame added the enhancement New feature or request label Aug 28, 2023
@thake
Copy link
Member

thake commented Aug 29, 2023

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

A few things to note:

  • I think you already intended it, but I just want to state it explicitly. Let's make it an additional option for users to choose between the existing and the new way to define default/props. As you already described very well, the chosen way depends on how one works (schema first vs code first).
  • I think the serializer field in AvroTypeSafeSupplier is not needed as we can deduct the Serializer using serializersModule.serializer(aAvroTypeSafeSupplier.value::class.java)

@Chuckame
Copy link
Contributor Author

  • yes, it won't change the current way of declaring defaults or props with json!
  • good point for the way of retrieving the serializer, I changed the original message to take this into account and simplify the code ! But, we will have to make attention to performances and cache the generated json of the value.

@thake
Copy link
Member

thake commented Aug 29, 2023

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

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

thake commented Aug 29, 2023

Opened up #159.

@Chuckame Chuckame mentioned this issue Jan 22, 2024
14 tasks
@Chuckame Chuckame changed the title Add type-safe for AvroDefault, AvroEnumDefault and AvroProp Add type-safe for AvroDefault May 9, 2024
@Chuckame
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants