Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions packages/app/src/cli/services/build/extension.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {buildFunctionExtension} from './extension.js'
import {testFunctionExtension} from '../../models/app/app.test-data.js'
import {buildGraphqlTypes, buildJSFunction, runWasmOpt, runTrampoline} from '../function/build.js'
import {validateSchemaApiVersion} from '../function/schema-version.js'
import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
import {beforeEach, describe, expect, test, vi} from 'vitest'
Expand All @@ -12,6 +13,7 @@ import {joinPath} from '@shopify/cli-kit/node/path'

vi.mock('@shopify/cli-kit/node/system')
vi.mock('../function/build.js')
vi.mock('../function/schema-version.js')
vi.mock('proper-lockfile')
vi.mock('@shopify/cli-kit/node/fs')

Expand Down Expand Up @@ -418,6 +420,26 @@ describe('buildFunctionExtension', () => {
expect(runWasmOpt).toHaveBeenCalled()
})

test('validates the schema api_version with the values from the extension config', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The title of this test makes me think that there will be some validation assertion but only thing we do is assert that we called the validate function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. The actual logic is covered in the schema-version code so this is in fact just making sure we're invoking it with the right params

// When
await expect(
buildFunctionExtension(extension, {
stdout,
stderr,
signal,
app,
environment: 'production',
}),
).resolves.toBeUndefined()

// Then
expect(validateSchemaApiVersion).toHaveBeenCalledWith({
directory: extension.directory,
localIdentifier: extension.localIdentifier,
apiVersion: extension.configuration.api_version,
})
})

test('does not rebundle when build.path stays in the default output directory', async () => {
// Given
extension.configuration.build!.path = 'dist/custom.wasm'
Expand Down
13 changes: 11 additions & 2 deletions packages/app/src/cli/services/build/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {formatBundleSize} from './bundle-size.js'
import {AppInterface} from '../../models/app/app.js'
import {bundleExtension} from '../extensions/bundle.js'
import {buildGraphqlTypes, buildJSFunction, runTrampoline, runWasmOpt} from '../function/build.js'
import {validateSchemaApiVersion} from '../function/schema-version.js'
import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
import {exec} from '@shopify/cli-kit/node/system'
Expand Down Expand Up @@ -156,12 +157,20 @@ export async function buildFunctionExtension(
}

try {
const functionConfiguration = (extension as ExtensionInstance<FunctionConfigType>).configuration
const bundlePath = extension.outputPath
const relativeBuildPath =
(extension as ExtensionInstance<FunctionConfigType>).configuration.build?.path ?? extension.outputRelativePath
const relativeBuildPath = functionConfiguration.build?.path ?? extension.outputRelativePath

extension.outputPath = joinPath(extension.directory, relativeBuildPath)

if (functionConfiguration.api_version) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this condition, why wouldn't there be an api_version in the function_configuration?

await validateSchemaApiVersion({
directory: extension.directory,
localIdentifier: extension.localIdentifier,
apiVersion: functionConfiguration.api_version,
})
}

if (extension.isJavaScript) {
await runCommandOrBuildJSFunction(extension, options)
} else {
Expand Down
110 changes: 110 additions & 0 deletions packages/app/src/cli/services/function/schema-version.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import {
prependSchemaVersionHeader,
readSchemaApiVersion,
validateSchemaApiVersion,
SCHEMA_VERSION_MARKER_PREFIX,
} from './schema-version.js'
import {describe, expect, test} from 'vitest'
import {AbortError} from '@shopify/cli-kit/node/error'
import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'

function options(directory: string, apiVersion: string) {
return {directory, localIdentifier: 'my-function', apiVersion}
}

describe('prependSchemaVersionHeader', () => {
test('prepends a comment block with the version marker', () => {
const result = prependSchemaVersionHeader('type Query { id: ID }', '2025-10')

expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10`)).toBe(true)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we assert there's a newline at the end here?

Suggested change
expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10`)).toBe(true)
expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10\n`)).toBe(true)

expect(result.endsWith('type Query { id: ID }')).toBe(true)
})
})

describe('readSchemaApiVersion', () => {
test('returns the version when the marker is present', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const path = joinPath(tmpDir, 'schema.graphql')
await writeFile(path, prependSchemaVersionHeader('type Query { id: ID }', '2025-10'))

await expect(readSchemaApiVersion(path)).resolves.toEqual('2025-10')
})
})

test('returns undefined when the file has no marker', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const path = joinPath(tmpDir, 'schema.graphql')
await writeFile(path, '# some other comment\ntype Query { id: ID }')

await expect(readSchemaApiVersion(path)).resolves.toBeUndefined()
})
})

test('returns undefined when the file does not exist', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await expect(readSchemaApiVersion(joinPath(tmpDir, 'missing.graphql'))).resolves.toBeUndefined()
})
})

test('does not match the marker once SDL content has started', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const path = joinPath(tmpDir, 'schema.graphql')
// Marker buried after SDL content should be ignored.
await writeFile(path, `type Query { id: ID }\n${SCHEMA_VERSION_MARKER_PREFIX}2025-10\n`)

await expect(readSchemaApiVersion(path)).resolves.toBeUndefined()
})
})

test('finds the marker when it is not the first comment line', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const path = joinPath(tmpDir, 'schema.graphql')
await writeFile(path, `# preamble comment\n${SCHEMA_VERSION_MARKER_PREFIX}unstable\ntype Query { id: ID }`)

await expect(readSchemaApiVersion(path)).resolves.toEqual('unstable')
})
})
})

