feature: Add variable validation rule to query save

[markkelnar]

When processing a persisted query document string for saving, run the validation rules on the string. Add a new rule to error for arguments that are present in operations but instead should be a variable instead. Return the graphql Error or display the error in the editor.

Github issue

[markkelnar]

Example query string validation against the graphql rules and error displayed in Admin Editor. In this example, a draft query was being saved as draft with 'typename' should have been '__typename'.

[markkelnar]

This adds the other rules to the query string validation.

[markkelnar]

Doing the rule validation, brought to attention some invalid test query strings. :)

[markkelnar]

Oh, my notes are in here. Ha!

[jasonbahl]

We'd want to error on all 3 of these examples.

The problem we're trying to prevent, is folks doing string concatenation for their queries and instead push folks toward using variables.

All 3 of those queries could be concatenated like so:

---

example: query FooQuery { post( id: "1", idType: DATABASE_ID) { title } }

const id = 123;
const query = `query FooQuery { post( id: ${id}, idType: DATABASE_ID) { title } }`

---

example: query ($num: ID!) { post( id: "1", idType: DATABASE_ID) { title } } (perhaps this was a mistake, but this looks like it might be invalid already as an argument is defined but not used)

const id = 123;
const idType: "DATABASE_ID";
const query = `query ($num: ID!) { post( id: ${id}, idType: ${idType}) { title } }`

---

example: query ($num: ID!) { post( id: $num, idType: DATABASE_ID) { title } }

const idType = "DATABASE_ID";
const query = `query ($num: ID!) { post( id: $num, idType: ${idType}) { title } }`

---

In each of these cases we have arguments passed in to fields that do not map to an argument defined for the query.

The expected "valid" definition of the query would be:

query ($id: ID! $idType: PostIdType ) {
  post( id: $id idType: $idType ) {
    title
  }
}

Here, the field has 2 arguments, and both are defined in the operation.

If either of the arguments are not defined on the operation and used at the field level as an argument, then we might still be letting concatenated query strings persist many permutations of the same query, which is the situation we're trying to prevent.

---

[jasonbahl]

I don't believe this validates fully. See my comments re: how this still allows the problem we're trying to prevent.

[jasonbahl]

We'd want to error on all 3 of these examples.

The problem we're trying to prevent, is folks doing string concatenation for their queries and instead push folks toward using variables.

All 3 of those queries could be concatenated like so:

---

example: query FooQuery { post( id: "1", idType: DATABASE_ID) { title } }

const id = 123;
const query = `query FooQuery { post( id: ${id}, idType: DATABASE_ID) { title } }`

---

example: query ($num: ID!) { post( id: "1", idType: DATABASE_ID) { title } } (perhaps this was a mistake, but this looks like it might be invalid already as an argument is defined but not used)

const id = 123;
const idType: "DATABASE_ID";
const query = `query ($num: ID!) { post( id: ${id}, idType: ${idType}) { title } }`

---

example: query ($num: ID!) { post( id: $num, idType: DATABASE_ID) { title } }

const idType = "DATABASE_ID";
const query = `query ($num: ID!) { post( id: $num, idType: ${idType}) { title } }`

---

In each of these cases we have arguments passed in to fields that do not map to an argument defined for the query.

The expected "valid" definition of the query would be:

query ($id: ID! $idType: PostIdType ) {
  post( id: $id idType: $idType ) {
    title
  }
}

Here, the field has 2 arguments, and both are defined in the operation.

If either of the arguments are not defined on the operation and used at the field level as an argument, then we might still be letting concatenated query strings persist many permutations of the same query, which is the situation we're trying to prevent.

---

[jasonbahl]

This query should fail validation because the idType could be concatenated still, leading to duplicate query documents being stored for different possible values.

i.e.

const idTye = 'DATABASE_ID';
const query = `query ($num: ID!) { post( id: $num, idType: ${idTye}) { title } }`

or:

const idTye = 'SLUG';
const query = `query ($num: ID!) { post( id: $num, idType: ${idTye}) { title } }`

By using string interpolation / concatenation, we'd be able to have multiple versions of the same query, which is the situation we're trying to prevent.

Ideally we'd be guiding the user to:

const query = `query ($num: ID! $idType: PostIdType) { post( id: $num, idType: $idType }) { title } }`

Where here, each argument on the field level corresponds with a variable defined at the operation level.

[jasonbahl]

🤔 I checked out this branch:

And was able to sucessfully save this query:

[jasonbahl]

I tried another query, and was able to publish it without error.

---

https://github.com/wp-graphql/wp-graphql-smart-cache/pull/249/files#diff-cb45ae6ce2d1924aad6d649880ebd4bcf3ce4a8dd481b8f8be1d14e84ea60af6R34

I'm not super familiar with what's available in the ArgumentNode, but I'm curious if we can do any other type of check other than the in_array() check here 🤔

I wonder if there's prior art in the JavaScript GraphQL Ecosystem for this type of validation rule too? Maybe we can borrow from them, if there is.

[markkelnar]

If this is merged as is, apps with existing APQ style persisted queries that break this validation rule will fail to save. Thus a change in behavior and an impact to caching performance.

Consider making this validation on saving queries a) optional, opt-in, with a checkbox. b) only enforced in WP admin editor while saving a query.

[jasonbahl]

closing for now. We can reference this PR when work begins again on #224.