avro-kotlin / avro4k

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

Remove AvroName and AvroNamespace in favour of native SerialName #165

Closed Chuckame closed 2 months ago

Chuckame commented 10 months ago

Now, to redefine record's name or namespace, just use @SerialName("RecordName") or @SerialName("a.good.namespace.RecordName") (note that record name is now explicit). The goal of kotlinx serialization framework is to be a declarative serialization framework, so if the needs are to have all the records inside a specific namespace and/or all classes with a specific record name, then move all your classes inside this package and name the data classes accordingly.

Out-of-the-box:

// After (v2) package test object Bar

> No change as it is the default behavior

## Override namespace:
- `namespace`: `foo`
- `record name`: `Bar`
```kotlin
// Before (v1)
@AvroNamespace("foo")
object Bar

// After (v2)
@SerialName("foo.Bar")
object Bar

If you need to redefine the namespace, then you also need to explicit the record's name

Override record's name:

// After (v2): package test @SerialName("test.foo") object Bar

> If you need to redefine the name, then you also need to explicit the namespace, or it will become empty

## Remove namespace or redefine name-only record
- no `namespace`
- `record name`: `foo`
```kotlin
// Before (v1)
package test
@AvroNamespace("")
@AvroName("foo")
object Bar

// After (v2):
package test
@SerialName("foo")
object Bar

Override record's name for a specific field

Having the parent class Bar in parent.namespace and the field Subtype in custom namespace:

// Before (v1)
package parent.namespace
data class Bar(
  @AvroNamespace("custom") val field: Subtype
)

// After (v2):
package parent.namespace
data class Bar(
  @AvroNamespaceOverride("custom") val field: Subtype
)

Initial problem/proposal

We can mess with priorities because of too much annotations, and avro4k needs caching just to handling those multiple annotations (AvroName, AvroNamespace, SerialName), and also can be ambiguous because of apache's avro java annotations. Also, the native SerialName is not fully handled because RecordNaming is using the class name, and I don't exactly understand how the AvroName and AvroNamespace is used since there are a lot of little cases (AvroFixed can change the name by example).

This is simplifying a lot the naming codebase, while it also simplifies the api. Maybe we can keep the AvroNamespace to be able to only change the namespace, but since it's the name of the package, just moving classes to the good package seems cleaner and clearer.

Chuckame commented 10 months ago

@thake what do you think ?

thake commented 5 months ago

Sorry for getting back to you on this so late. I think we should consolidate the current ways of naming a record. Have you checked if the @SerialName annotation can cover all use cases?

Chuckame commented 5 months ago

I don't understand, what it could not work with serial name annot? 😁

thake commented 5 months ago

I actually don't know because I've not checked it yet. That's why I'm asking you if you have checked it ;)

Everything possible according to the avro spec on naming must be possible with whatever we come up with. I just want to be sure before we settle on a future design.

Chuckame commented 5 months ago

No issue then, let's plan it as a breaking change to just remove the avro name and namespace annotations to centralize everything using native kotlinx annotations

Chuckame commented 5 months ago

There is still one edge case: overriding record's namespace of a field. I propose to still remove AvroNamespace while adding a new AvroNamespaceOverride that would be only usable on properties. The other advantage is to stop messing with the original AvroNamespace annotation.

I update the issue's description

Chuckame commented 2 months ago

Done in #182