Handle more advanced input queries #24

lopert · 2025-10-07T18:48:58Z

Fixes https://github.com/shop/issues-shopifyvm/issues/660

This PR reworks validation in order to support more graphql query features, such as aliasing, variables, fragments.

I also ended up reorganizing our test directory.

The fixture validation has been refactored into two functions

`validateFixtureInputStructure(queryAST, schema, fixtureData)`

Approach: Parallel Tree Traversal

Recursively walks through the query AST and fixture data simultaneously, comparing structure at each level:

Schema-aware traversal: Uses GraphQL schema to determine if types are abstract (unions/interfaces) or concrete, which dictates how fragments are processed
Fragment handling: Converts fragment spreads to inline fragments, then either matches fixture to one fragment (for abstract types) or merges all fragments (for concrete types)
Field-by-field comparison: At each object level, checks that fixture keys match query selections - detecting both extra and missing fields
Query generation: Builds a normalized GraphQL query string during traversal, resolving aliases and preserving structure

validateFixtureInputTypes(generatedQuery, schema, fixtureData, variables?)

Validates that the fixture's types match the schema by executing the query against fixture data:

Approach: GraphQL Execution Engine

Executes the normalized query against fixture data using GraphQL's built-in type validation.

Leverages GraphQL.js: Uses the standard GraphQL execution engine which handles all type coercion and validation rules
Custom resolvers: Injects field resolver that knows how to look up aliased fields in the original fixture data
Custom type resolver: Injects type resolver that infers concrete types for abstract types by matching fixture fields
Standard validation: All scalar validation, nullability checks, and type requirements are handled by GraphQL execution
Returns execution result: Provides the data and any type errors encountered during execution

adampetro

I wonder if there is a simpler overall approach that will eliminate a lot of this complexity. Can we not just traverse the input query AST alongside the JSON value and validate as we go?

src/methods/validate-fixture-input.ts

adampetro · 2025-10-08T15:34:31Z

src/methods/validate-input-query-fixture-match.ts

+  // Match alias pattern: word followed by colon and whitespace and another word
+  // But only when followed by whitespace, opening brace, or closing brace
+  // This avoids matching colons inside strings or arguments
+  return graphqlString.replace(/\b(\w+):\s*(\w+)(?=[\s{},])/g, '$2');


This feels like it has the potential to miss a number of edge cases. I would much prefer if we could manipulate the AST instead of doing regex operations.

Additionally, how do we expect to handle the case of the same field selected multiple times with aliases? For example:

{ discountNode { metafield1: metafield(key: "my-key1", namespace: "my-namespace") { jsonValue } metafield2: metafield(key: "my-key2", namespace: "my-namespace") { jsonValue } } }

src/utils/build-query-field-info.ts

adampetro · 2025-10-08T15:38:56Z

src/utils/build-query-field-info.ts

+      if (selection.kind === 'Field') {
+        const field = selection as FieldNode;
+        const actualFieldName = field.name.value;
+        const aliasName = field.alias?.value || actualFieldName;


Nit: it's not an alias name if we fallback to actualFieldName. I would call it responseKey or something like that.

src/utils/convert-fixture-to-query.ts

… union/interface type detection

saga-dasgupta · 2025-10-14T19:59:30Z

src/methods/validate-fixture-input-structure.ts

+  if (queryOperation.variableDefinitions && queryOperation.variableDefinitions.length > 0) {
+    const varDefs = queryOperation.variableDefinitions.map(varDef => {
+      const varName = varDef.variable.name.value;
+      const varType = print(varDef.type);


Why is this a print?

This is the print function from graphqljs. We're using it to convert from AST for to a consistent string format. Example:

// Variable definition in query AST: // $itemId: ID! varDef.variable.name.value // => "itemId" varDef.type // => NonNullType { type: NamedType { name: "ID" } } print(varDef.type) // => "ID!"

saga-dasgupta · 2025-10-14T20:00:15Z

src/methods/validate-fixture-input-structure.ts

+  if (fieldNode.arguments && fieldNode.arguments.length > 0) {
+    const args = fieldNode.arguments.map(arg => {
+      const argName = arg.name.value;
+      const argValue = print(arg.value);


Same. Here's another example with args:

// Field with arguments: // metafield(namespace: "$app:config", key: "setting1", limit: 10, enabled: true, id: $itemId) arg.name.value // => "namespace" arg.value // => StringValue { value: "$app:config" } print(arg.value) // => "\"$app:config\"" (with quotes!) arg.name.value // => "limit" arg.value // => IntValue { value: 10 } print(arg.value) // => "10" arg.name.value // => "enabled" arg.value // => BooleanValue { value: true } print(arg.value) // => "true" arg.name.value // => "id" arg.value // => Variable { name: { value: "itemId" } } print(arg.value) // => "$itemId" // Result: "metafield(namespace: \"$app:config\", key: "setting1", limit: 10, enabled: true, id: $itemId)"

src/methods/validate-fixture-input-types.ts

adampetro · 2025-10-16T14:31:53Z

src/methods/load-fixture.ts

- *   - expectedOutput: Object - The output data from payload.output  
+ *   - expectedOutput: Object - The output data from payload.output
 *   - target: string - The target string from payload.target
+ *   - inputQueryVariables: Object (optional) - Variable values from payload.inputQueryVariables


Why do we need variable values? As a user I would be very confused because the variable values wouldn't affect the execution at all given that I'm also providing the result of the input query?

adampetro · 2025-10-16T14:37:36Z

src/methods/run-function.ts

+      // Calculate paths correctly for when used as a dependency:
+      // __dirname = /path/to/function/tests/node_modules/function-testing-helpers/src/methods
+      // Go up 5 levels to get to function directory: ../../../../../ = /path/to/function


Is this always the case? Is there not a scenario with workspaces where the dependencies are downloaded higher up in the hierarchy?

adampetro · 2025-10-16T14:39:52Z

src/methods/run-function.ts

+    if (wasmPath) {
+      // Use user-provided WASM path
+      resolvedWasmPath = path.resolve(wasmPath);
+      if (!fs.existsSync(resolvedWasmPath)) {


Is it standard to use these sync functions in an async function?

adampetro · 2025-10-16T14:42:11Z

src/methods/run-function.ts

+      }
+    } else {
+      // Check Rust build output location
+      const rustWasmPath = path.join(functionDir, 'target', 'wasm32-wasip1', 'release', `${functionName}.wasm`);


This is another case where workspaces might complicate things. It's also not a guarantee that they are using wasm32-wasip1. We are starting to move away from this with the latest version of the shopify_function Rust crate to get off of wasi.

adampetro · 2025-10-16T14:43:09Z

src/methods/run-function.ts

Can this change be split into a different PR? It seems like switching to invoke function-runner directly is not related to handling more advanced input queries?

saga-dasgupta · 2025-10-16T16:27:08Z

src/methods/run-function.ts

 }

+/**
+ * Run a function by calling function-runner directly (bypassing Shopify CLI)


Lets not include this yet because we dont get usage analytics if we use function-runner directly.

lopert · 2025-10-16T17:25:28Z

Sorry I should have posted about the runFunction direct changes. I just added those on top of this to have a faster way to iterate through tests since going through the CLI invocation was taking too much time.

I should have branched off of this and kept that change separate.

For now, I've put this back into draft as I revisit things after discussing with @adampetro .
I'm going to come up with an alternative that doesn't perform type validation by invoking graphql, but instead does it as part of the traversal.

lopert · 2025-10-27T13:12:49Z

Closing in favour of the visitor approach that was merged here: #26

lopert marked this pull request as ready for review October 7, 2025 19:46

lopert requested a review from adampetro October 7, 2025 19:46

adampetro requested changes Oct 8, 2025

View reviewed changes

lopert added 3 commits October 9, 2025 14:55

validate fixture during traversal

4c4f130

Use customResolver to preserve aliases

4a4590a

use custom type resolver, and polish

cc4beb1

lopert force-pushed the lopert.handle-iq-aliases branch from b1009c4 to cc4beb1 Compare October 9, 2025 18:56

lopert added 9 commits October 9, 2025 15:18

remove printType, rely on graphql print

3f23449

return map from extractFieldsFromSelections

4701006

Keep types and similar function grouped

390a733

findMatchingFragment helper

84b812e

prefer for..of loops

cf6816f

fixup! findMatchingFragment helper

e582165

readibility improvements

59bac4a

Handle fragment spreads with different aliases for same field and fix…

9e7b047

… union/interface type detection

support input query variables

d0e35ba

saga-dasgupta reviewed Oct 14, 2025

View reviewed changes

src/methods/validate-fixture-input-types.ts Show resolved Hide resolved

lopert added 3 commits October 14, 2025 17:32

use schema for validation input structure, plus refactor

04e0892

add additional details to js-docs

54a42ef

Improve discount extension tests

130e2aa

lopert changed the title ~~Handle aliases~~ Handle more advanced input queries Oct 15, 2025

add comment detailing graphql call

5f59928

lopert requested review from adampetro and saga-dasgupta October 15, 2025 14:59

lopert mentioned this pull request Oct 15, 2025

Handle aliasing in input queries and fixture input data #23

Closed

lopert added 2 commits October 15, 2025 16:39

update validateTestAssets result to reflect new approach

3d993d6

add ability to invoke functionRunner directly

e5590c1

lopert force-pushed the lopert.handle-iq-aliases branch from 0a04b23 to e5590c1 Compare October 15, 2025 21:20

adampetro reviewed Oct 16, 2025

View reviewed changes

saga-dasgupta reviewed Oct 16, 2025

View reviewed changes

lopert marked this pull request as draft October 16, 2025 17:23

lopert closed this Oct 27, 2025

Uh oh!

Handle more advanced input queries #24

Handle more advanced input queries #24

Uh oh!

Conversation

lopert commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

validateFixtureInputStructure(queryAST, schema, fixtureData)

validateFixtureInputTypes(generatedQuery, schema, fixtureData, variables?)

Uh oh!

adampetro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lopert commented Oct 16, 2025

Uh oh!

lopert commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lopert commented Oct 7, 2025 •

edited

Loading

`validateFixtureInputStructure(queryAST, schema, fixtureData)`