Not validating tinystr keys #3406
Conversation
robertbastian
requested review from
zbraniecki,
sffc,
Manishearth,
snktd,
dminor,
nciric and
a team
as code owners
| if deserializer.is_human_readable() { | ||
| Ok(TinyAsciiStr::deserialize(deserializer)?.raw()) | ||
| } else { | ||
| Ok(Self(<[u8; $size]>::deserialize(deserializer)?)) |
There was a problem hiding this comment.
Nit: Please manually invoke deserialize_bytes because otherwise this might use deserialize_seq which is probably not as efficient.
There was a problem hiding this comment.
I believe because they're fixed-size, arrays go through deserialize_tuple
| use crate::TinyStrError; | ||
|
|
||
| #[derive(Debug, PartialEq, PartialOrd, Eq, Ord, Clone, Copy)] | ||
| pub struct UnvalidatedTinyAsciiStr<const N: usize>(pub(crate) [u8; N]); |
There was a problem hiding this comment.
TinyAsciiStr itself doesn't have docs...
robertbastian
requested review from
sffc
and removed request for
dminor,
zbraniecki,
nciric and
snktd
| #[derive(Debug, PartialEq, PartialOrd, Eq, Ord, Clone, Copy)] | ||
| pub struct UnvalidatedTinyAsciiStr<const N: usize>(pub(crate) [u8; N]); | ||
|
|
||
| impl<const N: usize> UnvalidatedTinyAsciiStr<N> { | ||
| #[inline] | ||
| // Converts into a [`TinyAsciiStr`]. Fails if the bytes are not valid ASCII. | ||
| pub fn try_into_tinystr(&self) -> Result<TinyAsciiStr<N>, TinyStrError> { |
There was a problem hiding this comment.
thought: could also be called try_into_validated()
(or just a TryInto impl?)
There was a problem hiding this comment.
TryInto isn't as chainable, I added as_tinystr to the subtags in the PR for the same reason.
There was a problem hiding this comment.
What do you mean by chainable here? TryInto seems like the right trait for me and it abstracts better for generic inputs.
There was a problem hiding this comment.
I think he's talking about how inference does not succeed if you do stuff like foo.try_into().bar()
There was a problem hiding this comment.
We definitely should have TryInto, IMO it's acceptable to also have an inherent impl.
There was a problem hiding this comment.
Ah, ok, for any further readers, Robert meant this: TryInto::<Bar>::try_into(Foo {})?.baz(); being ugly.
There was a problem hiding this comment.
UnvalidatedStr doesn't have a TryInto either
Fixes #2489