properties with list of values #152

potfur · 2023-08-04T13:40:05Z

Avro AVSC allows for custom annotations as shown here.

Same functionality can be partially achieved by using @AvroProp annotation, but only for single value annotations.

For cases - like the linked issue, where a list of values is needed, existing annotation is not enough.

Therefore I'm suggesting adding @AvropProps("name", "valueA", "valueB", "etc")

franckrasolo · 2023-08-24T11:59:41Z

Hey @thake & @sksamuel 👋

Is there any chance this could be reviewed/approved at some point?

Chuckame · 2023-08-24T12:01:43Z

Hey, I'll take a look

franckrasolo · 2023-08-24T12:13:28Z

Thanks @Chuckame, much appreciated!

src/main/kotlin/com/github/avrokotlin/avro4k/annotations.kt

thake · 2023-08-24T12:32:23Z

@potfur, thank you very much for the PR and sorry for the delay.

Wouldn't it be enough to just change @AvroProp to be @Repeatable? This would raise the minimal kotlin level to 1.6 which is acceptable. Code:

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
@Repeatable
annotation class AvroProp(val key: String, val value: String)

//Example usage
@Serializable
@AvroProp("cold", "play")
@AvroProp("some", "thing")
data class TypeAnnotated(val str: String)

Another option which does not require kotlin 1.6 is to define the new annotation @AvroProp like this:

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
annotation class AvroProps(vararg val props: AvroProp)

//Example usage
@Serializable
@AvroProps(
  AvroProp("cold", "play"),
  AvroProp("some", "thing")
)
data class TypeAnnotated(val str: String)

I'm in favor of the first solution as this is a classic example of why repeatable annotations have been introduced. But I would like to hear your opinion on it.

potfur · 2023-08-24T12:41:50Z

The problem that I'm trying to solve is to be able to define

"tags": ["val1", "val2"]

From what I know, repeating annotation will result in something like:

"tag1": "val1"
"tag2": "val2"

So, it's about being able to have list of values for key instead multiple keys with single value.

Chuckame · 2023-08-24T12:54:40Z

With the comment of @thake it would result to something like:

@AvroProp("cold", "play")
@AvroProp("cold", "some")
@AvroProp("cold", "thing")
data class TypeAnnotated(val str: String)

Personally I don't really like it as it feels really verbose.

Chuckame · 2023-08-24T12:56:11Z

Or, we could take into the middle: Just modify AvroProp to accept multiple values using vararg or array. (breaking change, but cleaner)

thake · 2023-08-24T13:12:00Z

@potfur, thanks for fixing my confusion ;)

I'm totally with @Chuckame, the breaking change seems to be legit. @potfur can you also add the repeatable annotation?

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
@Repeatable
annotation class AvroProp(val key: String, vararg val values: String)

potfur · 2023-08-24T13:21:41Z

If breaking change is way to go - even better.

Chuckame · 2023-08-24T13:24:49Z

But an near invisible breaking change since it's not changing the type but only the name (I think it's not common to call this annotation with class' property names 🤞 ) so users will continue to call like this AvroProp("key", "value").

But let's make a major increase when releasing this PR.

Chuckame · 2023-08-24T20:16:30Z

Oh we missed something important @thake , that prevent us to have like a common AvroProp with single value and list value: We can have additional props like this "key": "value" or "key": ["value"]. How we can differentiate list and non-list added props ?

So new idea, we keep AvroProp having a single value (no vararg so it remains untouched) and another AvroListProp (same as the previously AvroProps) :

annotation class AvroListProp(val key: String, values: Array<String>)

src/main/kotlin/com/github/avrokotlin/avro4k/AnnotationExtractor.kt

potfur · 2023-08-24T20:51:47Z

Something like this (below) will work nicely, but has it's own limitations.

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
annotation class AvroProp(val key: String, val value: String = "", val values: Array<String> = [])

Mainly non-blank value must take precendence over values, otherwise it will end up in situation where same prop is declared twice and AvroRuntimeException: Can't overwrite property: cold is being thrown.
Also is slightly error prone, as the annotation allows for putting both value and values at once without any complaint.

So maybe, let's go back to my initial proposal with minor improvements.

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
annotation class AvroProp(val key: String, val value: String)

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
annotation class AvroProps(val key: String, val values: Array<String>)

