fix: return proper error rather than persisting error message on snapshot #1960
Conversation
…shot Signed-off-by: StandingMan <jmtangcs@gmail.com>
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Hi @Standing-Man , thanks for the contribution! Looks good overall, I just have some minor feedbacks
| (Some(_), Some(_)) => { | ||
| return Err(Error::new( | ||
| ErrorKind::Unexpected, | ||
| "v1 snapshot invariant violated: manifest_list and manifests are both set", |
There was a problem hiding this comment.
| "v1 snapshot invariant violated: manifest_list and manifests are both set", | |
| "Invalid v1 snapshot, when manifest list provided, manifest files should be omitted", |
I think the original error message is more specific
| (None, _) => { | ||
| return Err(Error::new( | ||
| ErrorKind::FeatureUnsupported, | ||
| "v1 snapshot invariant violated: manifest_list is missing", |
There was a problem hiding this comment.
| "v1 snapshot invariant violated: manifest_list is missing", | |
| "Unsupported v1 snapshot, only manifest list is supported", |
Same here, the original message is more specific
| }, | ||
| (Some(_), Some(_)) => { | ||
| return Err(Error::new( | ||
| ErrorKind::Unexpected, |
There was a problem hiding this comment.
| ErrorKind::Unexpected, | |
| ErrorKind::DataInvalid, |
Referring to the v3 parsing and error handling logic here, DataInvlid seems more apporpriate
| } | ||
| (None, _) => { | ||
| return Err(Error::new( | ||
| ErrorKind::FeatureUnsupported, |
There was a problem hiding this comment.
| ErrorKind::FeatureUnsupported, | |
| ErrorKind::DataInvalid, |
Same here
| "#; | ||
|
|
||
| let result = serde_json::from_str::<TableMetadata>(metadata); | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
We should also check if the error has the correct error type/message. So other developers will know what we are testing here
| } | ||
| "#; | ||
| let result = serde_json::from_str::<TableMetadata>(metadata); | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
Same here, we should also check if the error has the correct error type/message.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Avoid storing error message on snapshot, return error instead.
Are these changes tested?
Yes