Query variables validation #227

cecton · 2021-12-01T14:30:40Z

Changes

~~Remove unneeded Option around Request's variables`: this has been moved to Remove unneeded option #257 (review this first to reduce the diff size here 🧠)~~
Add error variant ValidationInvalidTypeVariable for variables of invalid type
Introduce new module spec for our higher level representation of GraphQL's Schema and Query: this has been moved to Cleanup prelude and create spec module #258 (review this first to reduce the diff size here 🧠)
~~Clean-up prelude and added a note: this has been moved to Cleanup prelude and create spec module #258 (review this first to reduce the diff size here 🧠)~~
Removed schema parameter from Query's constructor: if the schema changes we don't want the query to be executed on a wrong
schema. So unless we include the Schema to the hashing (Hash) we should make this a parameter of the methods of Query.
Introduce public method validate_variables on Query that allows type validation of the request's variable (the whole point of the damn PR, see Variable types are not validated #62)
Add a lot of transformation from apollo-rs's ast to our own (private) higher level representation API: FieldType, ObjectType, Interface, ...
Refactor fragments creation to its own type so it can be re-used in the Schema and Query
Refactor formatting tests so we don't need to update everything when our API changes
Add a bunch of type validation tests
Made Schema's is_subtype() private: we don't need to expose that to the user (at least not for now...)
Parse and validate the query before making a query plan
Fix invalid integration tests for missing variables: we used to check for the variables to be present before running the query plan. The implementation was not correct as the variables are optional by default unless the type is a non-null type. I removed this entirely in favor of this new variable type validation.
Fix a tracing stuff according to ADR on telemetry.
Add documentation
Disable parse error printing if log level is OFF

Fixes #62

cecton · 2021-12-02T12:17:14Z

Waiting on #233 before continuing

If the schema change we don't want the query to be executed on a wrong schema. Unless we include the Schema to the hashing (Hash) we should make this a parameter.

...so we have less work when we change the API of this thing.

o0Ignition0o · 2021-12-13T19:50:48Z

I’ll see myself out 😂🤣

Geal · 2021-12-14T08:40:04Z

ok so I worried more about what is said in

Removed schema parameter from Query's constructor: if the schema changes we don't want the query to be executed on a wrong schema. So unless we include the Schema to the hashing (Hash) we should make this a parameter of the methods of Query.

than about what the code is actually doing, which does not change much. A big assumption in the way we execute query is that the schema will not change during the query: if we establish a query plan from a query and schema, when formatting at the end it should still be the same schema. This is the benefit that was given by carrying the schema along with the query, it guarantees that the same schema will be used for the entire life of the query, even if the calling code in apollo-router changes.

Now looking more at the code, this is mainly affecting the query cache, that caches the query parsed with apollo-rs and does not require the schema, than the CachingQueryPlanner who needs both query and schema. So it's ok. Maybe I just worry too much 😅

o0Ignition0o · 2021-12-14T08:48:11Z

this is mainly affecting the query cache, that caches the query parsed with apollo-rs and does not require the schema, than the CachingQueryPlanner who needs both query and schema.

This is the key point imo

apollo-router-core/src/spec/query.rs

Geal · 2021-12-14T09:06:25Z

Perf test please

cecton · 2021-12-14T09:58:47Z

Perf test please

We now have automatic perf tests! ^_^ https://github.com/apollographql/router/pull/227/checks?check_run_id=4517674898

abernix · 2021-12-14T10:05:34Z

We now have automatic perf tests! ^_^ https://github.com/apollographql/router/pull/227/checks?check_run_id=4517674898

@cecton From what I understand, they only get triggered and show up there if you explicitly request them using the phrasing above (or some variations, like Je voudrais un test de perf 😉 ). So they are semi-automated.

cecton · 2021-12-14T10:05:41Z

This is the key point imo

