-
Notifications
You must be signed in to change notification settings - Fork 0
486: fix!: Small things #56
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
base: main
Are you sure you want to change the base?
Changes from all commits
bab8998
4e9ae89
1582cad
b7d4757
ed4bc32
7ce2e38
a1bfb30
055d2c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ thread_local! { | |
| /// [`Value::Bytes`] or [`Value::Fixed`]. | ||
| /// | ||
| /// Relies on the fact that serde's serialization process is single-threaded. | ||
| pub(crate) static SER_BYTES_TYPE: Cell<BytesType> = const { Cell::new(BytesType::Bytes) }; | ||
| pub(crate) static SER_BYTES_TYPE: Cell<BytesType> = const { Cell::new(BytesType::Unset) }; | ||
|
|
||
| /// A thread local that is used to decide if a [`Value::Bytes`] needs to be deserialized to | ||
| /// a [`Vec`] or slice. | ||
|
|
@@ -33,6 +33,7 @@ thread_local! { | |
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| pub(crate) enum BytesType { | ||
| Unset, | ||
| Bytes, | ||
| Fixed, | ||
| } | ||
|
|
@@ -92,6 +93,7 @@ impl Drop for BorrowedGuard { | |
| /// | ||
| /// [`apache_avro::serde::bytes_opt`]: bytes_opt | ||
| pub mod bytes { | ||
| use super::BytesType; | ||
| use std::collections::HashSet; | ||
|
|
||
| use serde::{Deserializer, Serializer}; | ||
|
|
@@ -119,13 +121,15 @@ pub mod bytes { | |
| where | ||
| S: Serializer, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| serde_bytes::serialize(bytes, serializer) | ||
| } | ||
|
|
||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The This concern also applies to the
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The thread local is used only during serialization, so there is no need to set it for deserialization. The change does not cause any harm but it should be reverted. |
||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
| } | ||
|
|
@@ -155,6 +159,7 @@ pub mod bytes { | |
| /// | ||
| /// [`apache_avro::serde::bytes`]: bytes | ||
| pub mod bytes_opt { | ||
| use super::BytesType; | ||
| use serde::{Deserializer, Serializer}; | ||
| use std::{borrow::Borrow, collections::HashSet}; | ||
|
|
||
|
|
@@ -184,13 +189,15 @@ pub mod bytes_opt { | |
| S: Serializer, | ||
| B: Borrow<[u8]> + serde_bytes::Serialize, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| serde_bytes::serialize(bytes, serializer) | ||
| } | ||
|
|
||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Vec<u8>>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
| } | ||
|
|
@@ -268,6 +275,7 @@ pub mod fixed { | |
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Fixed); | ||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
| } | ||
|
|
@@ -342,6 +350,7 @@ pub mod fixed_opt { | |
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Fixed); | ||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
| } | ||
|
|
@@ -373,6 +382,7 @@ pub mod fixed_opt { | |
| /// [`Value::Fixed`]: crate::types::Value::Fixed | ||
| /// [`apache_avro::serde::slice_opt`]: slice_opt | ||
| pub mod slice { | ||
| use super::BytesType; | ||
| use std::collections::HashSet; | ||
|
|
||
| use serde::{Deserializer, Serializer}; | ||
|
|
@@ -400,13 +410,15 @@ pub mod slice { | |
| where | ||
| S: Serializer, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| serde_bytes::serialize(bytes, serializer) | ||
| } | ||
|
|
||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<&'de [u8], D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _bytes_guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| let _guard = super::BorrowedGuard::set(true); | ||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
|
|
@@ -439,6 +451,7 @@ pub mod slice { | |
| /// [`Value::Fixed`]: crate::types::Value::Fixed | ||
| /// [`apache_avro::serde::slice`]: mod@slice | ||
| pub mod slice_opt { | ||
| use super::BytesType; | ||
| use serde::{Deserializer, Serializer}; | ||
| use std::{borrow::Borrow, collections::HashSet}; | ||
|
|
||
|
|
@@ -468,13 +481,15 @@ pub mod slice_opt { | |
| S: Serializer, | ||
| B: Borrow<[u8]> + serde_bytes::Serialize, | ||
| { | ||
| let _guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| serde_bytes::serialize(&bytes, serializer) | ||
| } | ||
|
|
||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<&'de [u8]>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let _bytes_guard = super::BytesTypeGuard::set(BytesType::Bytes); | ||
| let _guard = super::BorrowedGuard::set(true); | ||
| serde_bytes::deserialize(deserializer) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that
()maps toSchema::Null,Option<()>will try to build a union[Null, Null]inOption<T>::get_schema_in_ctxt, whichUnionSchema::newrejects (and the codeexpects), causing a panic during schema generation.Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! Trying to derive an AvroSchema for
Option<()>will lead to a runtime error due to the duplicate Null variant. It would be better to fail the build with an useful error instead of failing at runtime.