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

Add integration crate for the warp framework #216

Merged
merged 32 commits into from Sep 7, 2018

Conversation

Projects
None yet
4 participants
@tomhoule
Member

tomhoule commented Aug 3, 2018

I think this still needs more testing but coming up with this integration was very smooth and it feels like idiomatic warp (I've only been programming with warp since its first public release, so yesterday :)).

I'll keep working on polishing this over the weekend (docs, tests) but feedback would still be appreciated.

This notably does not have support for batch requests. Should it be implemented in this PR or can it be done separately?

@tomhoule tomhoule changed the title from Add integration crate for the warp framework to WIP - Add integration crate for the warp framework Aug 3, 2018

@tomhoule tomhoule referenced this pull request Aug 3, 2018

Closed

Build a warp integration? #215

) -> BoxedFilter<(impl warp::Reply,)>
where
Context: Send + Sync + 'static,
// CtxRef: AsRef<Context> + Send + 'static,

This comment has been minimized.

@tomhoule

tomhoule Aug 3, 2018

Member

this should be deleted

Context: Send + Sync + 'static,
// CtxRef: AsRef<Context> + Send + 'static,
Query: juniper::GraphQLType<Context = Context, TypeInfo = ()> + Send + Sync + 'static,
Mutation: juniper::GraphQLType<Context = Context, TypeInfo = ()> + Send + Sync + 'static,

This comment has been minimized.

@tomhoule

tomhoule Aug 3, 2018

Member

I played with the design quite a bit, maybe not all of these bounds are needed anymore

@LegNeato

This comment has been minimized.

Member

LegNeato commented Aug 6, 2018

This is looking great! I'm just coming up to speed, but if you are not aware we have an integration test harness you likely want to leverage. Here's Rocket using it: https://github.com/graphql-rust/juniper/blob/master/juniper_rocket/src/lib.rs#L453

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 6, 2018

I wasn't aware of that, thanks for the pointer!

.and(juniper_warp::graphiql_handler("/graphql"))
.or(homepage),
).or(warp::path("graphql").and(graphql_filter))
.with(log),

This comment has been minimized.

@tomhoule

tomhoule Aug 7, 2018

Member

with a few more days of warp experience, this doesn't look like the right way to do it, I'll have to improve this. Should this be a single file example by the way? I haven't worked with those but this one is meant to be run and used in a browser

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 12, 2018

Just a heads up: I am in vacations for the next 10 days so I will probably not work on this, but I intend to finish it after.

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 19, 2018

Small update on this: I spent some time setting up the framework integration harness, and GET query parameters handling is a bit weird when testing a filter directly, I may have to change the tests to spawn a whole server to test against instead.

edit: this is solved, now other tests are failing

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 19, 2018

