You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/10/17 20:33:56 UTC
[GitHub] [avro] davidpeklak opened a new pull request, #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
davidpeklak opened a new pull request, #1914:
URL: https://github.com/apache/avro/pull/1914
## What is the purpose of the change
*This pull request fixes a bug in GenericSingleObjectReader::read_value for non-exhaustive reads, fixing AVRO-3642
## Verifying this change
This change adds the test reader::tests::test_avro_3507_single_object_reader_incomplete_reads
## Documentation
This pull request does _not_ introduce a new feature
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1914:
URL: https://github.com/apache/avro/pull/1914#issuecomment-1281473798
Ah, now I see that you both discussed the issue already on the mailing list ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g merged pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
martin-g merged PR #1914:
URL: https://github.com/apache/avro/pull/1914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] jklamer commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
jklamer commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997785128
##########
lang/rust/avro/src/reader.rs:
##########
@@ -396,9 +396,9 @@ impl GenericSingleObjectReader {
pub fn read_value<R: Read>(&self, reader: &mut R) -> AvroResult<Value> {
let mut header: [u8; 10] = [0; 10];
- match reader.read(&mut header) {
- Ok(size) => {
- if size == 10 && self.expected_header == header {
+ match reader.read_exact(&mut header) {
+ Ok(_) => {
Review Comment:
That would make sense. LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] jklamer commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
jklamer commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997703818
##########
lang/rust/avro/src/reader.rs:
##########
@@ -396,9 +396,9 @@ impl GenericSingleObjectReader {
pub fn read_value<R: Read>(&self, reader: &mut R) -> AvroResult<Value> {
let mut header: [u8; 10] = [0; 10];
- match reader.read(&mut header) {
- Ok(size) => {
- if size == 10 && self.expected_header == header {
+ match reader.read_exact(&mut header) {
Review Comment:
very simple fix. I like.
##########
lang/rust/avro/src/reader.rs:
##########
@@ -396,9 +396,9 @@ impl GenericSingleObjectReader {
pub fn read_value<R: Read>(&self, reader: &mut R) -> AvroResult<Value> {
let mut header: [u8; 10] = [0; 10];
- match reader.read(&mut header) {
- Ok(size) => {
- if size == 10 && self.expected_header == header {
+ match reader.read_exact(&mut header) {
+ Ok(_) => {
Review Comment:
What would you feel about pattern matching to 10?
Along with a follow up error for more details, something like:
```
Ok(10) => { //this case},
Ok(x) => Err(Error::InompleteHeader(x)),
Err(io_error)=> Err(Error::ReadHeader(io_error)), //existing below
```
##########
lang/rust/avro/src/reader.rs:
##########
@@ -804,6 +804,38 @@ mod tests {
assert_eq!(expected_value, val);
}
+ #[test]
+ fn test_avro_3507_single_object_reader_incomplete_reads() {
+ let obj = TestSingleObjectReader {
+ a: 42,
+ b: 3.33,
+ c: vec!["cat".into(), "dog".into()],
+ };
+ let mut to_read_1 = Vec::<u8>::new();
+ to_read_1.extend_from_slice(&[0xC3, 0x01]);
+ let mut to_read_2 = Vec::<u8>::new();
+ to_read_2.extend_from_slice(
+ &TestSingleObjectReader::get_schema()
+ .fingerprint::<Rabin>()
+ .bytes[..],
+ );
+ let mut to_read_3 = Vec::<u8>::new();
+ encode(
+ &obj.clone().into(),
+ &TestSingleObjectReader::get_schema(),
+ &mut to_read_3,
+ )
+ .expect("Encode should succeed");
+ let mut to_read = (&to_read_1[..]).chain(&to_read_2[..]).chain(&to_read_3[..]);
Review Comment:
interesting. `the` method would fail in this case? not would I would expect. Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1914:
URL: https://github.com/apache/avro/pull/1914#issuecomment-1281948402
Thank you for the contribution, @davidpeklak !
Thank you for the review, @jklamer !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] davidpeklak commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
davidpeklak commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997773174
##########
lang/rust/avro/src/reader.rs:
##########
@@ -804,6 +804,38 @@ mod tests {
assert_eq!(expected_value, val);
}
+ #[test]
+ fn test_avro_3507_single_object_reader_incomplete_reads() {
+ let obj = TestSingleObjectReader {
+ a: 42,
+ b: 3.33,
+ c: vec!["cat".into(), "dog".into()],
+ };
+ let mut to_read_1 = Vec::<u8>::new();
+ to_read_1.extend_from_slice(&[0xC3, 0x01]);
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997490396
##########
lang/rust/avro/src/reader.rs:
##########
@@ -804,6 +804,38 @@ mod tests {
assert_eq!(expected_value, val);
}
+ #[test]
+ fn test_avro_3507_single_object_reader_incomplete_reads() {
Review Comment:
3507 ?!
```suggestion
fn avro_3642_test_single_object_reader_incomplete_reads() {
```
##########
lang/rust/avro/src/reader.rs:
##########
@@ -804,6 +804,38 @@ mod tests {
assert_eq!(expected_value, val);
}
+ #[test]
+ fn test_avro_3507_single_object_reader_incomplete_reads() {
+ let obj = TestSingleObjectReader {
+ a: 42,
+ b: 3.33,
+ c: vec!["cat".into(), "dog".into()],
+ };
+ let mut to_read_1 = Vec::<u8>::new();
+ to_read_1.extend_from_slice(&[0xC3, 0x01]);
Review Comment:
do we need the Vec+extend_from_slice() ? Can't we just do `let to_read_1 = vec!(0xC3, 0x01);` ?
Also please add a comment that those two bytes are the special magic ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] jklamer commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
jklamer commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997707042
##########
lang/rust/avro/src/reader.rs:
##########
@@ -804,6 +804,38 @@ mod tests {
assert_eq!(expected_value, val);
}
+ #[test]
+ fn test_avro_3507_single_object_reader_incomplete_reads() {
+ let obj = TestSingleObjectReader {
+ a: 42,
+ b: 3.33,
+ c: vec!["cat".into(), "dog".into()],
+ };
+ let mut to_read_1 = Vec::<u8>::new();
+ to_read_1.extend_from_slice(&[0xC3, 0x01]);
+ let mut to_read_2 = Vec::<u8>::new();
+ to_read_2.extend_from_slice(
+ &TestSingleObjectReader::get_schema()
+ .fingerprint::<Rabin>()
+ .bytes[..],
+ );
+ let mut to_read_3 = Vec::<u8>::new();
+ encode(
+ &obj.clone().into(),
+ &TestSingleObjectReader::get_schema(),
+ &mut to_read_3,
+ )
+ .expect("Encode should succeed");
+ let mut to_read = (&to_read_1[..]).chain(&to_read_2[..]).chain(&to_read_3[..]);
Review Comment:
interesting. `read` method would fail in this case? not would I would expect. Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] davidpeklak commented on a diff in pull request #1914: AVRO-3642: [RUST] Fix GenericSingleObjectReader::read_value for non-exhaustive reads
Posted by GitBox <gi...@apache.org>.
davidpeklak commented on code in PR #1914:
URL: https://github.com/apache/avro/pull/1914#discussion_r997769175
##########
lang/rust/avro/src/reader.rs:
##########
@@ -396,9 +396,9 @@ impl GenericSingleObjectReader {
pub fn read_value<R: Read>(&self, reader: &mut R) -> AvroResult<Value> {
let mut header: [u8; 10] = [0; 10];
- match reader.read(&mut header) {
- Ok(size) => {
- if size == 10 && self.expected_header == header {
+ match reader.read_exact(&mut header) {
+ Ok(_) => {
Review Comment:
`reader.read_exact` returns `Result<()>`, so it is not possible to match to 10 here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org