To me the key point is more like: there is a way to use the API where you can instantiate the query with a one schema and do validation/formatting with another 😅 I don't need to look at the cache to know that it is wrong. Pretty sure it was done by me and completely unintentionally.

cecton · 2021-12-14T10:07:26Z

@cecton From what I understand, they only get triggered and show up there if you explicitly request them using the phrasing above (or some variations, like Je voudrais un test de perf wink ). So they are semi-automated.

Oh nice! There was one available before though:

cecton · 2021-12-14T10:50:11Z

Oh my branch makes huge perf improvement 😂 that's probably not right lol

cecton · 2021-12-14T10:52:48Z

Perf test please

Geal · 2021-12-14T12:09:32Z

Oh my branch makes huge perf improvement 😂 that's probably not right lol

It looks like it compares to one of my failed attempts in the mutex PR 🤭

cecton · 2021-12-15T10:49:23Z

OH NOW WE HAVE SCORES!!

Mine: 7287
main: 7195

Wait... this is still wrong XD this PR should be slower....

BrynCooke

Minor issues around validation logic, but otherwise looks good.

BrynCooke · 2021-12-15T10:59:51Z

apollo-router-core/src/spec/field_type.rs

+    ) -> Result<(), InvalidValue> {
+        match (self, value) {
+            (FieldType::String, Value::String(_)) => Ok(()),
+            (FieldType::Int, Value::Number(number)) if number.is_i64() || number.is_u64() => Ok(()),


Integers need to be a signed 32 bit integer https://spec.graphql.org/June2018/#sec-Int

oh god really

Fixed in 91d51fb

BrynCooke · 2021-12-15T11:03:15Z

apollo-router-core/src/spec/field_type.rs

+}
+
+impl FieldType {
+    pub(crate) fn validate_value(


Float, Integer and Bool can be represented as strings, so coercion supported need to be added. Can be a followup ticket though.

what, where did you see that? oh that would explain what i saw with the ID type

Fixed in 41e2279

garypen

Let me know what you think about the question I have on FieldTypes. Apart from that, looks good!

apollo-router-core/src/query_cache.rs

garypen · 2021-12-15T10:58:56Z

apollo-router-core/src/spec/field_type.rs

+            // should always serialize as a String."
+            //
+            // In practice it seems Int works too
+            (FieldType::Id, Value::String(_) | Value::Number(_)) => Ok(()),


Does this indicate a problem somewhere. In other words, if it should serialize as a String, why are we getting Numbers?

No I think it's the type coercion mentioned by Bryn there: #227 (comment)

garypen · 2021-12-15T11:29:15Z

OH NOW WE HAVE SCORES!!

Mine: 7287 main: 7195

Wait... this is still wrong XD this PR should be slower....

I think there is "variation" in the benchmarking system due to "noisy neighbours" in the cloud and other source of variance.

cecton · 2021-12-15T11:35:42Z

I think there is "variation" in the benchmarking system due to "noisy neighbours" in the cloud and other source of variance.

Ah I restarted it and now I have 6912... which seems to low

Geal · 2021-12-15T12:50:18Z

Ah I restarted it and now I have 6912... which seems to low

no need to worry too much for a 5% difference. Panic when it's larger than 20% 😄

Convert ObjectTypeDefinition to ObjectType

0d57366

cecton added the size/medium label Dec 1, 2021

cecton self-assigned this Dec 1, 2021

cecton added 5 commits December 1, 2021 15:31

Turns out this is case sensitive

b23c7dd

Add Boolean to the party

dad1d52

Move ObjectType to macro and add Interface which is identical

4d754ab

Convert interface definition and interface extension

f897c3c

Add root operation types

d5f8d25

cecton force-pushed the cecton-query-variable-validation branch from ec094f5 to d5f8d25 Compare December 2, 2021 16:11

cecton added 8 commits December 3, 2021 09:39

Merge commit 31eb0d8 (no conflict)

941aa3b

Remove useless Option on Request's field variables

376a4a8

WIP

383914e

Validate variables recursively

127cf93

Also check for interfaces

cc8849e

Add custom scalars validation

eb0f01f

Fix tests

14e49f0

Fix bug introduced here after removing Option for deserialization

6a6602e

cecton force-pushed the cecton-query-variable-validation branch from e6186ff to 6a6602e Compare December 4, 2021 09:39

cecton added 11 commits December 8, 2021 17:24

Moved parsing of types to schema & add validation tests

df8d681

Update apollo-rs & add some tests on arrays

3f72d46

Remove Schema from Query, this is wrong

45f66e1

If the schema change we don't want the query to be executed on a wrong schema. Unless we include the Schema to the hashing (Hash) we should make this a parameter.

Forgot to fix the tests

6cbf775

Refactor format response test

a164a60

...so we have less work when we change the API of this thing.

Slight optimization & parse query and fail earlier

a69c1df

Same slight optimization for Schema

96416aa

Split into modules

8abdc9c

Merge commit c759f46 (conflicts)

f2a2ce2

Remove unneeded imports

3ffd002

Merge commit ba6540b (no conflict)

9b31b5e

Geal reviewed Dec 14, 2021

View reviewed changes

apollo-router-core/src/spec/query.rs Show resolved Hide resolved

Geal approved these changes Dec 14, 2021

View reviewed changes

Merge commit 13045c5 (no conflict)

61482c3

cecton added 3 commits December 15, 2021 10:49

Replace bool return type in validate_* methods with Result

10e3e17

clippy (TIL .try_for_each())

ad1a12d

Fix for Int type checking

e42d107

BrynCooke approved these changes Dec 15, 2021

View reviewed changes

garypen reviewed Dec 15, 2021

View reviewed changes

cecton added 2 commits December 15, 2021 12:32

Limit Int to 32-bit signed integer

91d51fb

No need to move

693c65d

Type coercion for Int, Boolean and Float

41e2279

cecton merged commit 2e627f3 into main Dec 15, 2021

cecton deleted the cecton-query-variable-validation branch December 15, 2021 14:00

cecton mentioned this pull request Jan 5, 2022

add the missing skip_all on instrument attributes #287

Merged

abernix removed the size/medium label Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query variables validation #227

Query variables validation #227

cecton commented Dec 1, 2021 •

edited

Loading

cecton commented Dec 2, 2021

o0Ignition0o commented Dec 13, 2021

Geal commented Dec 14, 2021 •

edited

Loading

o0Ignition0o commented Dec 14, 2021

Geal commented Dec 14, 2021

cecton commented Dec 14, 2021

abernix commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

Geal commented Dec 14, 2021

cecton commented Dec 15, 2021

BrynCooke left a comment

BrynCooke Dec 15, 2021

cecton Dec 15, 2021

cecton Dec 15, 2021

BrynCooke Dec 15, 2021

cecton Dec 15, 2021

cecton Dec 15, 2021

garypen left a comment

garypen Dec 15, 2021

cecton Dec 15, 2021

garypen commented Dec 15, 2021

cecton commented Dec 15, 2021

Geal commented Dec 15, 2021

Query variables validation #227

Query variables validation #227

Conversation

cecton commented Dec 1, 2021 • edited Loading

Changes

cecton commented Dec 2, 2021

o0Ignition0o commented Dec 13, 2021

Geal commented Dec 14, 2021 • edited Loading

o0Ignition0o commented Dec 14, 2021

Geal commented Dec 14, 2021

cecton commented Dec 14, 2021

abernix commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

cecton commented Dec 14, 2021

Geal commented Dec 14, 2021

cecton commented Dec 15, 2021

BrynCooke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garypen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garypen commented Dec 15, 2021

cecton commented Dec 15, 2021

Geal commented Dec 15, 2021

cecton commented Dec 1, 2021 •

edited

Loading

Geal commented Dec 14, 2021 •

edited

Loading