Skip to content

Update app bulk execute to fail if given a mutation document but no variables #6755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jordanverasamy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update app bulk execute to fail if given a mutation document but no variables #6755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion packages/app/src/cli/services/bulk-operations/execute-bulk-operation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ describe('executeBulkOperation', () => {

test('runs mutation operation when GraphQL document starts with mutation', async () => {
const mutation = 'mutation productUpdate($input: ProductInput!) { productUpdate(input: $input) { product { id } } }'
const variables = ['{"input":{"id":"gid://shopify/Product/123"}}']
const mockResponse: BulkOperationRunMutationMutation['bulkOperationRunMutation'] = {
bulkOperation: createdBulkOperation,
userErrors: [],
Expand All @@ -145,12 +146,13 @@ describe('executeBulkOperation', () => {
remoteApp: mockRemoteApp,
store: mockStore,
query: mutation,
variables,
})

expect(runBulkOperationMutation).toHaveBeenCalledWith({
adminSession: mockAdminSession,
query: mutation,
variablesJsonl: undefined,
variablesJsonl: '{"input":{"id":"gid://shopify/Product/123"}}',
version: BULK_OPERATIONS_MIN_API_VERSION,
})
expect(runBulkOperationQuery).not.toHaveBeenCalled()
Expand Down Expand Up @@ -326,6 +328,22 @@ describe('executeBulkOperation', () => {
})
})

test('throws error when mutation is provided without variables', async () => {
const mutation = 'mutation productUpdate($input: ProductInput!) { productUpdate(input: $input) { product { id } } }'

await expect(
executeBulkOperation({
organization: mockOrganization,
remoteApp: mockRemoteApp,
store: mockStore,
query: mutation,
}),
).rejects.toThrow('Bulk mutations require variables')

expect(runBulkOperationQuery).not.toHaveBeenCalled()
expect(runBulkOperationMutation).not.toHaveBeenCalled()
})

test('uses watchBulkOperation (not quickWatchBulkOperation) when watch flag is true', async () => {
const query = '{ products { edges { node { id } } } }'
const initialResponse: BulkOperationRunQueryMutation['bulkOperationRunQuery'] = {
Expand Down
11 changes: 8 additions & 3 deletions packages/app/src/cli/services/bulk-operations/execute-bulk-operation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,15 @@ function resultsContainUserErrors(results: string): boolean {
})
}

/**
* Validates bulk operation-specific constraints for variables.
*/
function validateBulkOperationVariables(graphqlOperation: string, variablesJsonl?: string): void {
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')}.`,
)
}

Comment on lines +211 to +218
Copy link
Contributor

@alexanderMontague alexanderMontague Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@jordanverasamy jordanverasamy Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alexanderMontague alexanderMontague Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jordanverasamy jordanverasamy Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alexanderMontague alexanderMontague Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks for the explanation. Looks great then!

if (!isMutation(graphqlOperation) && variablesJsonl) {
throw new AbortError(
outputContent`The ${outputToken.yellow('--variables')} and ${outputToken.yellow(
Expand Down
Loading