Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add integration of serde_json #975

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft

Conversation

chirino
Copy link

@chirino chirino commented Aug 14, 2021

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:
    • GraphQLType and GraphQLValue traits
    • GraphQLAsyncValue trait
    • IsOutputType marker and IsInputType marker
  • Tested as:
    • input
    • field of input type
    • output
    • field of output type
    • subsctiption return type (Strem::Item)
    • in async context
  • Described in Book
  • CHANGELOG entry added

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems labels Aug 15, 2021
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chirino thanks for bringing this up! 🍻

Seems to be a nice implementation idea for a long-waited feature! But more polishing and work should be done to cover all the nececities.

I've corrected the codestyle a bit, linked the related issue/PRs to it and described the checklist of what is awaited to be done for this.

Also, it would be better to mark this PR as draft until it's not ready for review, so I don't review it accidentally!

@chirino
Copy link
Author

chirino commented Aug 15, 2021

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?

@tyranron
Copy link
Member

@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 tyranron marked this pull request as draft August 15, 2021 17:53
@chirino
Copy link
Author

chirino commented Aug 16, 2021

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?

@tyranron
Copy link
Member

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

@chirino
Copy link
Author

chirino commented Aug 16, 2021

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

@tyranron
Copy link
Member

tyranron commented Aug 17, 2021

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

@LegNeato
Copy link
Member

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.

…rde_json support.

Also renamed json.rs to serde_json.rs.
@chirino
Copy link
Author

chirino commented Aug 22, 2021

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

@tyranron
Copy link
Member

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

@LegNeato
Copy link
Member

This is great work, thank you! ❤️

@tyranron
Copy link
Member

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.

@chirino
Copy link
Author

chirino commented Sep 20, 2021

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

@tyranron
Copy link
Member

tyranron commented Sep 20, 2021

@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
Copy link
Member

tyranron commented Oct 8, 2021

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.

@chirino
Copy link
Author

chirino commented Oct 8, 2021

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

@chirino
Copy link
Author

chirino commented Feb 22, 2022

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

@tyranron tyranron added this to the 0.16.0 milestone Apr 13, 2022
@tyranron tyranron mentioned this pull request May 31, 2022
26 tasks
@juancampa
Copy link

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

@tyranron
Copy link
Member

tyranron commented Aug 8, 2022

@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 tyranron modified the milestones: 0.16.0, 0.17.0 Oct 17, 2022
@chirino
Copy link
Author

chirino commented Apr 24, 2023

@tyranron should I close this PR out?

1 similar comment
@chirino
Copy link
Author

chirino commented Apr 2, 2024

@tyranron should I close this PR out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems
Projects
None yet
4 participants