Switch to serde for serialization/deserialization

[pimeys]

rustc_serialize is deprecated, serde taking it's place as the serialization solution. Using serde's nice macros we don't need our own JSON serialization code.

I could almost completely make the shift without breaking the API, but the custom data setup I had to change to accept anything that is deriving the Serialize functionality, so what that means is:

• The data parameter must be wrapped in a Box just so we can use almost anything that can be serialized as a parameter
• Which means, the custom data can now be made type-safe, which is a huge win at least in our projects.

I did a pull request to open up discussion first. This requires a major version change because of that broken API. I can bump the version and publish a new version if this looks OK to you.

---

This change is 

[pimeys]

Oh, and one thing I'd like to change here too: the response from FCM has a reason, which could basically be an enum. This would be another breaking change to our API here, but would be much nicer to work with in your application.

[panicbit]

I don't quite understand the reasoning behind taking Box in that API. Why wouldn't this work:

pub fn data<T: Serialize>(&mut self, data: T) -> Result<&mut MessageBuilder, serde_json::Error> { … }

It would mirror the signature of serde_json::to_value and still allow passing a Box if needed.

edit:
Nvm, after reading the erased-serde readme I think I get it. Though, if you are going to serialize the data right away I'd probably use &Serialize instead of Box<Serialize>.

[panicbit]

Reviewed 8 of 10 files at r1.
Review status: 8 of 10 files reviewed at latest revision, 2 unresolved discussions.

---

src/lib.rs, line 41 at r1 (raw file):

//!
//! let mut builder = fcm::MessageBuilder::new("<FCM API Key>", "<registration id>");
//! builder.data(Box::new(map));

builder.data(&map) if we decide to change the signature of data.

---

src/message/mod.rs, line 186 at r1 (raw file):

    /// let message = builder.finalize();
    /// ```
    pub fn data<'a>(&mut self, data: Box<Serialize>) -> Result<&mut MessageBuilder, serde_json::Error> {

1. I think the lifetime parameter is not needed
2. I think we should accept &Serialize instead of Box<Serialize>

---

Comments from Reviewable

[panicbit]

Review status: 8 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

examples/simple_sender.rs, line 37 at r1 (raw file):

    let mut builder = MessageBuilder::new(api_key.as_ref(), device_token.as_ref());
    builder.data(Box::new(data));

builder.data(&data) if we decide to change the signature of data.

---

src/message/mod.rs, line 183 at r1 (raw file):

    ///
    /// let mut builder = MessageBuilder::new("<FCM API Key>", "<registration id>");
    /// builder.data(Box::new(map));

builder.data(&map) if we decide to change the signature of data.

---

src/message/tests.rs, line 40 at r1 (raw file):

    };

    builder.data(Box::new(data)).unwrap();

builder.data(&data) if we decide to change the signature of data.

---

Comments from Reviewable

[pimeys]

You were totally right about that Box. You only need it if you want to store Serialize in a struct. What about turning those response reasons into an enum?

[panicbit]

Are you talking about the response reasons that are mentioned here in Table 4 for status code 400?
Are the possible reasons documented somewhere? Even if they are, I'd probably be against using an enum because the reasons might not be exhaustive (similar to how/why the mime crate doesn't use enums).

[panicbit]

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/message/mod.rs, line 186 at r1 (raw file):

Previously, panicbit wrote…

1. I think the lifetime parameter is not needed
2. I think we should accept &Serialize instead of Box<Serialize>

What about the unused lifetime parameter?

---

Comments from Reviewable

[pimeys]

Missed that lifetime parameter. Fixed.

For the enum, if it would be possible to have one that is Other(String), that would solve it? I just like to map enums for the responses, it's more safe to catch typos with the compiler.

[panicbit]

You could do that, but then adding a new variant can potentially lead to bugs. If someone is matching against Other and then you add a variant for that particular reason, then it breaks the code (while still being compilable if there's a catch all arm).
Typos could be just as easily avoided by providing constants (like the mime crate does).

OTOH, while providing the most flexibility, using consts allows for using the API the wrong way (e.g. by not using the available constants). Maybe we would provide a function that gives you an enum with an opaque/private Other variant (so that nobody can match against its contents) and also provide a function to get the raw string.

It would be nice to have RFC 2008 implemented.

OK, so my final thought is: If RFC 2008 gets implemented it's probably best to use an enum, and even if it is not, this crate is still pre 1.0 so we can still more or less freely break stuff. So I'm fine with an enum for now.

[pimeys]

Well, it's pre 1.0, but we push millions of notifications through this library and my consumer every day. So let's try to make it right still :)

[panicbit]

So what do you think is the best option? After thinking about RFC 2008 a bit more it doesn't seem to be that useful in our case. I think we should simply make the contents of the Other variant opaque. If someone needs check for a not-yet-implemented reason, I assume they would either exhaustively match against all variants and extract the raw reason in the Other case OR they would not match against Other and have a wildcard case in witch they extract the raw reason. Adding a new variant for a reason would either stop the code from compiling (former case) or just continue to work (latter case). The only case where this design pattern becomes problematic is if someone matches against Other AND has a wildcard match arm.

Here's some code for clarification:

Setup:

enum Reason {
    Foo,
    Bar,
    Other(OpaqueReason),
}

Case 1, exhaustive (ok):

match reason {
    Reason::Foo => …,
    Reason::Bar => …,
    Reason::Other(..) => {
        // acquire raw reason somehow and deal with it
    }
}

Adding a new variant in case 1 causes a compile-time error, which I think is good.

Case 2, wildcard (ok):

match reason {
    Reason::Foo => …,
    Reason::Bar => …,
    _ => {
        // acquire raw reason somehow and deal with it
    }
}

Adding a new variant in case 2 wouldn't have an effect. The code would still be able to observe the expected reasons.

Case 3, wildcard (bad):

match reason {
    Reason::Foo => …,
    Reason::Other => {
        // acquire raw reason somehow and deal with it
    },
    _ => {} // Ignore other errors
}

Adding a new variant in case 3 could cause the Other arm to be skipped and ignored as a result.

We could also hide the Other variant from the docs, prepend "__" and add some warnings in the comments. If nobody ever tries to match against Other there should be no problem.

[panicbit]

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[panicbit]

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[pimeys]

The enums in responses are not done quickly. I need to think this whole Response thingy a bit more here. If you're happy with this pull request, go ahead and merge it, I'll try it on production then ;)