Drop Rustc Serialization Support #43

TheServerAsterisk · 2017-04-25T19:09:47Z

This will close #36.

One notable change is the GraphQlIronError enum I added. The purpose of this enum was to capture any serde or IO errors and log the error instead of making the thread panic if serde failed or the user failed to put in a valid JSON object.

TheServerAsterisk · 2017-04-25T19:11:47Z

src/integrations/iron_handlers.rs


-use ::{InputValue, GraphQLType, RootNode, Variables, execute};
+use ::{InputValue, GraphQLType, RootNode, Variables, execute as execute_query};


Changed the name of this in order to clarify the difference between the execute function that exists in the iron_handlers file vs the execute function that resides in the lib.rs file.

TheServerAsterisk · 2017-04-25T19:16:24Z

src/integrations/iron_handlers.rs


        match result {
            Ok((result, errors)) => {
-                map.insert("data".to_owned(), result.to_json());
+                let response_data = serde_json::to_value(result)
+                    .expect("Failed to convert response data to JSON.");


Wasn't sure how you, @mhallin, wanted handed handle errors at this points especially since the response has probably been parsed already and should only fail if the developer messes up.

Yeah, if things fail here the only way out is to fail - whether through a clean exit or a panic which Iron will handle anyway doesn't really matter.

TheServerAsterisk · 2017-04-25T19:18:13Z

src/integrations/serde.rs

+    /// Converts serde_json::Value to juniper::InputValue
+    pub fn from_json(json: Json) -> InputValue {
+        match json {
+          Json::Number(num) => {


Since Serde doesn't have I64 etc but just a general number it is unclear whether u64 should be converted to an i64 as well. Please note if you want me to handle this case differently.

I think we've got two options: either add u64 to InputValue and Value, or convert it to either i64 or f64 if it's larger than i64::MAX.

BTW, I think it would be cleaner if this code read like:

if let Some(num) = num.as_i64() { InputValue::int(num) } else if let Some(num) = num.as_f64() { InputValue::float(num) }

TheServerAsterisk · 2017-04-25T19:51:18Z

src/integrations/iron_handlers.rs

+                    if k == "query" {
+                        query = serde_json::to_string(&v)
+                            .ok()
+                            .map(|query| query.replace("\"", ""));


sede_json::to_string will convert the query value to something like this. "\"{hero{name}}\"" thus the need to replace it.

This sounds like a case of double encoding - what happens if you send {droid(id: "1001"){name}}?

…eferenced rustc.

- Added a GraphQlIronError - Changed execute from lib.rs to execute_query

TheServerAsterisk · 2017-05-01T02:31:44Z

@mhallin I made the appropriate changes, let me know if you want anythings else to be altered.

zaeleus · 2017-05-08T03:39:01Z

src/integrations/iron_handlers.rs

-                query = v.as_string().map(|s| s.to_owned());
+        let mut request_payload = String::new();
+        itry!(req.body.read_to_string(&mut request_payload));
+        let json_data =


This section can be simplified by using serde's derive macro for deserialization instead of manually pulling out values, e.g., https://github.com/zaeleus/juniper/blob/a321204cfc8d378fe3d8d9af2c817476cba1211d/examples/rocket/src/graphql.rs#L32-L67

I also ended up taking your WrappedGraphQlResult idea and placed both it and the GraphQlQuery into the serde module. Let me know if you have any more suggestions on how to improve the code.

zaeleus · 2017-05-08T03:41:11Z

src/integrations/serde.rs

+
+impl InputValue {
+    /// Converts serde_json::Value to juniper::InputValue
+    pub fn from_json(json: Json) -> InputValue {


Deserialization is already implemented for InputValue. Why not use it?

…ait and added a serialization implementation for the graphql result.

…erage the GraphQlQuery struct. Added a couple more tests for get requests.

TheServerAsterisk · 2017-05-15T02:21:42Z

Ahhh, I forgot serde_derive is only support for rust 1.15 and greater. So I can either drop serde_derive as a dependency and manually implement serialization or support for rust 1.13 and rust 1.14 can be dropped. If the former let me know @mhallin and I will make the appropriate changes.

theduke · 2017-05-15T05:52:28Z

I'd vote for just dropping older Rust versions.

mhallin · 2017-05-18T07:29:10Z

Yeah, I think this is where we drop 1.14 and older :) The reason I let Travis test on older Rust versions is to not accidentally dropping support for older versions.

norcalli · 2017-06-12T20:09:29Z

From my reading, this seems to be ready. And this would fix Travis failing for the serde 1.0 PR as well, since that was only failing on <=1.14. If this PR is left stale for too long, forks may appear that will cause fragmentation of the codebase.

TheServerAsterisk commented Apr 25, 2017

View reviewed changes

TheServerAsterisk added 5 commits April 30, 2017 20:07

Drop support for rustc-serialization and use serde by default.

41112d0

Removed rustc-serialization as dependency and updated the docs that r…

9adc0c4

…eferenced rustc.

- Replaced rustc-serialization logic with Serde.

54cfbeb

- Added a GraphQlIronError - Changed execute from lib.rs to execute_query

Removed rustc-serialization as a dependency.

494628a

Implemented from_json for InputValue.

1e334be

TheServerAsterisk changed the title ~~WIP: Drop rustc serialization~~ Drop rustc serialization May 4, 2017

TheServerAsterisk changed the title ~~Drop rustc serialization~~ Drop Rustc Serialization Support May 4, 2017

zaeleus reviewed May 8, 2017

View reviewed changes

TheServerAsterisk added 5 commits May 14, 2017 19:30

Added serde_derive and urlencoded as dependecies.

6c4b329

Removed from_json, leveraged the already implemented serialization tr…

e890e9b

…ait and added a serialization implementation for the graphql result.

Removed iexpect macro, added urlencoded and serde_derive dependencies.

998faec

Added more support for get requests and the get and post handlers lev…

d3b1433

…erage the GraphQlQuery struct. Added a couple more tests for get requests.

Added conditional compliation logic.

ba15579

Drop support for rust 1.13 and 1.14

80de43e

mhallin merged commit 80de43e into graphql-rust:master Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Rustc Serialization Support #43

Drop Rustc Serialization Support #43

TheServerAsterisk commented Apr 25, 2017 •

edited

Loading

TheServerAsterisk Apr 25, 2017

TheServerAsterisk Apr 25, 2017

mhallin Apr 30, 2017

TheServerAsterisk Apr 25, 2017

mhallin Apr 30, 2017

TheServerAsterisk Apr 25, 2017 •

edited

Loading

mhallin Apr 30, 2017

TheServerAsterisk commented May 1, 2017

zaeleus May 8, 2017

TheServerAsterisk May 15, 2017

zaeleus May 8, 2017

TheServerAsterisk commented May 15, 2017

theduke commented May 15, 2017

mhallin commented May 18, 2017

norcalli commented Jun 12, 2017


		use ::{InputValue, GraphQLType, RootNode, Variables, execute};
		use ::{InputValue, GraphQLType, RootNode, Variables, execute as execute_query};

Drop Rustc Serialization Support #43

Drop Rustc Serialization Support #43

Conversation

TheServerAsterisk commented Apr 25, 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

TheServerAsterisk Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheServerAsterisk commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheServerAsterisk commented May 15, 2017

theduke commented May 15, 2017

mhallin commented May 18, 2017

norcalli commented Jun 12, 2017

TheServerAsterisk commented Apr 25, 2017 •

edited

Loading

TheServerAsterisk Apr 25, 2017 •

edited

Loading