Implement support for dynamic schemas

[srijs]

Hi!

Apologies in advance for the somewhat large patch.

This is my first attempt at a feature contribution. The feature I'm trying to implement scratches an itch of my own, and has been in-use in a project of mine for quite some time now and I thought it might be nice to try and contribute it back up-stream. So I took the time today to rebase it and clean it up.

To frame the problem, I'm working on a database that provides a GraphQL-based API. The objects stored in the database are defined using a schema which the user can configure. The schema will be loaded from a configuration file at start-up.

Previously, it was only possible to define the GraphQL schema at compile time, due to the fact that the name and type methods in the GraphQLType trait do not take any objects as arguments which could carry any sort of schema information, i.e. they are effectively "static" functions.

This patch changes this so that a GraphQLType has an associated type TypeInfo, which can hold schema information that can be loaded at runtime and passed when constructing a RootNode.

I have not added support for custom TypeInfo when using the macros yet, so in order to leverage TypeInfo the traits need to be implemented manually. In practice this has proven not to be an issue at all, and I'm not even sure if it's a good idea to make the macros more powerful in that sense.

Let me know what you think!

Also, no worries at all if you feel this does not fit within your vision for the juniper project, it would just mean that my fork would diverge more and more, which would be sad but also acceptable for me.

But of course, my preference would be to upstream this, so if you are generally interested in the feature, but have a completely different implementation in mind, I'm happy to explore it as well!

[srijs]

Looks like the Appveyor tests have problems with SSL currently:

could not download file from 'https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256' to 'C:\Users\appveyor\.rustup\tmp\du00kqkra4sea6w1_file'
info: caused by: error during download
info: caused by: [35] SSL connect error (schannel: next InitializeSecurityContext failed: Unknown error (0x80092013) - The revocation function was unable to check revocation because the revocation server was offline.)

Perhaps if you find the time you could re-run them.

[theduke]

@srijs I like having an option for dynamic schemas, but I'd like to hear @mhallin s thoughts and should also do a thorough review.

We also don't want to impact performance much for the static case.

[srijs]

Thanks for your feedback, @theduke. A review on this from either you or @mhallin would be great!

As for performance, I have just pushed an update to the PR that removes the single .clone() I added that would have been called per-request (in the validation logic). So in terms of allocations/operations on data types there should be virtually no change in performance here.

Also, since the TypeInfo for non-dynamic cases (such as scalars) is () (i.e. a Zero-Sized Type), I would expect the compiler to optimise calls to the resolve* methods accordingly. Compare the body of the bar function both with a pointer to unit, and without.

But of course if you have specific concerns about negative impacts on performance, I'd be happy to run a few benchmarks and see where that takes us.

[mhallin]

Overall, I like the direction and the feature it's adding. However, I want to raise some concerns:

• The "internal" API on the Registry and related types is becoming a becoming a bit polluted and asymmetric. Maybe we could remove all the specialized forms for where TypeInfo=() and just force all methods to take a TypeInfo type too? That would remove e.g. Registry::field_with_info() and just add a parameter to Registry::field().
• The cases where TypeInfo is not () aren't covered by any tests. Maybe just an example binary with a dynamic schema would suffice, so we know if/when we're introducing regressions for this feature. Right now, I'm a bit afraid that future changes to Juniper might break this feature without us even knowing.

That being said, thanks for this PR! Let us know if you want to make these changes, or if it something we could do instead.

[srijs]

These concerns make a lot of sense, @mhallin, thanks for raising them!

Regarding the pollution of the Registry API, I actually had it the way you proposed back in my fork, but then changed it to reduce the amount of API breakage this PR would cause if merged. If you as the maintainer are okay with it though, I'm happy to change it again. Just let me know!

I'll go and add a couple of tests in the meantime...

[srijs]

I've implemented your suggestions, @mhallin, would appreciate a second review!

[srijs]

@mhallin, @theduke I'm going to resolve the conflicts, but it would be great to get a go/no-go decision/review on this soon(ish).

[mhallin]

I didn't realize that you had added a test here, so I was waiting for that :) I think this looks good and ready for merging.

[srijs]

Thanks for the hint, @theduke, that worked well! @mhallin, should be ready to merge as soon as the tests are green!

[srijs]

🎉

[theduke]

We haven't configured appveyor properly yet, but this shouldn't make any difference here, so I'm merging.

Thanks for the big contirbution, @srijs .