Add a basic lookahead functionality

[weiznich]

I've tried a bit around how to implement a look ahead feature as described in #124.
This should be seen as WIP. I've only opened this PR to start some discussion about this design.

[fungl164]

Hi,

Was looking at your implementation and had a few questions:

1. Would it make more sense to define LookAheadSelection.arguments and LookAheadSelection.childs as Option<Vec<?>> in order to avoid unnecessary creation of objects and arrays?
2. While on the topic, would it make more sense to change childs to children as the more appropriate term?
3. Could you include a simple example on how you would use this along with diesel as described on Query introspection in resolvers #124?

Many thanks for the contribution!

[weiznich]

Would it make more sense to define LookAheadSelection.arguments and LookAheadSelection.childs as Option<Vec<?>> in order to avoid unnecessary creation of objects and arrays?

As far as I understand the documentation of Vec::new this is not necessary because the constructor does not allocate anything. So I don't see any reason to use Option<Vec<T>> in place of Vec<T>

While on the topic, would it make more sense to change childs to children as the more appropriate term?

I'm really bad in naming things, so just suggest a better name and I will change it

Could you include a simple example on how you would use this along with diesel as described on #124?

I have same code using this api, that provides a somehow working solution for a specific database. It is currently quite hacky, so I need to do some cleanup before showing it somewhere.
Also before merging this I need to write some documentation. (I will start on this after we have figured out a api for this feature.)

[fungl164]

@weiznich all great points. thnxs! Looking forward to your updates... : )

[weiznich]

I have same code using this api, that provides a somehow working solution for a specific database. It is currently quite hacky, so I need to do some cleanup before showing it somewhere.

Just a little update about that: I've not forgotten about this PR, but I found that writing diesel code that interacts with the lookahead functionality involves quite a lot of boilerplate code. So I've decided to try to write some generic code that is able to be used in place of the boilerplate code, but this needs some time.
Hopefully I will have something to show in the next few weeks.

[fungl164]

This would be great! Thnxs!!

[weiznich]

So finally I got something to show. Please note that the look ahead functionality is not visible in the example code, because it is only used inside the library.

[weiznich]

@fungl164 @theduke
Are there any comments on the design here? Anything improvements that needs to be done?
Otherwise I will try to add some documentation to this so this could be merged in near future?

[fungl164]

@weiznich this is great! just downloaded your code and tried it. Seems to be working right out-of-the-box. Have not had much time to look into the details; however, I did notice the SQL queries pull all the fields on the first select, but not on the subsequent ones. Not sure if by design or oversight. I'll try to spend more time on it in the next week or so when I get a chance. Great stuff, btw!!

[fungl164]

@weiznich just wondering if it would be useful to use a proper pluralizer/inflector (e.g. https://github.com/whatisinternet/Inflector) for models when running them through the mutation macros for example.

[weiznich]

@fungl164 Thanks your suggestions.

just wondering if it would be useful to use a proper pluralizer/inflector (e.g. https://github.com/whatisinternet/Inflector) for models when running them through the mutation macros for example.

I'm not sure if that's possible because I need the names currently as string literals. A maybe better solution would be to provide a way to pass custom names in there instead.

I did notice the SQL queries pull all the fields on the first select, but not on the subsequent ones.

What do you exactly mean here?
On each request for a table all columns are loaded. (For references to other tables only the foreign keys. As soon as you request a nested table the foreign key will be resolved to the nested object.). Later juniper will forward only that ones that are requested.

I will try to do a rebase in the next days so we could work on actively merging this.

[fungl164]

What do you exactly mean here?

If you look at the GraphQL query (below), there is no request for 'hair_color'. However, when looking at the logs (also below), the first SELECT statement includes it as SQL field (even though it was not requested in GraphQL).

Interestingly enough, the subsequent SELECT statements in the logs include only the exact fields as requested on the GraphQL query. So, I was just wondering why the first SELECT did not behave the same way (e.g. exact field extraction/selection in SQL as those submitted via the GraphQL query). Hope this makes sense...

In any case, it all works fine in the end so thanks for the wonderful work you've put in. I just didn't know if this was the intended behavior, an optimization or a just a plain oversight in the implementation...

[weiznich]

If you look at the GraphQL query (below), there is no request for 'hair_color'. However, when looking at the logs (also below), the first SELECT statement includes it as SQL field (even though it was not requested in GraphQL).

This is totally expected. The current strategy is to load all fields of the table and then let juniper do the selection stuff. Only fields that represents other tables are handled different, because I think loading them would be become quite expensive. (As soon as I have landed all those PR's (this one and 2 or 3 to diesel) required to make wundergraph working I need to write some blog post about this 😉)

@theduke @mhallin
I've added some documentation to the newly added code and fixed the failing test cases. I would be happy to get a hopefully final review on this.

[codecov-io]

Codecov Report

Merging #136 into master will decrease coverage by 0.18%.
The diff coverage is 83.43%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   90.59%   90.41%   -0.19%     
==========================================
  Files          95       96       +1     
  Lines       18083    18616     +533     
==========================================
+ Hits        16383    16832     +449     
- Misses       1700     1784      +84

Impacted Files	Coverage Δ
juniper/src/lib.rs	86.84% <ø> (+2.22%)	⬆️
juniper/src/value.rs	83.53% <100%> (ø)	⬆️
juniper/src/executor/look_ahead.rs	83.1% <83.1%> (ø)
juniper/src/executor/mod.rs	93.67% <94.44%> (+0.92%)	⬆️
juniper/src/parser/tests/document.rs	93.69% <0%> (-0.22%)	⬇️
juniper/src/macros/tests/object.rs	85.9% <0%> (-0.19%)	⬇️
juniper/src/tests/introspection_tests.rs	97.18% <0%> (-0.1%)	⬇️
juniper/src/executor_tests/variables.rs	97.09% <0%> (-0.09%)	⬇️
juniper/src/parser/tests/value.rs	90.78% <0%> (-0.07%)	⬇️
...r/src/executor_tests/introspection/input_object.rs	94.51% <0%> (-0.07%)	⬇️
... and 27 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 9313c7c...19fe7ae. Read the comment docs.