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

Remove AvroName and AvroNamespace in favour of native SerialName #165

Closed
Chuckame opened this issue Sep 2, 2023 · 7 comments · Fixed by #182
Closed

Remove AvroName and AvroNamespace in favour of native SerialName #165

Chuckame opened this issue Sep 2, 2023 · 7 comments · Fixed by #182
Labels
enhancement New feature or request
Milestone

Comments

@Chuckame
Copy link
Contributor

Chuckame commented Sep 2, 2023

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:

  • namespace: test
  • record name: Bar
// Before (v1)
package test
object Bar

// After (v2)
package test
object Bar

No change as it is the default behavior

Override namespace:

  • namespace: foo
  • record name: Bar
// 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:

  • namespace: test
  • record name: foo
// Before (v1)
package test
@AvroName("foo")
object Bar

// 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
// 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 Chuckame added the enhancement New feature or request label Sep 2, 2023
@Chuckame
Copy link
Contributor Author

Chuckame commented Sep 2, 2023

@thake what do you think ?

@thake
Copy link
Member

thake commented Jan 21, 2024

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?

@thake thake added this to the Version 2 milestone Jan 21, 2024
@Chuckame
Copy link
Contributor Author

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

@thake
Copy link
Member

thake commented Jan 21, 2024

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

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

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 added a commit to Chuckame/avro4k that referenced this issue Jan 28, 2024
@Chuckame Chuckame linked a pull request Jan 28, 2024 that will close this issue
Chuckame added a commit to Chuckame/avro4k that referenced this issue Jan 28, 2024
Chuckame added a commit to Chuckame/avro4k that referenced this issue Feb 3, 2024
Chuckame added a commit to Chuckame/avro4k that referenced this issue Apr 10, 2024
@Chuckame
Copy link
Contributor Author

Done in #182

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

Successfully merging a pull request may close this issue.

2 participants