-
Notifications
You must be signed in to change notification settings - Fork 6
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
Validate bare items on construction #102
Open
zcei
wants to merge
17
commits into
undef1nd:master
Choose a base branch
from
zcei:validate-bare-items
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
5746d9e
feat: add BareItem::Validated* variants
zcei 7d3c842
tmp: bench example to show direct Token construction fails
zcei eb6cff8
refactor: move ValidatedInteger to Integer
zcei f33ec99
refactor: move ValidatedString to String
zcei 89019a9
feat: create bare_item::Decimal
zcei 246289f
refactor: move ValidatedToken to Token
zcei 179106c
refactor: move ValidatedByteSeq to ByteSeq
zcei 1a20e37
refactor: move ValidatedBoolean to Boolean
zcei 51fa5b3
test: cleanup serialize errors
zcei 635acc3
cleanup: remove parsing attempts from bare_items
zcei c1b6ff6
cleanup: remove temporary examples
zcei cf09aa7
refactor: use serializer to validate bare_item::Integer
zcei bb0faef
refactor: move BareItem enum into bare_item module
zcei b61fbea
refactor: move validation logic out of serializer
zcei 1481e0b
feat: add BareItem::new_* constructors
zcei 046450d
refactor: rename bare item value structs
zcei bf7db68
docs: use BareItem::new_* constructors
zcei File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,252 @@ | ||
use crate::serializer::Serializer; | ||
use std::{convert::TryFrom, fmt, ops::Deref}; | ||
|
||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct Decimal(pub(crate) rust_decimal::Decimal); | ||
|
||
impl TryFrom<rust_decimal::Decimal> for Decimal { | ||
type Error = &'static str; | ||
fn try_from(value: rust_decimal::Decimal) -> Result<Self, Self::Error> { | ||
let mut output = String::new(); | ||
Serializer::serialize_decimal(value, &mut output)?; | ||
|
||
Ok(Decimal(value)) | ||
} | ||
} | ||
|
||
impl Deref for Decimal { | ||
type Target = rust_decimal::Decimal; | ||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl fmt::Display for Decimal { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
|
||
/// Integers have a range of -999,999,999,999,999 to 999,999,999,999,999 inclusive (i.e., up to fifteen digits, signed), for IEEE 754 compatibility. | ||
/// | ||
/// The ABNF for Integers is: | ||
/// ```abnf,ignore,no_run | ||
/// sf-integer = ["-"] 1*15DIGIT | ||
/// ``` | ||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct Integer(pub(crate) i64); | ||
|
||
impl Deref for Integer { | ||
type Target = i64; | ||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl TryFrom<i64> for Integer { | ||
type Error = &'static str; | ||
|
||
fn try_from(value: i64) -> Result<Self, Self::Error> { | ||
let mut output = String::new(); | ||
Serializer::serialize_integer(value, &mut output)?; | ||
Ok(Integer(value)) | ||
} | ||
} | ||
|
||
impl fmt::Display for Integer { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
|
||
// TODO: how to get around naming collision without using std::string::String everywhere? | ||
zcei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// Strings are zero or more printable ASCII (RFC0020) characters (i.e., the range %x20 to %x7E). Note that this excludes tabs, newlines, carriage returns, etc. | ||
/// | ||
/// The ABNF for Strings is: | ||
/// ```abnf,ignore,no_run | ||
/// sf-string = DQUOTE *chr DQUOTE | ||
/// chr = unescaped / escaped | ||
/// unescaped = %x20-21 / %x23-5B / %x5D-7E | ||
/// escaped = "\" ( DQUOTE / "\" ) | ||
/// ``` | ||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct BareItemString(pub(crate) std::string::String); | ||
|
||
impl Deref for BareItemString { | ||
type Target = String; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl TryFrom<String> for BareItemString { | ||
type Error = &'static str; | ||
fn try_from(value: String) -> Result<Self, Self::Error> { | ||
let mut output = String::new(); | ||
Serializer::serialize_string(&value, &mut output)?; | ||
|
||
Ok(BareItemString(value)) | ||
} | ||
} | ||
|
||
impl fmt::Display for BareItemString { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
|
||
/// Byte Sequences can be conveyed in Structured Fields. | ||
/// | ||
/// The ABNF for a Byte Sequence is: | ||
/// ```abnf,ignore,no_run | ||
/// sf-binary = ":" *(base64) ":" | ||
/// base64 = ALPHA / DIGIT / "+" / "/" / "=" | ||
/// ``` | ||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct ByteSeq(pub(crate) Vec<u8>); | ||
|
||
impl From<&[u8]> for ByteSeq { | ||
fn from(value: &[u8]) -> Self { | ||
ByteSeq(value.to_vec()) | ||
} | ||
} | ||
|
||
impl From<Vec<u8>> for ByteSeq { | ||
fn from(value: Vec<u8>) -> Self { | ||
ByteSeq(value) | ||
} | ||
} | ||
|
||
impl Deref for ByteSeq { | ||
type Target = [u8]; | ||
fn deref(&self) -> &Self::Target { | ||
self.0.as_slice() | ||
} | ||
} | ||
|
||
/// Boolean values can be conveyed in Structured Fields. | ||
/// | ||
/// The ABNF for a Boolean is: | ||
/// ```abnf,ignore,no_run | ||
/// sf-boolean = "?" boolean | ||
/// boolean = "0" / "1" | ||
/// ``` | ||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct Boolean(pub(crate) bool); | ||
|
||
impl From<bool> for Boolean { | ||
fn from(value: bool) -> Self { | ||
Boolean(value) | ||
} | ||
} | ||
|
||
impl Deref for Boolean { | ||
type Target = bool; | ||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl fmt::Display for Boolean { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
|
||
/// Tokens are short textual words; their abstract model is identical to their expression in the HTTP field value serialization. | ||
/// | ||
/// The ABNF for Tokens is: | ||
/// ```abnf,ignore,no_run | ||
/// sf-token = ( ALPHA / "*" ) *( tchar / ":" / "/" ) | ||
/// ``` | ||
/// | ||
/// # Example | ||
/// ``` | ||
/// use sfv::{BareItem, Token}; | ||
/// use std::convert::{TryFrom, TryInto}; | ||
/// | ||
/// # fn main() -> Result<(), &'static str> { | ||
/// let token_try_from = Token::try_from("foo")?; | ||
/// let item = BareItem::Token(token_try_from); | ||
/// | ||
/// let str_try_into: Token = "bar".try_into()?; | ||
/// let item = BareItem::Token(str_try_into); | ||
/// | ||
/// let direct_item_construction = BareItem::Token("baz".try_into()?); | ||
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
/// | ||
/// ```compile_fail | ||
/// Token("foo"); // A Token can not be constructed directly | ||
/// ``` | ||
#[derive(Debug, PartialEq, Clone)] | ||
pub struct Token(pub(crate) String); | ||
|
||
impl Deref for Token { | ||
type Target = String; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl TryFrom<String> for Token { | ||
type Error = &'static str; | ||
fn try_from(value: String) -> Result<Self, Self::Error> { | ||
let mut output = String::new(); | ||
Serializer::serialize_token(&value, &mut output)?; | ||
|
||
Ok(Token(value)) | ||
} | ||
} | ||
|
||
impl TryFrom<&str> for Token { | ||
type Error = &'static str; | ||
fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
let mut output = String::new(); | ||
Serializer::serialize_token(&value, &mut output)?; | ||
Ok(Token(value.to_owned())) | ||
} | ||
} | ||
|
||
impl fmt::Display for Token { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use std::convert::TryInto; | ||
use std::error::Error; | ||
use std::str::FromStr; | ||
|
||
use super::*; | ||
|
||
#[test] | ||
fn create_non_ascii_string_errors() -> Result<(), Box<dyn Error>> { | ||
let disallowed_value: Result<BareItemString, &str> = | ||
"non-ascii text 🐹".to_owned().try_into(); | ||
|
||
assert_eq!( | ||
Err("serialize_string: non-ascii character"), | ||
disallowed_value | ||
); | ||
|
||
Ok(()) | ||
} | ||
|
||
#[test] | ||
fn create_too_long_decimal_errors() -> Result<(), Box<dyn Error>> { | ||
let disallowed_value: Result<Decimal, &str> = | ||
rust_decimal::Decimal::from_str("12345678912345.123")?.try_into(); | ||
assert_eq!( | ||
Err("serialize_decimal: integer component > 12 digits"), | ||
disallowed_value | ||
); | ||
|
||
Ok(()) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So, doing an actual serialization in the try_from implementation seems less than optimal.
That means for every bare item that we construct, we allocate a string we then throw away. And then we do the serialization again when we actually serialize it into a buffer.
I think what we should do here is split the serialization from the validity checks, so try_from for Decimal would only check if the decimal is in the allowed range, and not actually serialize it.
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.
Yeah I'm not happy with validating by serializing either, and as mentioned in the PR description we could (and should 😅) move validations to a place where both can access it.
To get my idea validated and not change another hundreds of lines, I decided to keep it like this for the moment - but I'm totally supportive of the fact that this needs to change.
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.
Created a new
ValidateValue
trait that the value structs implement. It will either return aResult
where theOk
value is either the input (passing back ownership) or a new "sanitized" value (important forDecimal
as we can round to three decimal places already)