As for AvroListProp vs AvroProps, I think AvroProps is consistent with already existing annotations - AvroAlias & AvroAliases

Chuckame · 2023-08-24T21:14:07Z

Let's go to your initial proposal yes. Just the name, I would prefer AvroListProp against AvroProps, since AvroProps is ambiguous, like "is it a list of props". Aliases is different where we should fix it and remove it prior to a AvroAlias with a vararg value. I'll create an issue to track it !

thake · 2023-08-24T21:38:23Z

I'll have a look into it tomorrow.

Chuckame · 2023-08-24T21:42:00Z

@thake it has been changed to AvroListProp 👌

thake · 2023-08-25T07:01:54Z

@thake, @potfur thanks for your patience.
I rechecked the avro spec on this topic and while the current proposal would solve the discrepancy for array values, the specification explicitly allows additional properties of any type (i.e., boolean, number, object, array, string, null). Maybe we should consider this for this PR.

We have a similar situation with the @AvroDefault annotation. Maybe we can apply the same solution so that the API feels coherent.

Proposal:

@SerialInfo
@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
@Repeatable
annotation class AvroJsonProp(
    val key: String,
    val jsonValue: String)

//Usage
data class TypeAnnotated(
    @AvroJsonProp("array", """["play","blub"]""")
    @AvroJsonProp("obj", """{"javascript":"object"}""")
    val str: String
)

Chuckame · 2023-08-25T08:12:07Z

@thake I'm not really a fan of this, since json in strings are source of errors and ambiguous, and regarding performances it requires parsing the value each time with ser/deser. We will have to make a cache etc...

Why not just having a supplier class, to have a typesafe implementation ? See the following :

annotation class AvroGenericProp(
    val key: String,
    val valueSupplier: KClass<out AvroPropSupplier<*>>,
)

interface AvroPropSupplier<T> {
    val serializer: SerializationStrategy<T>
    fun getValue(): T
}

//usage
data class TypeAnnotated(
    @AvroGenericProp("array", CustomPropSupplier::class)
    val str: String
)

@Serializable
data class CustomObject(val field1: String)

object CustomPropSupplier : AvroPropSupplier<CustomObject> {
    override val serializer = CustomObject.serializer()
    override fun getValue() = CustomObject("value")
}

