graphql-parser icon indicating copy to clipboard operation
graphql-parser copied to clipboard

Add Serde Support For AST

Open anshap1719 opened this issue 2 years ago • 6 comments

anshap1719 avatar Nov 27 '23 05:11 anshap1719

Should this support deserializing as well?

LegNeato avatar Dec 06 '23 21:12 LegNeato

@LegNeato I think that would be ideal, yes. Sorry I didn't realize that I didn't add it in. WIll make the change.

anshap1719 avatar Dec 07 '23 03:12 anshap1719

@LegNeato I now remember why I didn't add it initially. It's because of having to deal with lifetimes and I didn't want to bother with that at the time. I will eventually (hopefully soon) add it in, but just wanted to give you an update here.

anshap1719 avatar Jan 02 '24 08:01 anshap1719

@tyranron are you ok with this landing without deserialize or do you want to wait? (I haven't reviewed yet FYI)

LegNeato avatar Jan 02 '24 12:01 LegNeato

@LegNeato I think we need @tailhook's decision/opinion here as a main maintainer of this crate. I haven't actually dived into graphql-parser's code yet.

tyranron avatar Jan 05 '24 14:01 tyranron

Ah yeah, I was in the wrong project 🤣

LegNeato avatar Jan 05 '24 16:01 LegNeato

👋 I need both serialization and deserialization for a project I'm working on. I am willing to try to finish this PR by adding a test and also adding deserialization, but the second one might be a bit beyond my abilities.

I checked out the wip commit before the reverts of the deserialization, but haven't been able to fix the lifetime issues. If someone is willing and available to provide some guidance, I'm still happy to try to make progress on this.

The lifetime issues are pretty tricky as they seem to come from the derive macro implementation.

On this line, the + Serialize + Deserialize<'a> seem unnecessary, as the implementer of the Text trait should impl them if required.

When I try removing those, I get fewer (10) compiler errors, that look like this

help: `'de` and `'a` must be the same: replace one with the other

error: lifetime may not live long enough
  --> src/schema/ast.rs:43:25
   |
38 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
   |                                                 ----------- lifetime `'de` defined here
39 | pub enum Definition<'a, T: Text<'a>> {
   |                     -- lifetime `'a` defined here
...
43 |     DirectiveDefinition(DirectiveDefinition<'a, T>),
   |                         ^^^^^^^^^^^^^^^^^^^ requires that `'de` must outlive `'a`
   |
   = help: consider adding the following bound: `'de: 'a`

I tried using serde(borrow = "'a") as described here, but that doesn't seem to work.

Any help appreciated!

sodiumjoe avatar Jun 14 '24 23:06 sodiumjoe

I got some help from a colleague: https://github.com/graphql-rust/graphql-parser/pull/80

sodiumjoe avatar Jun 19 '24 21:06 sodiumjoe

Thanks @sodiumjoe, I'll close this PR in favour of yours.

anshap1719 avatar Jun 20 '24 02:06 anshap1719