From 8a5649e9fd181d3316b569c2bc0228a9dcbd7fdb Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 7 Sep 2024 20:58:31 +0200 Subject: [PATCH] Add result type for `JsonWriter::finish_document` --- src/reader/stream_reader.rs | 2 ++ src/writer/mod.rs | 18 ++++++---- src/writer/simple.rs | 16 ++++++--- src/writer/stream_writer.rs | 24 +++++++++++-- tests/custom_json_writer.rs | 70 +++++++++++++++---------------------- 5 files changed, 75 insertions(+), 55 deletions(-) diff --git a/src/reader/stream_reader.rs b/src/reader/stream_reader.rs index 14303c6..51fb234 100644 --- a/src/reader/stream_reader.rs +++ b/src/reader/stream_reader.rs @@ -4433,6 +4433,8 @@ mod tests { /* Note: If maintaining this becomes too cumbersome when adjusting JsonWriter API, can remove this test */ struct FailingJsonWriter; impl JsonWriter for FailingJsonWriter { + type WriterResult = (); + fn begin_object(&mut self) -> Result<(), IoError> { Err(err()) } diff --git a/src/writer/mod.rs b/src/writer/mod.rs index 69160e4..ed41483 100644 --- a/src/writer/mod.rs +++ b/src/writer/mod.rs @@ -89,6 +89,11 @@ type IoError = std::io::Error; /// related to incorrect usage by the user (such as trying to call [`end_object`](Self::end_object) when currently /// writing a JSON array). pub trait JsonWriter { + /// Result returned by [`finish_document`](Self::finish_document) + /// + /// The type of the result and its meaning depends on the JSON writer implementation. + type WriterResult; + // TODO: Should these methods all return &mut Self to allow chaining? /// Begins writing a JSON object @@ -521,6 +526,12 @@ pub trait JsonWriter { /// Verifies that the JSON document is complete and flushes the buffer /// + /// Returns the [`WriterResult`](Self::WriterResult) on success. Depending on the + /// JSON writer implementation this can for example be the written JSON value. + /// The result might not be relevant for all JSON writer implementations, for example + /// a JSON writer writing to a `Write` will already produce the JSON document during + /// usage, so the `WriterResult` returned by this method can be ignored. + /// /// This method **must** be called explicitly. Dropping the JSON writer will not /// automatically flush the buffer. /// @@ -537,12 +548,7 @@ pub trait JsonWriter { * Note: Dropping writer will not automatically finish document since that would silently discard errors which might occur * TODO: Does consuming 'self' here really guarantee that writer cannot be used afterwards anymore; can this be bypassed with reference somehow? */ - /* - * TODO (custom JSON writer support): Instead of returning `()` maybe use an associated type - * on JsonWriter as result type; then custom JsonWriter implementations which produce a - * value can return it here. - */ - fn finish_document(self) -> Result<(), IoError>; + fn finish_document(self) -> Result; } /// Writer for lazily writing a JSON string value diff --git a/src/writer/simple.rs b/src/writer/simple.rs index 5d724d4..60ce95e 100644 --- a/src/writer/simple.rs +++ b/src/writer/simple.rs @@ -287,6 +287,8 @@ mod error_safe_writer { } impl JsonWriter for ErrorSafeJsonWriter { + type WriterResult = J::WriterResult; + fn begin_object(&mut self) -> Result<(), IoError> { use_delegate!(self, |w| w.begin_object()) } @@ -377,7 +379,7 @@ mod error_safe_writer { ) } - fn finish_document(self) -> Result<(), IoError> { + fn finish_document(self) -> Result { // Special code instead of `use_delegate!(...)` because this method consumes `self` if let Some(error) = self.error { return Err(error_from_stored(&error)); @@ -516,17 +518,20 @@ impl SimpleJsonWriter> { impl ValueWriter for SimpleJsonWriter { fn write_null(mut self) -> Result<(), IoError> { self.json_writer.null_value()?; - self.json_writer.finish_document() + self.json_writer.finish_document()?; + Ok(()) } fn write_bool(mut self, value: bool) -> Result<(), IoError> { self.json_writer.bool_value(value)?; - self.json_writer.finish_document() + self.json_writer.finish_document()?; + Ok(()) } fn write_string(mut self, value: &str) -> Result<(), IoError> { self.json_writer.string_value(value)?; - self.json_writer.finish_document() + self.json_writer.finish_document()?; + Ok(()) } fn write_string_with_writer( @@ -540,7 +545,8 @@ impl ValueWriter for SimpleJsonWriter { fn write_number(mut self, value: N) -> Result<(), IoError> { self.json_writer.number_value(value)?; - self.json_writer.finish_document() + self.json_writer.finish_document()?; + Ok(()) } fn write_fp_number(mut self, value: N) -> Result<(), JsonNumberError> { diff --git a/src/writer/stream_writer.rs b/src/writer/stream_writer.rs index 11ba4da..f7765bb 100644 --- a/src/writer/stream_writer.rs +++ b/src/writer/stream_writer.rs @@ -388,6 +388,14 @@ impl JsonStreamWriter { } impl JsonWriter for JsonStreamWriter { + /// Result returned by [`finish_document`](Self::finish_document) + /// + /// This JSON writer implementation returns the underlying `Write` to allow for + /// example to reuse it for other purposes. However, the JSON document is already + /// written during JSON writer usage, so the returned `Write` can be ignored in + /// case it is not needed for anything else. + type WriterResult = W; + fn begin_object(&mut self) -> Result<(), IoError> { self.before_value()?; self.stack.push(StackValue::Object); @@ -509,7 +517,7 @@ impl JsonWriter for JsonStreamWriter { // might not be necessary because Serde's Serialize API enforces this } - fn finish_document(mut self) -> Result<(), IoError> { + fn finish_document(mut self) -> Result { if self.is_string_value_writer_active { panic!("Incorrect writer usage: Cannot finish document when string value writer is still active"); } @@ -523,7 +531,8 @@ impl JsonWriter for JsonStreamWriter { } else { panic!("Incorrect writer usage: Cannot finish document when top-level value is not finished"); } - self.flush() + self.flush()?; + Ok(self.writer) } fn string_value_writer(&mut self) -> Result { @@ -1585,6 +1594,17 @@ mod tests { Ok(()) } + /// Verify that `finish_document` returns the wrapped writer. + #[test] + fn finish_document_result() -> TestResult { + let mut json_writer = JsonStreamWriter::new(Vec::::new()); + json_writer.string_value("text")?; + let written_bytes = json_writer.finish_document()?; + assert_eq!("\"text\"", String::from_utf8(written_bytes)?); + + Ok(()) + } + #[test] #[should_panic( expected = "Incorrect writer usage: Cannot finish document when no value has been written yet" diff --git a/tests/custom_json_writer.rs b/tests/custom_json_writer.rs index 41fab3d..2711621 100644 --- a/tests/custom_json_writer.rs +++ b/tests/custom_json_writer.rs @@ -29,37 +29,25 @@ mod custom_writer { Object(Map), } - pub struct JsonValueWriter<'a> { + pub struct JsonValueWriter { stack: Vec, pending_name: Option, is_string_value_writer_active: bool, /// Holds the final value until `finish_document` is called - final_value_temp: Option, - /// Holds the final value after `finish_document` is called, and which is accessible - /// to creator of `JsonValueWriter` (who should still have a reference to this `Option`) - final_value_holder: &'a mut Option, + final_value: Option, } - impl<'a> JsonValueWriter<'a> { - /* - * TODO: This approach of taking an `Option` reference and storing the final value in it - * matches the current JsonWriter API, however a cleaner approach might be if `finish_document` - * could return the result instead, see TODO comment on `JsonWriter::finish_document` - */ - pub fn new(final_value_holder: &'a mut Option) -> Self { - if final_value_holder.is_some() { - panic!("Final value holder should be None"); - } + impl JsonValueWriter { + pub fn new() -> Self { JsonValueWriter { stack: Vec::new(), pending_name: None, is_string_value_writer_active: false, - final_value_temp: None, - final_value_holder, + final_value: None, } } } - impl JsonValueWriter<'_> { + impl JsonValueWriter { fn verify_string_writer_inactive(&self) { if self.is_string_value_writer_active { panic!("Incorrect writer usage: String value writer is active"); @@ -68,7 +56,7 @@ mod custom_writer { fn check_before_value(&self) { self.verify_string_writer_inactive(); - if self.final_value_temp.is_some() || self.final_value_holder.is_some() { + if self.final_value.is_some() { panic!("Incorrect writer usage: Top-level value has already been written") } if let Some(StackValue::Object(_)) = self.stack.last() { @@ -88,10 +76,10 @@ mod custom_writer { }; } else { debug_assert!( - self.final_value_temp.is_none(), + self.final_value.is_none(), "caller should have verified that final value is not set yet" ); - self.final_value_temp = Some(value); + self.final_value = Some(value); } } } @@ -101,7 +89,9 @@ mod custom_writer { .ok_or_else(|| JsonNumberError::InvalidNumber(format!("non-finite number: {f}"))) } - impl JsonWriter for JsonValueWriter<'_> { + impl JsonWriter for JsonValueWriter { + type WriterResult = Value; + fn begin_object(&mut self) -> Result<(), IoError> { self.check_before_value(); self.stack.push(StackValue::Object(Map::new())); @@ -253,22 +243,21 @@ mod custom_writer { // might not be necessary because Serde's Serialize API enforces this } - fn finish_document(mut self) -> Result<(), IoError> { + fn finish_document(self) -> Result { self.verify_string_writer_inactive(); - if let Some(value) = self.final_value_temp.take() { - *self.final_value_holder = Some(value); - Ok(()) + if let Some(value) = self.final_value { + Ok(value) } else { panic!("Incorrect writer usage: Top-level value is incomplete") } } } - struct StringValueWriterImpl<'j, 'a> { + struct StringValueWriterImpl<'j> { buf: Vec, - json_writer: &'j mut JsonValueWriter<'a>, + json_writer: &'j mut JsonValueWriter, } - impl Write for StringValueWriterImpl<'_, '_> { + impl Write for StringValueWriterImpl<'_> { fn write(&mut self, buf: &[u8]) -> std::io::Result { self.buf.extend_from_slice(buf); Ok(buf.len()) @@ -279,7 +268,7 @@ mod custom_writer { Ok(()) } } - impl StringValueWriter for StringValueWriterImpl<'_, '_> { + impl StringValueWriter for StringValueWriterImpl<'_> { fn finish_value(self) -> Result<(), IoError> { let string = String::from_utf8(self.buf).map_err(|e| IoError::new(ErrorKind::InvalidData, e))?; @@ -303,8 +292,7 @@ fn write() -> Result<(), Box> { } } - let mut final_value_holder = None; - let mut json_writer = JsonValueWriter::new(&mut final_value_holder); + let mut json_writer = JsonValueWriter::new(); json_writer.begin_array()?; @@ -343,7 +331,7 @@ fn write() -> Result<(), Box> { ); json_writer.end_array()?; - json_writer.finish_document()?; + let written_value = json_writer.finish_document()?; let expected_json = json!([ { @@ -361,15 +349,14 @@ fn write() -> Result<(), Box> { -67, 8.9, ]); - assert_eq!(expected_json, final_value_holder.unwrap()); + assert_eq!(expected_json, written_value); Ok(()) } #[test] fn transfer() -> Result<(), Box> { - let mut final_value_holder = None; - let mut json_writer = JsonValueWriter::new(&mut final_value_holder); + let mut json_writer = JsonValueWriter::new(); let mut json_reader = JsonStreamReader::new( "[true, 123, {\"name1\": \"value1\", \"name2\": null}, false]".as_bytes(), @@ -377,7 +364,7 @@ fn transfer() -> Result<(), Box> { json_reader.transfer_to(&mut json_writer)?; json_reader.consume_trailing_whitespace()?; - json_writer.finish_document()?; + let written_value = json_writer.finish_document()?; let expected_json = json!([ true, @@ -389,7 +376,7 @@ fn transfer() -> Result<(), Box> { }, false, ]); - assert_eq!(expected_json, final_value_holder.unwrap()); + assert_eq!(expected_json, written_value); Ok(()) } @@ -405,16 +392,15 @@ fn serialize() -> Result<(), Box> { b: &'static str, } - let mut final_value_holder = None; - let mut json_writer = JsonValueWriter::new(&mut final_value_holder); + let mut json_writer = JsonValueWriter::new(); json_writer.serialize_value(&CustomStruct { a: 123, b: "test" })?; - json_writer.finish_document()?; + let written_value = json_writer.finish_document()?; let expected_json = json!({ "a": 123, "b": "test", }); - assert_eq!(expected_json, final_value_holder.unwrap()); + assert_eq!(expected_json, written_value); Ok(()) }