Update app bulk execute to fail if given a mutation document but no variables
#6755
Conversation
Bulk mutations need input data to operate on - without variables, there's nothing to execute. This adds early validation with a clear error message instead of letting the request fail with a cryptic XML error from the API. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
jordanverasamy
changed the title
app bulk execute to fail if given a mutation document but no variables
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3636 tests passing in 1426 suites. Report generated by 🧪jest coverage report action from c0d93db |
| if (isMutation(graphqlOperation) && !variablesJsonl) { | ||
| throw new AbortError( | ||
| outputContent`Bulk mutations require variables. Provide a JSONL file with ${outputToken.yellow( | ||
| '--variable-file', | ||
| )} or individual JSON objects with ${outputToken.yellow('--variables')}.`, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not the correct spot for this in my opinion. OCLIF already has this validation functionality built in via the exactlyOne modifier that I already see is being used for the buildOperationFlags. Can we do this instead? Less code, proper use of the framework, and setting a better pattern!
There was a problem hiding this comment.
We need to know whether the provided GraphQL executable document is a query or a mutation in order to determine whether the provided flags are valid. That means we have to read the flag value, parse it (being done here with isMutation), and only then do we know what to validate.
As far as I understand, that means exactlyOne can't be used here, right? I'm still open to a better/more on-pattern way to do this validation if OCLIF provides a mechanism for it, but not sure what that would be.
There was a problem hiding this comment.
ahh, I gotcha. Depends on the runtime value of the query flag. I don't think OCLIF supports this. This runtime approach is fine in that case, but another question. Some GQL mutations don't require variables. Will we always expect to have them?
There was a problem hiding this comment.
Good question! Some GQL mutations don't require variables, but if that's the case, you would only need to run it once. The only reason to run a mutation in bulk is to run it against N different variable hashes, and so bulk mutations do in fact strictly require an input variable JSONL file.
To match this, app execute allows having no variables, but app bulk execute requires them.
There was a problem hiding this comment.
Perfect, thanks for the explanation. Looks great then!
|
Good catch! |
4 participants
Small bugfix related to https://github.com/shop/issues-api-foundations/issues/1047. Found it while testing the command looking for bugs (https://github.com/shop/issues-api-foundations/issues/1081).
Summary
Without variables, bulk mutations have no input data to operate on. So, this PR adds validation that bulk mutations must include variables (via
--variablesor--variable-file). If not, the user now gets a clear error message instead of a cryptic XML error from the staged upload APITophatting
On
main, runningapp bulk executeagainst a dev store with a mutation but no variables:On this branch, doing the same: