Tighten JsonPointer and methods

[jeddenlea]

The JsonPointer type ought to be #[repr(transparent)] if std::mem::transmute is going to be used to coerce one from a str. ReferenceToken already did.

The validate method did not ensure that a string starts with a /. This may not have led a direct path to any undefined behavior, because it was ensured that all ~ are followed by a 0 or 1, but other methods do act as though they assumed a pointer string always starts with a /. I think this makes things a bit more clear.

The private token_end can be mostly replaced by find("/").

as_array_index can be implemented in terms of the standard library's integer parsing. (Allowing "+000" is an annoying gotcha in seemingly every major language's standard library!).

I've added Deserialize for &JsonPointer.

I've made the internals of the new json_pointer! macro const {} to better ensure that its checks actually happen at compile time.

I've adjusted some methods to reduce the total number of unsafe blocks

[CLAassistant]

All committers have signed the CLA.

[timothee-haudebourg]

Hi, thanks for the contribution it definitely looks like an improvement. However we moved the JSON pointer implementation in recent changes into ssi-core, and made the validation function const. You'll have to update you implementation accordingly before I can accept it.

[jeddenlea]

Hey @timothee-haudebourg . const JsonPointers are definitely neat. But, I'm curious, was there a particular reason that you needed to switch the internals of each of the types to be Vec<u8>/[u8] instead of String/str?

[jeddenlea]

I've taken a leap and am assuming that #612 felt forced to change the internals of these types from str/String to [u8]/Vec<u8> to accommodate the new BytesBuf signatures and const validation, as I've run into problems like that before. However, it doesn't appear necessary in this case. Given that these types all do require their internals to be UTF-8, it seems more straightforward to use str/String directly and not worry about yet another implied invariant for unsafe. But, if I am missing some detail, please let me know and I'll change them back!

[timothee-haudebourg]

Thanks for the effort 😄 It doesn't really matter what internal type is used in the wrapper types, but having the validate and new_unchecked function accept [u8] is important for parsing JSON pointers from byte strings without having to call str::from_utf8 before. You can decline validate into validate_bytes and validate_str (the former using the later) if you want to avoid the inner call to str::from_utf8.

[jeddenlea]

Ok, I've split the verify functions up. I've tried to ensure there is always a path for str/String to become JsonPointers without the utf-8 check being done again.

This means that in general, if you already have a &str, then from_str_const is the better method to construct a JsonPointer. And, if you already have a String, then try_from is best.

I also changed JsonPointerBuf::deserialize to use try_into instead of parse, as from_str implied making an extra copy.

[timothee-haudebourg]

The CI is failing but that's expected and fixed in another PR. Thanks again!

[timothee-haudebourg]

I don't think that was necessary since we can always use s.as_bytes() even in const context, but why not.