Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 80 additions & 29 deletions avro/src/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,16 @@ impl<'a, W: Write> Writer<'a, W> {
self.schema
}

/// Append a compatible value (implementing the `ToAvro` trait) to a `Writer`, also performing
/// schema validation.
/// Append a compatible value to a `Writer`, also performing schema validation.
///
/// Returns the number of bytes written (it might be 0, see below).
///
/// **NOTE**: This function is not guaranteed to perform any actual write, since it relies on
/// internal buffering for performance reasons. If you want to be sure the value has been
/// written, then call [`flush`](Writer::flush).
pub fn append<T: Into<Value>>(&mut self, value: T) -> AvroResult<usize> {
let n = self.maybe_write_header()?;

let avro = value.into();
self.append_value_ref(&avro).map(|m| m + n)
self.append_value_ref(&avro)
}

/// Append a compatible value to a `Writer`, also performing schema validation.
Expand All @@ -195,9 +192,58 @@ impl<'a, W: Write> Writer<'a, W> {
/// internal buffering for performance reasons. If you want to be sure the value has been
/// written, then call [`flush`](Writer::flush).
pub fn append_value_ref(&mut self, value: &Value) -> AvroResult<usize> {
if let Some(reason) = value.validate_internal(
self.schema,
self.resolved_schema.get_names(),
&self.schema.namespace(),
) {
return Err(Details::ValidationWithReason {
value: value.clone(),
schema: self.schema.clone(),
reason,
}
.into());
}
self.unvalidated_append_value_ref(value)
}

/// Append a compatible value to a `Writer`.
///
/// This function does **not** validate that the provided value matches the schema. If it does
/// not match, the file will contain corrupt data. Use [`Writer::append_value`] to have the
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation here refers to Writer::append_value, but this function does not exist. It should likely refer to Writer::append for the validated counterpart.

Suggested change
/// not match, the file will contain corrupt data. Use [`Writer::append_value`] to have the
/// not match, the file will contain corrupt data. Use [`Writer::append`] to have the

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:documentation; feedback:The Gemini AI reviewer is correct! There is no method with name append_value()! But for consistency append() should be deprecated and it should delegate to a new append_value(). This way all related methods (unvalidated_**, **_ref) will be consistently named.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs reference [Writer::append_value], but there doesn't appear to be an append_value method on Writer (likely meant Writer::append). As written this looks like a broken intra-doc link and could confuse users.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:documentation; feedback:The Augment AI reviewer is correct! There is no method with name append_value()! But for consistency append() should be deprecated and it should delegate to a new append_value(). This way all related methods (unvalidated_**, **_ref) will be consistently named.

/// value validated during write or use [`Value::validate`] to validate the value.
///
/// Returns the number of bytes written (it might be 0, see below).
///
/// **NOTE**: This function is not guaranteed to perform any actual write, since it relies on
/// internal buffering for performance reasons. If you want to be sure the value has been
/// written, then call [`flush`](Writer::flush).
pub fn unvalidated_append_value<T: Into<Value>>(&mut self, value: T) -> AvroResult<usize> {
let value = value.into();
self.unvalidated_append_value_ref(&value)
}
Comment on lines +210 to +224
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix doc reference to the validated API.

The docs mention Writer::append_value, but the public validated method is Writer::append (or Writer::append_value_ref). This will mislead callers.

✏️ Proposed doc fix
-    /// not match, the file will contain corrupt data. Use [`Writer::append_value`] to have the
+    /// not match, the file will contain corrupt data. Use [`Writer::append`] (or
+    /// [`Writer::append_value_ref`]) to have the
🤖 Prompt for AI Agents
In `@avro/src/writer.rs` around lines 210 - 224, The docstring for
unvalidated_append_value incorrectly references Writer::append_value; update the
documentation to reference the actual validated APIs (e.g., Writer::append
and/or Writer::append_value_ref) and any other relevant symbols mentioned
(Value::validate, Writer::flush, Writer::unvalidated_append_value_ref) so
callers are not misled — replace the incorrect `Writer::append_value`
occurrences with `Writer::append` (or note `Writer::append_value_ref`) and
ensure the cross-links point to the real public methods.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:documentation; feedback:The CodeRabbit AI reviewer is correct! There is no method with name append_value()! But for consistency append() should be deprecated and it should delegate to a new append_value(). This way all related methods (unvalidated_**, **_ref) will be consistently named.


/// Append a compatible value to a `Writer`.
///
/// This function does **not** validate that the provided value matches the schema. If it does
/// not match, the file will contain corrupt data. Use [`Writer::append_value_ref`] to have the
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the previous comment, the documentation refers to Writer::append_value_ref which is correct, but it also mentions Writer::append_value which does not exist. It should refer to Writer::append if the intent is to mention the top-level validated append function.

Suggested change
/// not match, the file will contain corrupt data. Use [`Writer::append_value_ref`] to have the
/// not match, the file will contain corrupt data. Use [`Writer::append_value_ref`] to have the

/// value validated during write or use [`Value::validate`] to validate the value.
///
/// Returns the number of bytes written (it might be 0, see below).
///
/// **NOTE**: This function is not guaranteed to perform any actual write, since it relies on
/// internal buffering for performance reasons. If you want to be sure the value has been
/// written, then call [`flush`](Writer::flush).
pub fn unvalidated_append_value_ref(&mut self, value: &Value) -> AvroResult<usize> {
let n = self.maybe_write_header()?;
encode_internal(
value,
self.schema,
self.resolved_schema.get_names(),
&self.schema.namespace(),
&mut self.buffer,
)?;

write_value_ref_resolved(self.schema, &self.resolved_schema, value, &mut self.buffer)?;
self.num_values += 1;

if self.buffer.len() >= self.block_size {
Expand Down Expand Up @@ -688,29 +734,6 @@ where
}
}

fn write_value_ref_resolved(
schema: &Schema,
resolved_schema: &ResolvedSchema,
value: &Value,
buffer: &mut Vec<u8>,
) -> AvroResult<usize> {
match value.validate_internal(schema, resolved_schema.get_names(), &schema.namespace()) {
Some(reason) => Err(Details::ValidationWithReason {
value: value.clone(),
schema: schema.clone(),
reason,
}
.into()),
None => encode_internal(
value,
schema,
resolved_schema.get_names(),
&schema.namespace(),
buffer,
),
}
}

fn write_value_ref_owned_resolved<W: Write>(
resolved_schema: &ResolvedOwnedSchema,
value: &Value,
Expand Down Expand Up @@ -1805,4 +1828,32 @@ mod tests {

Ok(())
}

#[test]
fn avro_rs_310_append_unvalidated_value() -> TestResult {
let schema = Schema::String;
let value = Value::Int(1);

let mut writer = Writer::new(&schema, Vec::new())?;
writer.unvalidated_append_value_ref(&value)?;
writer.unvalidated_append_value(value)?;
let buffer = writer.into_inner()?;

assert_eq!(&buffer[buffer.len() - 18..buffer.len() - 16], &[2, 2]);

let mut writer = Writer::new(&schema, Vec::new())?;
let value = Value::Int(1);
let err = writer.append_value_ref(&value).unwrap_err();
assert_eq!(
err.to_string(),
"Value Int(1) does not match schema String: Reason: Unsupported value-schema combination! Value: Int(1), schema: String"
);
let err = writer.append(value).unwrap_err();
assert_eq!(
err.to_string(),
"Value Int(1) does not match schema String: Reason: Unsupported value-schema combination! Value: Int(1), schema: String"
);

Ok(())
}
}