feat: Use the Serde attributes and check for conflicting attributes #377

Kriskras99 · 2025-12-28T09:17:12Z

This commit changes the attribute parsing in #[derive(AvroSchema)] to also use the Serde attributes. It will use the Serde attributes and check that they match the Avro attributes if provided. It will also check that known incompatible Serde attributes result in a compile error instead of a runtime panic.

Testing for the compile errors is done using trybuild. These tests run only on nightly as the output can vary between compiler versions (otherwise MSRV and latest stable could have different outputs and break the entire CI).

For some reason Github closed #373, not sure why. This is the exact same commit.

avro_derive/src/case.rs

avro_derive/src/attributes/serde.rs

avro_derive/src/attributes/mod.rs

avro_derive/src/attributes/avro.rs

avro_derive/src/attributes/mod.rs

martin-g · 2025-12-28T11:05:58Z

avro_derive/src/attributes/mod.rs

+        if avro.skip && !serde.skip {
+            errors.push(syn::Error::new(
+                span,
+                "`#[avro(skip)]` requires `#[serde(skip)]`, it's also deprecated. Please use only `#[serde(skip)]`"


Is there a way to report the usage of a deprecated avro(attribute) as a warning without depending on the value of its respective serde counter-part ?
I.e. pseudo code:

if avro.skip { report a warning like "#[avro(skip)] is deprecated. Use #[serde(skip)] instead" } if avro.skip != serde.skip {report an error, without the ", it's also deprecated" text}

Currently the user will be notified that a deprecated avro attribute is used only if the avro value differs from the serde's one.

That's only possible on nightly sadly (rust-lang/rust#54140). We could put #[rustversion::attr(not(nightly), ignore)] on a codeblock and only emit warnings on nightly

Sounds good!
Let's do it!
Then once it is in stable and our MSRV is at least this stable version we could delete just this attribute to enable it for all.

avro_derive/src/attributes/mod.rs

avro_derive/src/attributes/serde.rs

avro_derive/src/attributes/mod.rs

This commit changes the attribute parsing in `#[derive(AvroSchema)]` to also use the Serde attributes. It will use the Serde attributes and check that they match the Avro attributes if provided. It will also check that known incompatible Serde attributes result in a compile error instead of a runtime panic. Testing for the compile errors is done using `trybuild`. These tests run only on nightly as the output can vary between compiler versions (otherwise MSRV and latest stable could have different outputs and break the entire CI).

martin-g · 2025-12-29T04:04:37Z

A PR has been opened at Derling repo with an example how to parse serde:
https://github.com/TedDriggs/darling/pull/412/changes#diff-9df22bcc91f2a6d5114da4fff9f20353fb0171013e7a7fa385cea38e6130f691

We may borrow something useful from it.

martin-g · 2025-12-31T01:54:36Z

avro_derive/src/case.rs

-            }
+    pub fn from_str(rename_all_str: &str) -> Result<Self, ParseError> {
+        match rename_all_str {
+            "lowercase" => Ok(Self::LowerCase),


Why this no more uses RENAME_RULES ? Now the rules are duplicated.

RENAME_RULES .iter() .find(|(name, _)| *name == rename_all_str) .map(|(_, rule)| *rule) .ok_or_else(|| ParseError { unknown: rename_all_str.to_string(), })

martin-g · 2025-12-31T02:08:32Z

avro_derive/src/attributes/serde.rs

+impl RenameAll {
+    fn from_expr(expr: &Expr) -> darling::Result<Self> {
+        let Expr::Lit(lit) = expr else {
+            return Err(darling::Error::custom("Expected a string or a tuple!"));


Suggested change

return Err(darling::Error::custom("Expected a string or a tuple!"));

return Err(darling::Error::custom("Expected a string literal!"));

The tuple form is handled by FromMeta derive.

martin-g · 2025-12-31T02:11:57Z

avro_derive/src/attributes/mod.rs

+        if avro.skip && !serde.skip {
+            errors.push(syn::Error::new(
+                span,
+                "`#[avro(skip)]` requires `#[serde(skip)]`, it's also deprecated. Please use only `#[serde(skip)]`"


Sounds good!
Let's do it!
Then once it is in stable and our MSRV is at least this stable version we could delete just this attribute to enable it for all.

martin-g · 2025-12-31T02:12:19Z

avro_derive/src/attributes/mod.rs

+        if avro.rename.is_some() && serde.rename != avro.rename {
+            errors.push(syn::Error::new(
+                span,
+                "`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename  = \"..\")]`"


Suggested change

"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"

"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"

martin-g · 2025-12-31T02:12:31Z

avro_derive/src/attributes/mod.rs

+        if avro.rename.is_some() && serde.rename != avro.rename {
+            errors.push(syn::Error::new(
+                span,
+                "`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename  = \"..\")]`"


Suggested change

"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"

"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"

martin-g · 2025-12-31T02:48:28Z

avro_derive/tests/serde.rs

+    assert_eq!(obj, serde(obj.clone()).unwrap());
+}
+
+/// Takes in a type that implements the right combination of traits and runs it through a Serde Cycle and asserts that the error matches the expected string


Suggested change

/// Takes in a type that implements the right combination of traits and runs it through a Serde Cycle and asserts that the error matches the expected string

/// Takes in a type that implements the right combination of traits and runs it through a Serde round-trip and asserts that the error matches the expected string

martin-g · 2025-12-31T02:49:20Z

avro_derive/tests/serde.rs

+    if let Some(res) = reader.next() {
+        return res.and_then(|v| from_value::<T>(&v));
+    }
+    unreachable!("Nothing was encoded!")


Suggested change

unreachable!("Nothing was encoded!")

panic!("Nothing was encoded!")

martin-g · 2025-12-31T02:50:54Z

avro_derive/tests/serde.rs

+        let schema = r#"
+        {
+            "type":"record",
+            "name":"Foo",


Shouldn't this be "Bar" ?!

https://github.com/apache/avro-rs/pull/377/changes#r2654685022

martin-g · 2025-12-31T02:53:12Z

avro_derive/tests/serde.rs

+                    "name":"bar",
+                    "type": {
+                        "type":"record",
+                        "name":"Foo",


https://github.com/apache/avro-rs/pull/377/changes#r2654685022

martin-g · 2025-12-31T03:00:39Z

avro_derive/src/attributes/serde.rs

+pub struct ContainerAttributes {
+    #[darling(rename = "rename")]
+    /// Rename this container.
+    pub _rename: Option<String>,


I just noticed in tests/serde.rs that this attribute is ignored.
Shall we try to support it too ?

if #[avro(name=...)] is provided then use it

if #[serde(rename=...)] is provided then use it

if both are provided then error

if none are provided then use the struct name

WDYT ?

martin-g · 2025-12-31T03:24:48Z

avro_derive/src/attributes/mod.rs

+            errors.push(syn::Error::new(
+                span,
+                "AvroSchema derive does not support Serde `transparent` attribute",
+            ))


Suggested change

))

));

martin-g · 2025-12-31T03:24:58Z

avro_derive/src/attributes/mod.rs

+            errors.push(syn::Error::new(
+                span,
+                "AvroSchema derive does not support different rename rules for serializing and deserializing (`rename_all(serialize = \"..\", deserialize = \"..\")`)"
+            ))


Suggested change

))

));

martin-g · 2025-12-31T03:25:09Z

avro_derive/src/attributes/mod.rs

+            errors.push(syn::Error::new(
+                span,
+                "#[avro(rename_all = \"..\")] must match #[serde(rename_all = \"..\")], it's also deprecated. Please use only `#[serde(rename_all = \"..\")]`",
+            ))


Suggested change

))

));

martin-g reviewed Dec 28, 2025

View reviewed changes

Kriskras99 force-pushed the feat/use_serde_attributes2 branch from 762c230 to 4efa998 Compare December 28, 2025 14:35

Kriskras99 force-pushed the feat/use_serde_attributes2 branch from 4efa998 to 392ae2c Compare December 28, 2025 14:40

martin-g reviewed Dec 31, 2025

View reviewed changes

martin-g mentioned this pull request Jan 2, 2026

Add CI check for ASFv2 license header in the files #385

Open

	return Err(darling::Error::custom("Expected a string or a tuple!"));
	return Err(darling::Error::custom("Expected a string literal!"));

	"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"
	"`#[avro(rename = \"..\")]` must match `#[serde(rename = \"..\")]`, it's also deprecated. Please use only `#[serde(rename = \"..\")]`"

	/// Takes in a type that implements the right combination of traits and runs it through a Serde Cycle and asserts that the error matches the expected string
	/// Takes in a type that implements the right combination of traits and runs it through a Serde round-trip and asserts that the error matches the expected string

	unreachable!("Nothing was encoded!")
	panic!("Nothing was encoded!")

feat: Use the Serde attributes and check for conflicting attributes #377

Are you sure you want to change the base?

feat: Use the Serde attributes and check for conflicting attributes #377

Uh oh!

Conversation

Kriskras99 commented Dec 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martin-g commented Dec 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants