Update Serde to 1.0 #28

mike-engel · 2017-04-21T22:24:55Z

This is PR updates serde to 1.0. The README and Cargo file have been updated to use 1.0. This will close out #27.

mike-engel · 2017-04-21T22:25:37Z

~~As of right now, there are two lifetime issues here and here. I've tried using str::from_utf8, but that's no good either since then decoded goes out of scope too quick.~~

~~@Keats I'll keep trying here, but I don't think I have enough Rust experience with how to combat these two lifetime issues. If you have any ideas, let me know!~~

Fixed!

``` error: `s` does not live long enough --> src/serialization.rs:31:18 | 31 | Ok(from_str(&s)?) | ^ does not live long enough 32 | } | - borrowed value only lives until here | note: borrowed value must be valid for the lifetime 'a as defined on the body at 27:85... --> src/serialization.rs:27:86 | 27 | pub fn from_jwt_part<'a, B: AsRef, T: Deserialize<'a>>(encoded: B) -> Result { | ______________________________________________________________________________________^ starting here... 28 | | let decoded = base64::decode_config(encoded.as_ref(), base64::URL_SAFE_NO_PAD)?; 29 | | let s = String::from_utf8(decoded)?; 30 | | 31 | | Ok(from_str(&s)?) 32 | | } | |_^ ...ending here

error: s does not live long enough
--> src/serialization.rs:41:31
|
41 | let claims: T = from_str(&s)?;
| ^ does not live long enough
...
44 | }
| - borrowed value only lives until here
|
note: borrowed value must be valid for the lifetime 'a as defined on the body at 37:40...
--> src/serialization.rs:37:41
|
37 | -> Result<(T, Map<String, Value>)> {
| _________________________________________^ starting here...
38 | | let decoded = base64::decode_config(encoded.as_ref(), base64::URL_SAFE_NO_PAD)?;
39 | | let s = String::from_utf8(decoded)?;
40 | |
41 | | let claims: T = from_str(&s)?;
42 | | let map: Map<_, > = from_str(&s)?;
43 | | Ok((claims, map))
44 | | }
| |^ ...ending here

</details>

Keats · 2017-04-22T01:34:20Z

Thanks!
Actually I just noticed there is a https://docs.serde.rs/serde/de/trait.DeserializeOwned.html trait that might be more what we want since as a library we don't really know how the end struct is going to be used.

Is that how it's meant to be used @dtolnay (sorry for the random ping)?

dtolnay · 2017-04-22T01:43:43Z

I always appreciate a ping! Yes, both from_jwt_part and from_jwt_part_claims require T: DeserializeOwned because they are deserializing from the base64-decoded data so they shouldn't be allowed to borrow anything (the base64-decoded data goes out of scope when the function returns).

I will leave a few review comments as well.

dtolnay · 2017-04-22T01:46:41Z

src/lib.rs

+                 algorithm: Algorithm,
+                 validation: &Validation)
+                 -> Result<TokenData<T>>
+    where for<'a> T: Deserialize<'a>


This would be more readable as T: DeserializeOwned. This function requires that T can deserialize without borrowing any data because the string it is deserialized from is thrown away before this function returns.

dtolnay · 2017-04-22T01:49:35Z

src/serialization.rs

 use header::Header;


 /// The return type of a successful call to decode
 #[derive(Debug)]
-pub struct TokenData<T: Deserialize> {
+pub struct TokenData<T>
+    where for<'a> T: Deserialize<'a>


This bound should not be here. Structs in Rust should never have trait bounds. Trait bounds are for behavior.

impl<T> Data<T> where T: Behavior { /* behavior, okay */ } fn behave<T>() where T: Behavior { /* also behavior, also okay */ } struct Data<T> where T: Behavior { /* don't do this */ }

The only exception is things like Cow that use associated types to define data.

Thanks for the clarification. If it's not obvious, I'm new to Rust, and this result was me fighting with lifetime errors from the compiler 😞

I filed https://github.com/Manishearth/rust-clippy/issues/1689 to catch this case with a warning in Clippy. Apparently even the standard library does it wrong sometimes!

dtolnay · 2017-04-22T01:50:50Z

src/crypto.rs

-        hmac::sign(&signing_key, signing_input.as_bytes()).as_ref(),
-        base64::URL_SAFE_NO_PAD
-    ))
+    Ok(base64::encode_config(hmac::sign(&signing_key, signing_input.as_bytes()).as_ref(),


Next time please make whitespace changes in a separate commit. They makes it hard to find and review the thing you are actually changing.

Agreed on that, @mike-engel can you revert all the formatting changes for now?
Is it rustfmt using the latest config? I thought it would be more block indented than that

Block style is currently recommended by the fmt-rfc folks but not the default in rustfmt. This is the settings they recommend.

More official source I guess: here.

Yeah, sorry about that! Turns out my rustfmt was old. Would you prefer me to avoid rustfmt altogether, or add a config like the two linked from serde & rustmft?

Avoid it for now please, we can run it later

mike-engel · 2017-04-22T05:26:30Z

Ok, updated now with DeserializedOwned, which made everything much better. Sorry I didn't see that in the docs before, and thanks to @dtolnay for the suggestions, clarifications, work the work on serde 😄

Tests and benchmarks run fine. If there's anything else you'd like me to fix/edit/add, let me know.

dtolnay · 2017-04-22T05:29:30Z

src/serialization.rs

@@ -10,7 +10,7 @@ use header::Header;

 /// The return type of a successful call to decode
 #[derive(Debug)]
-pub struct TokenData<T: Deserialize> {
+pub struct TokenData<T: DeserializeOwned> {


This bound should be removed: just pub struct TokenData<T> { ... }. See https://github.com/Manishearth/rust-clippy/issues/1689.

Keats · 2017-04-22T06:00:59Z

Thanks to both of you!

Update serde, serde_json, and serde_derive to 1.0

ca59ac1

dtolnay reviewed Apr 22, 2017

View reviewed changes

Migrate Deserialize trait to DeserializeOwned for Serde 1.0

0932f14

mike-engel force-pushed the serde-1.0 branch from 046c6e4 to 0932f14 Compare April 22, 2017 05:21

Update the readme to suggest serde 1.0 over 0.9

56e2f97

dtolnay reviewed Apr 22, 2017

View reviewed changes

Remove trait from TokenData struct

cbd9b23

Keats merged commit d8439b1 into Keats:master Apr 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Serde to 1.0 #28

Update Serde to 1.0 #28

mike-engel commented Apr 21, 2017 •

edited

Loading

mike-engel commented Apr 21, 2017 •

edited

Loading

Keats commented Apr 22, 2017

dtolnay commented Apr 22, 2017 •

edited

Loading

dtolnay Apr 22, 2017

dtolnay Apr 22, 2017

mike-engel Apr 22, 2017

dtolnay Apr 22, 2017

dtolnay Apr 22, 2017

Keats Apr 22, 2017

dtolnay Apr 22, 2017

dtolnay Apr 22, 2017

mike-engel Apr 22, 2017

Keats Apr 22, 2017

mike-engel commented Apr 22, 2017

dtolnay Apr 22, 2017

Keats commented Apr 22, 2017

Update Serde to 1.0 #28

Update Serde to 1.0 #28

Conversation

mike-engel commented Apr 21, 2017 • edited Loading

mike-engel commented Apr 21, 2017 • edited Loading

Keats commented Apr 22, 2017

dtolnay commented Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-engel commented Apr 22, 2017

Choose a reason for hiding this comment

Keats commented Apr 22, 2017

mike-engel commented Apr 21, 2017 •

edited

Loading

mike-engel commented Apr 21, 2017 •

edited

Loading

dtolnay commented Apr 22, 2017 •

edited

Loading