That way we are sure that we can put whatever we want as inside the field value, it can be static or we can pass some context to the getValue (like the annotated field's value, the parent class, etc...). And it's not so complex to implement, what do you think ?

potfur · 2023-08-25T08:40:33Z

I followed @thake suggestion, as it is more as Avro actually suggests with handling this case, and it also brings bit more flexibility.

About @Repeatable and type safety - I think for those two separate PR would be better, as those would affect also other annotations - just to be consistent, otherwise it would be weird.

Chuckame · 2023-08-25T09:21:08Z

I don't understand, avro only suggest to handle all the types in additional props. Why we continue using json while we have pure kotlin to write objects ? 🤔

Chuckame · 2023-08-25T09:29:35Z

src/main/kotlin/com/github/avrokotlin/avro4k/annotations.kt

@@ -8,6 +8,10 @@ import kotlinx.serialization.SerialInfo
 @Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
 annotation class AvroProp(val key: String, val value: String)

+@SerialInfo
+@Target(AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
+annotation class AvroJsonProp(val key: String, val jsonValue: String)


We could use @Language("json") coming from jetbrains to validate json at least in jetbrains editors, what do you think ?

Yes, that would improve the development experience.

I've just added it.

Can you add it also to the AvroDefault annotation ? 👼

potfur · 2023-08-25T11:13:19Z

@Chuckame I get your point, and for sure it would be better to use proper types.

On the other hand, the AvroDefault annotation already uses JSON strings, so AvroJsonProp is aligned and consistent what's already there (I like being consistent, even when it is suboptimal - gives some predictability and less cognitive load).

IMHO, doing improvements on types, and adding Repeatable, would be separate work - especially that forcing types would be a breaking change. So far, all is still backwards compatible - introduces new capability while preserving present.

Chuckame · 2023-08-25T11:20:26Z

I have exactly the same concern about AvroDefault btw 🥲

Let's do AvroJsonProp, but it won't be that performant.

I'll create an issue to discuss about "type-safe" AvroDefault, AvroJsonProp and other jsons annotations.

franckrasolo · 2023-08-25T14:12:41Z

Given the backward-compatible nature of these changes, could we get a new release out once @thake has approved this PR?

thake

Just some small remarks. After merging this, we'll release a new version.

thake · 2023-08-25T14:36:27Z

src/main/kotlin/com/github/avrokotlin/avro4k/AnnotationExtractor.kt

@@ -26,6 +26,7 @@ class AnnotationExtractor(private val annotations: List<Annotation>) {
         annotations.filterIsInstance<AvroAliases>().flatMap { it.value.toList() }
      }
   fun props(): List<Pair<String, String>> = annotations.filterIsInstance<AvroProp>().map { it.key to it.value }
+   fun json(): List<Pair<String, String>> = annotations.filterIsInstance<AvroJsonProp>().map { it.key to it.jsonValue }


Suggested change

fun json(): List<Pair<String, String>> = annotations.filterIsInstance<AvroJsonProp>().map { it.key to it.jsonValue }

fun jsonProps(): List<Pair<String, String>> = annotations.filterIsInstance<AvroJsonProp>().map { it.key to it.jsonValue }

Function renamed

thake · 2023-08-25T14:40:15Z

src/test/kotlin/com/github/avrokotlin/avro4k/schema/AvroJsonPropSchemaTest.kt

+import io.kotest.matchers.shouldBe
+import kotlinx.serialization.Serializable
+
+class AvroJsonPropSchemaTest : WordSpec() {


Could you add some tests for object properties? Just to make sure that this is also correctly processed.

Do you mean adding tests for a JSON value that is an object with properties?

I ask because there's already the @AvroJsonProp should support props annotation on field test.

Do you mean adding tests for a JSON value that is an object with properties?

Exactly

I simply extended that test by annotating a property using a JSON object with heterogeneous property types.

thake · 2023-08-25T14:46:25Z

I have exactly the same concern about AvroDefault btw 🥲

Let's do AvroJsonProp, but it won't be that performant.

I'll create an issue to discuss about "type-safe" AvroDefault, AvroJsonProp and other jsons annotations.

It is a kind of trade-off between "ease of use" in combination with pre-existing schemas and type safety. Using JSON in the annotations makes it easy to model your kotlin code so that it matches an existing schema; you just copy and paste the value from the asvc schema file.

Performance-wise, it shouldn't impact serialization and deserialization. The annotations' JSON is only interpreted whenever a schema is derived from the kotlin class. The schema is usually only derived once for every serialized class (responsibility of the user) and not on every serialization/deserialization.

Chuckame · 2023-08-25T16:41:35Z

@thake Since we'll have the @Language annotation to validate the content, it's an acceptable trade-off 🚀

For the performances, If we use the Avro method toRecord(serializer, obj): GenericRecord then we regenerate the schema each time we serialize. Maybe we could add some doc to advice the use of toRecord(serializer, schema, obj) in an intensive serialization of the same schema.

Chuckame · 2023-08-25T17:16:43Z

FYI, I'm playing with @Repeatable with kotlinx serialization, and it's only taking the latest annotated value, even if it should be already fixed. See Kotlin/kotlinx.serialization#2099, it works only with kotlin 1.9 I don't know why

thake

LGTM, thank you very much for your work!

franckrasolo · 2023-08-28T13:28:09Z

@Chuckame / @thake Alas @Language("json") applied to @AvroJsonProp appears to only take effect in IntelliJ IDEA within the avro-kotlin/avro4k project itself, not when it's consumed from another project.

There are a number of open issues such as KTIJ-16874 and IDEA-173477 related to the processing of @Language as a meta-annotation. You may also want to vote them up.

Chuckame · 2023-08-28T18:46:58Z

@franckrasolo as I can read, both are related not to our case: we are directly using this language annotation on the value parameter. Those issues are related to meta annotations, implying that annotations (like @Language) are annotating an annotation A, where this annotation A is used on a parameter, so all the annotations on A will be applied to the parameter.

Also tested AvroJsonProp on my computer and the json language injection is not working. After a quick check, tldr: replace @Language("json") by @Language("JSON").
Longer story: language injection is done by intellij, and we can find all the languages in Parameters > Editor > Language injections. We should set a value found inside the Language column, where we can only find JSON and not json, maybe it's case sensitive! I'll do the pr

franckrasolo · 2023-08-28T18:50:36Z

Yes, not an exact match, but the number of issues makes me wonder how stable the processing of @Language is across a wide range of use cases: host languages x injected languages x IDE flavours, and so forth.

Chuckame requested changes Aug 24, 2023

View reviewed changes

src/main/kotlin/com/github/avrokotlin/avro4k/annotations.kt Outdated Show resolved Hide resolved

Chuckame requested changes Aug 24, 2023

View reviewed changes

src/main/kotlin/com/github/avrokotlin/avro4k/AnnotationExtractor.kt Outdated Show resolved Hide resolved

Chuckame approved these changes Aug 24, 2023

View reviewed changes

thake self-requested a review August 25, 2023 07:02

AvroJsonProp for greater flexibility

351f706

Chuckame reviewed Aug 25, 2023

View reviewed changes

annotate the JSON value for JetBrains editors

3d9e4c2

thake requested changes Aug 25, 2023

View reviewed changes

rename json to jsonProps in AnnotationExtractor

c1e5e80

franckrasolo added 2 commits August 25, 2023 19:36

annotate AvroDefault's value as JSON for JetBrains editors

9897cda

cover AvroJsonProp using a JSON object with heterogeneous property types

def06bf

thake approved these changes Aug 25, 2023

View reviewed changes

thake merged commit 18dd189 into avro-kotlin:main Aug 25, 2023

potfur deleted the properties-with-list-of-values branch August 28, 2023 08:15

	fun json(): List<Pair<String, String>> = annotations.filterIsInstance<AvroJsonProp>().map { it.key to it.jsonValue }
	fun jsonProps(): List<Pair<String, String>> = annotations.filterIsInstance<AvroJsonProp>().map { it.key to it.jsonValue }

properties with list of values #152

properties with list of values #152

Conversation

potfur commented Aug 4, 2023

franckrasolo commented Aug 24, 2023

Chuckame commented Aug 24, 2023

franckrasolo commented Aug 24, 2023

thake commented Aug 24, 2023 • edited Loading

potfur commented Aug 24, 2023 • edited Loading

Chuckame commented Aug 24, 2023

Chuckame commented Aug 24, 2023

thake commented Aug 24, 2023

potfur commented Aug 24, 2023

Chuckame commented Aug 24, 2023

Chuckame commented Aug 24, 2023

potfur commented Aug 24, 2023 • edited Loading

Chuckame commented Aug 24, 2023

thake commented Aug 24, 2023 • edited Loading

Chuckame commented Aug 24, 2023

thake commented Aug 25, 2023

Chuckame commented Aug 25, 2023 • edited Loading

potfur commented Aug 25, 2023 • edited Loading

Chuckame commented Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chuckame Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potfur commented Aug 25, 2023

Chuckame commented Aug 25, 2023

franckrasolo commented Aug 25, 2023

thake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thake commented Aug 25, 2023 • edited Loading

Chuckame commented Aug 25, 2023

Chuckame commented Aug 25, 2023 • edited Loading

thake left a comment

Choose a reason for hiding this comment

franckrasolo commented Aug 28, 2023 • edited Loading

Chuckame commented Aug 28, 2023 • edited Loading

franckrasolo commented Aug 28, 2023

thake commented Aug 24, 2023 •

edited

Loading

potfur commented Aug 24, 2023 •

edited

Loading

potfur commented Aug 24, 2023 •

edited

Loading

thake commented Aug 24, 2023 •

edited

Loading

Chuckame commented Aug 25, 2023 •

edited

Loading

potfur commented Aug 25, 2023 •

edited

Loading

Chuckame commented Aug 25, 2023 •

edited

Loading

Chuckame Aug 25, 2023 •

edited

Loading

thake commented Aug 25, 2023 •

edited

Loading

Chuckame commented Aug 25, 2023 •

edited

Loading

franckrasolo commented Aug 28, 2023 •

edited

Loading

Chuckame commented Aug 28, 2023 •

edited

Loading