juniper icon indicating copy to clipboard operation
juniper copied to clipboard

feat: add integration of serde_json

Open chirino opened this issue 4 years ago • 20 comments

Resolves #957
Fixes #280, #504, #509
Superseeds #325

Implements the GraphQLValue trait for serde_json::Value. It's type info is given using an SDL definition.

Checklist

  • Implemented:
    • [x] GraphQLType and GraphQLValue traits
    • [x] GraphQLAsyncValue trait
    • [x] IsOutputType marker and IsInputType marker
  • Tested as:
    • [ ] input
    • [x] field of input type
    • [x] output
    • [x] field of output type
    • [x] subsctiption return type (Strem::Item)
    • [x] in async context
  • [ ] Described in Book
  • [ ] CHANGELOG entry added

chirino avatar Aug 14 '21 23:08 chirino

Also what do you mean by tested as field of input/output type? Does that mean as field of a struct using the #[derive(GraphQLObject)] macro?

chirino avatar Aug 15 '21 13:08 chirino

@chirino yes

#[derive(GraphQLInputObject)]
struct InputObj {
    val: serde_json::Value,
}

struct Human;

#[graphql_object]
impl Human {
    fn output() -> serde_json::Value {
    }

    fn input(arg: serde_json::Value) -> bool {
    }

    fn input_object(obj: InputObj) -> bool {
    }
}

tyranron avatar Aug 15 '21 17:08 tyranron

Sadly we can't do something as simple as:

#[derive(GraphQLInputObject)]
struct InputObj {
    val: serde_json::Value,
}

Since we need a way to associate type info with the val field. I did included a test that shows how you can create a wrapper type around serde_json::Value that provides that type info.. but it's a bit verbose. Not sure if there is a way to reduce that.

Do you have an example of how you test the subsctiption return type case?

chirino avatar Aug 16 '21 02:08 chirino

@chirino

I did included a test that shows how you can create a wrapper type around serde_json::Value that provides that type info.

Does it worth to have such a wrapper predefined?

Do you have an example of how you test the subsctiption return type case?

See integration_tests/juniper_tests/src/codegen/subscription_attr.rs.

tyranron avatar Aug 16 '21 07:08 tyranron

@tyranron

I think the PR should have about the right stuff covered in the tests. It introduces a couple of new public types... and naming things is hard so let me know if you can think of better names for:

  • TypeInfo
  • TypedJsonInfo
  • TypedJson

Also not sure where to document this in the book.

chirino avatar Aug 16 '21 20:08 chirino

@chirino go with the names you think are best. I'll bikeshed them in the final review if a better naming will come up in my mind.

Also not sure where to document this in the book.

I think it would be nice to create a separate page in advanced topics.

tyranron avatar Aug 17 '21 09:08 tyranron

Minor nit, but we probably want to match the integration crate name with the integration file name with the flag name. That is, serde_json everywhere instead of json.

LegNeato avatar Aug 18 '21 06:08 LegNeato

renamed and added a section to the book. Is anything else needed?

chirino avatar Aug 22 '21 20:08 chirino

@chirino thanks! I'll look at it in few days.

tyranron avatar Aug 23 '21 08:08 tyranron

This is great work, thank you! ❤️

LegNeato avatar Aug 26 '21 08:08 LegNeato

Just to keep everybody up-to-date:
This PR is not abbandoned. I'm working on it right now to extend the work dony by @chirino. It will take quite a while, because the rabbit hole appeared to be deeper than expected.

tyranron avatar Sep 17 '21 12:09 tyranron

@tyranron thanks for the update. Do you have a branch of your work that I can peek at? Maybe I can find some time to help.

chirino avatar Sep 20 '21 15:09 chirino

@chirino unfortunately no, but thank you. You've already done so much 🙇‍♂️

I just need to somewhat extend that, fix some edge cases and polish. It will be harder to explain than do and finish.

tyranron avatar Sep 20 '21 16:09 tyranron

Wow, this PR has turned to be such a huge rabbit hole 😅
It has spotted so many juniper design weak points. And some of them I intend to fix in separate PRs before merging this.

Sorry for moving it slowly, but I'd like to do it the proper way from the start, rather than having it half-baked or redoing in future.

tyranron avatar Oct 08 '21 12:10 tyranron

No rush. Glad my "first ever rust PR" is nudging this project even to become even better.

chirino avatar Oct 08 '21 13:10 chirino

@tyranron hey.. I know this is a complex PR and I'm a noob, but feel free to reach out to me if you need help with it.

chirino avatar Feb 22 '22 15:02 chirino

@tyranron are you still planning on working on this PR? Super excited to get serde_json integration

juancampa avatar Jul 29 '22 20:07 juancampa

@juancampa yes, of course. It's scheduled for 0.16.0 release. But it requires first the #1072 to be completed, as at the moment there is no way to combine dynamic GraphQL schema with a static one, due to TypeInfo being an associative type, so allowing no polymorphism.

tyranron avatar Aug 08 '22 11:08 tyranron