describe('validateSchemaApiVersion', () => {
test('no-ops when the schema file does not exist', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await expect(validateSchemaApiVersion(options(tmpDir, '2025-10'))).resolves.toBeUndefined()
})
})

test('no-ops when the schema file has no version marker (legacy)', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we remove legacy, I think it's something good to have in perpetuity, but legacy implies to me that at some point this case might go away

await inTemporaryDirectory(async (tmpDir) => {
await writeFile(joinPath(tmpDir, 'schema.graphql'), 'type Query { id: ID }')

await expect(validateSchemaApiVersion(options(tmpDir, '2025-10'))).resolves.toBeUndefined()
})
})

test('no-ops when the marker matches the configured api_version', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await writeFile(
joinPath(tmpDir, 'schema.graphql'),
prependSchemaVersionHeader('type Query { id: ID }', '2025-10'),
)

await expect(validateSchemaApiVersion(options(tmpDir, '2025-10'))).resolves.toBeUndefined()
})
})

test('throws an AbortError with remediation when the marker is stale', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await writeFile(
joinPath(tmpDir, 'schema.graphql'),
prependSchemaVersionHeader('type Query { id: ID }', '2025-07'),
)

const result = validateSchemaApiVersion(options(tmpDir, '2025-10'))

await expect(result).rejects.toThrow(AbortError)
await expect(result).rejects.toThrow(/2025-07/)
await expect(result).rejects.toThrow(/2025-10/)
Comment on lines +106 to +107
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we combine these into one regex so we know that the ordering is correct?

})
})
})
77 changes: 77 additions & 0 deletions packages/app/src/cli/services/function/schema-version.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import {AbortError} from '@shopify/cli-kit/node/error'
import {fileExists, readFile} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'
import {outputContent, outputToken} from '@shopify/cli-kit/node/output'

/**
* Marker used in the leading comments of `schema.graphql` to record the
* `api_version` the schema was fetched for. Format: `# api_version: <version>`.
*/
export const SCHEMA_VERSION_MARKER_PREFIX = '# api_version: '

/**
* Prepends a versioned header to a schema definition. The header documents
* which `api_version` the schema was generated for so subsequent builds can
* detect when the on-disk schema is stale.
*/
export function prependSchemaVersionHeader(definition: string, apiVersion: string): string {
return `${SCHEMA_VERSION_MARKER_PREFIX}${apiVersion}\n\n${definition}`
}