Ok now all tests are passing locally (I haven't seen on CI yet). There is the small matter of error handling - I introduced failure as a dependency for juniper_warp since that's what I was able to implement a fast solution with, but we might want to have a custom error enum.

@tomhoule tomhoule changed the title from WIP - Add integration crate for the warp framework to Add integration crate for the warp framework Aug 20, 2018

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 20, 2018

@LegNeato I think this is ready for review.

There is an issue with rust 1.21 and 1.22 in CI - some warp dependencies don't seem to compile with these versions. If latest stable is the oldest supported compiler for warp, we should probably have the same requirement for juniper-warp. I haven't looked into how that could be implemented yet. What do you think?

I'll squash before merging.

/// let graphql_filter = make_graphql_filter(schema, context_extractor);
///
/// let graphql_endpoint = warp::path("graphql")
/// .and(warp::post(graphql_filter));

This comment has been minimized.

@tomhoule

tomhoule Aug 20, 2018

Member

this style of method filter is going to be deprecated, I'll change this next time I touch the code

This comment has been minimized.

@LegNeato
@LegNeato

Looks great! A couple of small things. I am not really qualified to review to warp portions so I will defer to you on those!

Cargo.toml Outdated
@@ -5,4 +5,6 @@ members = [
"juniper_tests",
"juniper_iron",
"juniper_rocket",
"juniper_warp",
"juniper_warp/examples/warp_server",

This comment has been minimized.

@LegNeato

LegNeato Aug 21, 2018

Member

Is this necessary? I don't believe we needed it for the other integrations.

/// let graphql_filter = make_graphql_filter(schema, context_extractor);
///
/// let graphql_endpoint = warp::path("graphql")
/// .and(warp::post(graphql_filter));

This comment has been minimized.

@LegNeato
// if url.len() > 200 {
// panic!("{:?}", url);
// }

This comment has been minimized.

@LegNeato

LegNeato Aug 21, 2018

Member

Delete.

impl HTTPIntegration for TestWarpIntegration {
fn get(&self, url: &str) -> TestResponse {
use percent_encoding::{percent_encode, DEFAULT_ENCODE_SET};
let url: String = percent_encode(url.replace("/?", "").as_bytes(), DEFAULT_ENCODE_SET)

This comment has been minimized.

@LegNeato

LegNeato Aug 21, 2018

Member

This is fine for now, but I think we'll have the harness send valid encoded URLs so all these integrations don't have to deal with it themselves.

This comment has been minimized.

@tomhoule

tomhoule Aug 21, 2018

Member

I'll check if it's possible to remove them, I remember I had to do the url encoding to make it work, but it could be a bug in warp (the framework is very young) - I'll try to open an issue on their side if that's the case.

This comment has been minimized.

@LegNeato

LegNeato Aug 21, 2018

Member

Oh, I wasn't clear...I was just musing out loud! I don't think there is a bug in warp or you changes here, but it is actually an issue with the integration test harness. See #230 (comment) for the same thing with Hyper, and I had to fix it hackily in the iron integration a bit ago in #208.

This comment has been minimized.

@tomhoule

tomhoule Aug 22, 2018

Member

Oh alright, sorry for the misunderstanding :)

@LegNeato

This comment has been minimized.

Member

LegNeato commented Aug 21, 2018

No need to squash before merging, I can do that.

As for rust version, I'm not sure. We could not include juniper_warp in the workspace and test it separately on CI using only the most recent stable rust versions. We could probably support that at the CI config level, but we are also using cargo make so maybe that is a better spot for the split. It feels weird to bump juniper's requirement for the warp integration, so let's not do that.

I'm happy to poke at the build stuff if you don't want to...just let me know!

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 21, 2018

Agreed, the higher minimum version should only apply to juniper_warp. I'll try to make the CI setup adjustments and come back here if I run into trouble.

@sagiegurari

This comment has been minimized.

Contributor

sagiegurari commented Aug 22, 2018

you can take a look at the juniper rocket makefile.toml as an example

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 25, 2018

I'll try to get this done some time this weekend.

Just a note so we don't forget: we need some kind of thread pool, or document that this needs to be done by the user, since warp is async but juniper is not (yet).

@LegNeato

This comment has been minimized.

Member

LegNeato commented Aug 25, 2018

@tomhoule Awesome, no rush! The Hyper integration (#230) includes a way to pass in afutures_cpupool::CpuPool so I would imagine we want to do the same. Not sure if there is a generic "pool" facade to be had or if one would even be necessary.

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 25, 2018

tokio currently has its own threadpool implementation that they claim is much faster than futures-cpupool. But mainly it is going to merge with the thread pool that will be built-in in the next major version of futures. This is probably what we will want to use eventually, but it's probably easier to use an external crate that does not depend on a particular version of futures (other than 0.1.*) or tokio at first (so futures-cpupool).

What do you think of making the thread pool implicit in make_graphql_filter (the filter makes one by default), and allow passing one in for users who want more control? The advantage of the implicit default thread pool is that the juniper filter appears as purely async from that angle (and less boilerplate).

edit: to make it more explicit, there would be two functions: make_graphql_filter and make_graphql_filter_with_thread_pool.

@codecov-io

This comment has been minimized.

codecov-io commented Aug 26, 2018

Codecov Report

Merging #216 into master will increase coverage by 0.58%.
The diff coverage is 74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   86.98%   87.57%   +0.58%     
==========================================
  Files          96       98       +2     
  Lines       14196    16680    +2484     
==========================================
+ Hits        12349    14607    +2258     
- Misses       1847     2073     +226
Impacted Files Coverage Δ
juniper_warp/examples/warp_server/src/main.rs 3.03% <3.03%> (ø)
juniper_warp/src/lib.rs 94.01% <94.01%> (ø)
juniper/src/macros/tests/object.rs 85.71% <0%> (-4.1%) ⬇️
juniper/src/macros/tests/scalar.rs 73.21% <0%> (-2.69%) ⬇️
juniper_tests/src/codegen/derive_object.rs 85.71% <0%> (-2.22%) ⬇️
juniper/src/macros/tests/union.rs 80.29% <0%> (-2%) ⬇️
juniper/src/macros/tests/interface.rs 80.81% <0%> (-1.98%) ⬇️
juniper/src/macros/tests/field.rs 94.66% <0%> (-1.86%) ⬇️
juniper/src/types/base.rs 88.32% <0%> (-0.39%) ⬇️
juniper/src/types/pointers.rs 90.9% <0%> (-0.27%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45004f7...0a6b2ab. Read the comment docs.

@LegNeato

This comment has been minimized.

Member

LegNeato commented Aug 27, 2018

FYI, we ended up dropping support for rust 1.21 to move our dep on regex to 1.0 in master...so we are fine requiring 1.22 here as well. If you rebase it should pass CI. Sorry for the churn and making you muck around with the build stuff!

@tomhoule

This comment has been minimized.

Member

tomhoule commented Aug 28, 2018

I'll push my latest changes (small improvements and some cargo make things that may not be necessary anymore) and see if I can make the Makefile.toml simpler, or even remove it.

When depending on juniper through a path instead of the latest version (0.9.2) I get a test failure with batched queries (which aren't implemented), so that still needs to be done but I think it's the last missing feature.

@tomhoule

This comment has been minimized.

Member

tomhoule commented Sep 1, 2018

So apparently even 1.23 is too old for warp. I tried fiddling with cargo make in various ways (mostly custom scripts to skip juniper_warp depending on the rust version - the scripts I put under the condition_script key in the Makefile.toml don't seem to run), and haven't gotten it to work.

There is a cargo-make issue (sagiegurari/cargo-make#110) about this. For now the only way I could make it work would be separating the test runs for juniper_warp from the rest of the crates, and invoking them directly from the .travis.yml. Do you have ideas on how to solve this?

@tomhoule

This comment has been minimized.

Member

tomhoule commented Sep 1, 2018

Oh and I should probably mention that I started using this code on a small project and it seems to be working fine so far.

@LegNeato

This comment has been minimized.

Member

LegNeato commented Sep 2, 2018

Let's just put multiple runs in .travis.yml, thanks!

@tomhoule

This comment has been minimized.

Member

tomhoule commented Sep 2, 2018

CI is passing now. I noticed #239 , should we wait for that to be merged in case the new tests fail for juniper_warp?

@LegNeato

This comment has been minimized.

Member

LegNeato commented Sep 2, 2018

I just merged that, so rebase and then we will get this merged!

@sagiegurari

This comment has been minimized.

Contributor

sagiegurari commented Sep 5, 2018

New cargo-make version 0.14.0 now has built in support for rust min/max/equal version condition criteria. so that might resolve your previous issues.

tomhoule added some commits Aug 19, 2018

Implement passing/creating a thread pool to avoid blocking
[to be squashed] there is one failing test that still needs to be investigated
Make the http response body bytes everywhere
Otherwise it is impossible to write `graphql_filter.or(graphiql_filter).unify()`.
@tomhoule

This comment has been minimized.

Member

tomhoule commented Sep 7, 2018

Just found time to work on this a bit - the new http integration tests are passing now :)

@LegNeato

Awesome!!! I think we are ready pending deleting the extra file.

@@ -0,0 +1,9 @@
# Exits successfully if the Rust version that is running is compatible with juniper_warp.

This comment has been minimized.

@LegNeato

LegNeato Sep 7, 2018

Member

We don't need this file anymore, right?

This comment has been minimized.

@tomhoule

tomhoule Sep 7, 2018

Member

Oh definitely, my bad. I'll also try to see if we can revert to using cargo make with the new feature.

This comment has been minimized.

@LegNeato

LegNeato Sep 7, 2018

Member

We can do that in a follow-up, just delete this file for now.

This comment has been minimized.

@tomhoule

@LegNeato LegNeato merged commit fd636e0 into graphql-rust:master Sep 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@LegNeato

This comment has been minimized.

Member

LegNeato commented Sep 7, 2018

🎉🎉🎉🍻 🎉 🍻

I am publishing a new juniper + all the integration crates next week...I'll publish juniper_warp and add you as an owner at that time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment