graphql-rust / graphql-parser

A graphql query language and schema definition language parser and formatter for rust
Apache License 2.0
351 stars 74 forks source link

Add Serde Support For AST #77

Closed anshap1719 closed 2 months ago

LegNeato commented 9 months ago

Should this support deserializing as well?

anshap1719 commented 9 months ago

@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 commented 8 months ago

@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.

LegNeato commented 8 months ago

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

tyranron commented 8 months ago

@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.

LegNeato commented 8 months ago

Ah yeah, I was in the wrong project 🤣

sodiumjoe commented 3 months ago

👋 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 commented 2 months ago

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

anshap1719 commented 2 months ago

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