/**
* Reads the `api_version` recorded in the leading comments of a schema file.
* Returns `undefined` if the file does not have the marker (e.g. legacy files
* generated before this header existed, or hand-authored schemas).
*/
export async function readSchemaApiVersion(filePath: string): Promise<string | undefined> {
if (!(await fileExists(filePath))) {
return undefined
}

const contents = await readFile(filePath)
// Only inspect the leading comment block — bail out as soon as we see a
// non-comment, non-empty line so we don't scan the whole SDL.
for (const line of contents.split(/\r?\n/)) {
if (line.startsWith(SCHEMA_VERSION_MARKER_PREFIX)) {
return line.slice(SCHEMA_VERSION_MARKER_PREFIX.length)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we trim too in case there is leading or trailing whitespace?

}
if (!line.startsWith('#')) break
}
Comment on lines +32 to +39
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we really need to inspect the whole comment block and not just the first line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My concern here was trying to not rely on being the first line. Stuff like:

  • Someone adds a # DO NOT EDIT / license / TODO comment above it
  • A future change prepends another metadata line (e.g. # generated_at: …)
  • Some tool reformats the file

I guess for now, since I'm the only one adding a special metadata comment, it's fine to just look at the first line.

return undefined
}

/**
* Validates that `<extension>/schema.graphql` matches the `api_version`
* declared in the extension TOML. Throws an `AbortError` with a remediation
* pointing at `shopify app function schema` when the on-disk schema is stale.
*
* Silently no-ops when:
* - the schema file does not exist (handled by codegen / out of scope here)
* - the schema file has no version marker (legacy file — we don't want to
* break existing setups)
*/
interface ValidateSchemaApiVersionOptions {
directory: string
localIdentifier: string
apiVersion: string
}

export async function validateSchemaApiVersion({
directory,
localIdentifier,
apiVersion,
}: ValidateSchemaApiVersionOptions): Promise<void> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the validate function just return a pure True or False type validation (with it defaulting to True if it cant verify because the schema file doesn't have the version) and then the extension file is responsible for throwing the AbortError like it does for other cases? So this method is pure validation, not responsible for responding to the user, that happens at the caller?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so? I'm not sure what the other cases are here.
In extensions.ts, we're catching errors from other things (such as trying to acquire the lock) and re-throwing an AbortError. I think this is different than validators, such as validateShopifyFunctionPackageVersion, which do throw their own errors.

The validator also knows what it's looking for, and the error provided can include more detail (i.e. the different versions).

const schemaPath = joinPath(directory, 'schema.graphql')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm, not ideal that we're hardcoding the file name but I don't think we have this in the config anywhere 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like we discussed on slack, we've got this hardcoded in a few places.
It's actually just the function build command that ends up looking at the package.json/codegen/schema property, so there's already a disconnect today if someone changes that file.

I've created an issue for this, but for this PR, I'll keep things consistent with the rest of the code.

const versionFromSchema = await readSchemaApiVersion(schemaPath)
if (versionFromSchema === undefined) return
if (versionFromSchema === apiVersion) return

throw new AbortError(
outputContent`The ${outputToken.cyan(
'schema.graphql',
)} file for ${outputToken.yellow(localIdentifier)} was generated for api_version ${outputToken.yellow(
versionFromSchema,
)} but your function is now on api_version ${outputToken.yellow(apiVersion)}.`,
outputContent`Run ${outputToken.genericShellCommand('shopify app function schema')} to refresh it.`,
)
}
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/generate-schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe('generateSchemaService', () => {

// Then
const outputFile = await readFile(joinPath(extension.directory, 'schema.graphql'))
expect(outputFile).toEqual('schema')
expect(outputFile).toEqual(`# api_version: ${extension.configuration.api_version}\n\nschema`)
})
})

Expand All @@ -72,7 +72,7 @@ describe('generateSchemaService', () => {
})

// Then
expect(mockOutput).toHaveBeenCalledWith('schema')
expect(mockOutput).toHaveBeenCalledWith(`# api_version: ${extension.configuration.api_version}\n\nschema`)
})
})

Expand Down
5 changes: 4 additions & 1 deletion packages/app/src/cli/services/generate-schema.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {prependSchemaVersionHeader} from './function/schema-version.js'
import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js'
import {SchemaDefinitionByApiTypeQueryVariables} from '../api/graphql/functions/generated/schema-definition-by-api-type.js'
import {SchemaDefinitionByTargetQueryVariables} from '../api/graphql/functions/generated/schema-definition-by-target.js'
Expand All @@ -22,7 +23,7 @@ export async function generateSchemaService(options: GenerateSchemaOptions) {
const apiKey = app.configuration.client_id
const {api_version: version, type, targeting} = extension.configuration
const usingTargets = Boolean(targeting?.length)
const definition = await (usingTargets
const fetchedDefinition = await (usingTargets
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont understand the difference between definition and fetched_definition, what if we keep this definition and call the other definition_with_version or something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

definition is essentially what was here before, the contents we're about to write to the local schema.

I introduced fetchedDefinition to name the intermediate value: the raw schema we just grabbed from the server, before the prepending step.

Reading top-down: fetch -> add version header -> write.
Whereas before it was just fetch -> write.
Open to renaming if it still reads confusingly, but it felt clearer to me separated.

? generateSchemaFromTarget({
localIdentifier: extension.localIdentifier,
developerPlatformClient,
Expand All @@ -41,6 +42,8 @@ export async function generateSchemaService(options: GenerateSchemaOptions) {
orgId,
}))

const definition = prependSchemaVersionHeader(fetchedDefinition, version)

if (stdout) {
outputResult(definition)
} else {
Expand Down
Loading