Upload settings_schema.json before validator-consumer assets

.changeset/fix-theme-push-fresh-dynamic-source-defaults.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Upload `config/settings_schema.json` before any other theme file. Fixes `theme push` failing on the first push when blocks or sections reference a `color_palette` theme setting.

packages/theme/src/cli/utilities/theme-fs.test.ts

@@ -473,7 +473,8 @@ describe('theme-fs', () => {
         otherLiquidFiles,
         templateJsonFiles,
         otherJsonFiles,
-        configFiles,
+        configSchemaFile,
+        configDataFile,
         staticAssetFiles,
         contextualizedJsonFiles,
         blockLiquidFiles,
@@ -489,10 +490,8 @@ describe('theme-fs', () => {
       ])
       expect(otherJsonFiles).toEqual([{key: 'locales/en.default.json', checksum: '6'}])
       expect(templateJsonFiles).toEqual([{key: 'templates/404.json', checksum: '7'}])
-      expect(configFiles).toEqual([
-        {key: 'config/settings_schema.json', checksum: '8'},
-        {key: 'config/settings_data.json', checksum: '9'},
-      ])
+      expect(configSchemaFile).toEqual([{key: 'config/settings_schema.json', checksum: '8'}])
+      expect(configDataFile).toEqual([{key: 'config/settings_data.json', checksum: '9'}])
       expect(staticAssetFiles).toEqual([
         {key: 'assets/base.css', checksum: '1'},
         {key: 'assets/sparkle.gif', checksum: '3'},
@@ -511,15 +510,23 @@ describe('theme-fs', () => {
       const files: Checksum[] = []
 
       // When
-      const {sectionLiquidFiles, otherLiquidFiles, templateJsonFiles, otherJsonFiles, configFiles, staticAssetFiles} =
-        partitionThemeFiles(files)
+      const {
+        sectionLiquidFiles,
+        otherLiquidFiles,
+        templateJsonFiles,
+        otherJsonFiles,
+        configSchemaFile,
+        configDataFile,
+        staticAssetFiles,
+      } = partitionThemeFiles(files)
 
       // Then
       expect(sectionLiquidFiles).toEqual([])
       expect(otherLiquidFiles).toEqual([])
       expect(templateJsonFiles).toEqual([])
       expect(otherJsonFiles).toEqual([])
-      expect(configFiles).toEqual([])
+      expect(configSchemaFile).toEqual([])
+      expect(configDataFile).toEqual([])
       expect(staticAssetFiles).toEqual([])
     })
   })

packages/theme/src/cli/utilities/theme-fs.ts

@@ -44,7 +44,8 @@ const THEME_PARTITION_REGEX = {
   layoutLiquidRegex: /^layout\/.+\.liquid$/,
   sectionLiquidRegex: /^sections\/.+\.liquid$/,
   blockLiquidRegex: /^blocks\/.+\.liquid$/,
-  configRegex: /^config\/(settings_schema|settings_data)\.json$/,
+  configSchemaRegex: /^config\/settings_schema\.json$/,
+  configDataRegex: /^config\/settings_data\.json$/,
   sectionJsonRegex: /^sections\/.+\.json$/,
   templateJsonRegex: /^templates\/.+\.json$/,
   jsonRegex: /^(?!config\/).*\.json$/,
@@ -465,7 +466,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
   const templateJsonFiles: T[] = []
   const otherJsonFiles: T[] = []
   const contextualizedJsonFiles: T[] = []
-  const configFiles: T[] = []
+  const configSchemaFile: T[] = []
+  const configDataFile: T[] = []
   const staticAssetFiles: T[] = []
   const blockLiquidFiles: T[] = []
   const layoutFiles: T[] = []
@@ -482,8 +484,10 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
       } else {
         otherLiquidFiles.push(file)
       }
-    } else if (THEME_PARTITION_REGEX.configRegex.test(fileKey)) {
-      configFiles.push(file)
+    } else if (THEME_PARTITION_REGEX.configSchemaRegex.test(fileKey)) {
+      configSchemaFile.push(file)
+    } else if (THEME_PARTITION_REGEX.configDataRegex.test(fileKey)) {
+      configDataFile.push(file)
     } else if (THEME_PARTITION_REGEX.jsonRegex.test(fileKey)) {
       if (THEME_PARTITION_REGEX.contextualizedJsonRegex.test(fileKey)) {
         contextualizedJsonFiles.push(file)
@@ -506,7 +510,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
     templateJsonFiles,
     contextualizedJsonFiles,
     otherJsonFiles,
-    configFiles,
+    configSchemaFile,
+    configDataFile,
     staticAssetFiles,
     blockLiquidFiles,
     layoutFiles,

packages/theme/src/cli/utilities/theme-uploader.test.ts

@@ -1,10 +1,12 @@
 import {renderTasksToStdErr} from './theme-ui.js'
 import {fakeThemeFileSystem} from './theme-fs/theme-fs-mock-factory.js'
 import {
+  ChecksumWithSize,
   MAX_BATCH_BYTESIZE,
   MAX_BATCH_FILE_COUNT,
   MAX_UPLOAD_RETRY_COUNT,
   MINIMUM_THEME_ASSETS,
+  orderFilesToBeUploaded,
   uploadTheme,
   updateUploadErrors,
 } from './theme-uploader.js'
@@ -319,17 +321,19 @@ describe('theme-uploader', () => {
     await renderThemeSyncProgress()
 
     // Then
-    expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(9)
+    expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(10)
     // Minimum theme files start first
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(1, remoteTheme.id, MINIMUM_THEME_ASSETS, adminSession)
-    // Dependent assets start second
+    // settings_schema.json is uploaded first among dependent files so block / section /
+    // section-group / template validators can resolve dynamic-source defaults that
+    // reference theme-level settings declared in the schema.
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
       2,
       remoteTheme.id,
-      [{key: 'layout/theme.liquid'}],
+      [{key: 'config/settings_schema.json'}],
       adminSession,
     )
-    // Independent assets start right after dependent assets start
+    // Independent assets fan out concurrently with the dependent chain.
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
       3,
       remoteTheme.id,
@@ -346,16 +350,22 @@ describe('theme-uploader', () => {
       ],
       adminSession,
     )
-    // Dependent assets continue after the first batch of dependent assets ends
+    // Layout files come after the schema is in place.
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
       4,
       remoteTheme.id,
-      [{key: 'blocks/block.liquid'}],
+      [{key: 'layout/theme.liquid'}],
       adminSession,
     )
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
       5,
       remoteTheme.id,
+      [{key: 'blocks/block.liquid'}],
+      adminSession,
+    )
+    expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
+      6,
+      remoteTheme.id,
       [
         {
           key: 'sections/header.liquid',
@@ -365,7 +375,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      6,
+      7,
       remoteTheme.id,
       [
         {
@@ -376,7 +386,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      7,
+      8,
       remoteTheme.id,
       [
         {
@@ -387,7 +397,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      8,
+      9,
       remoteTheme.id,
       [
         {
@@ -397,21 +407,56 @@ describe('theme-uploader', () => {
       adminSession,
     )
 
+    // settings_data.json must be last
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      9,
+      10,
       remoteTheme.id,
-      [
-        {
-          key: 'config/settings_data.json',
-        },
-        {
-          key: 'config/settings_schema.json',
-        },
-      ],
+      [{key: 'config/settings_data.json'}],
       adminSession,
     )
   })
 
+  test('orders settings_schema.json first and settings_data.json last in the dependent chain, surrounding every validator-consumer asset', () => {
+    // Regression for the fresh-theme bootstrap bug: blocks, sections, section-group
+    // JSONs and template JSONs run server-side validators that resolve dynamic-source
+    // defaults of the form `{{ settings.<theme_setting>.* }}` against the theme's
+    // currently-stored settings_schema.json. If the user's new schema lands AFTER
+    // those assets, validators see an empty schema and the first push errors out.
+    // settings_data.json must then go LAST because its values are validated against
+    // the freshly-uploaded schema and may reference earlier-uploaded sections/templates.
+    const file = (key: string): ChecksumWithSize => ({key, checksum: '_', size: 0})
+    const files: ChecksumWithSize[] = [
+      file('config/settings_schema.json'),
+      file('config/settings_data.json'),
+      file('layout/theme.liquid'),
+      file('blocks/quantity.liquid'),
+      file('sections/header.liquid'),
+      file('sections/header-group.json'),
+      file('templates/product.json'),
+      file('templates/product.context.uk.json'),
+    ]
+
+    const {dependentFiles} = orderFilesToBeUploaded(files)
+    const flat = dependentFiles.flat().map((f) => f.key)
+
+    expect(flat.at(0)).toBe('config/settings_schema.json')
+    expect(flat.at(-1)).toBe('config/settings_data.json')
+
+    const validatorConsumers = [
+      'blocks/quantity.liquid',
+      'sections/header.liquid',
+      'sections/header-group.json',
+      'templates/product.json',
+      'templates/product.context.uk.json',
+    ]
+    for (const key of validatorConsumers) {
+      const index = flat.indexOf(key)
+      expect(index, `${key} missing from dependent chain`).not.toBe(-1)
+      expect(index, `${key} should be after settings_schema.json`).toBeGreaterThan(0)
+      expect(index, `${key} should be before settings_data.json`).toBeLessThan(flat.length - 1)
+    }
+  })
+
   test('should create batches for files when bulk upload file count limit is reached', async () => {
     // Given
     const remoteChecksums: Checksum[] = []

packages/theme/src/cli/utilities/theme-uploader.ts

@@ -20,7 +20,7 @@ interface UploadOptions {
   multiEnvironment?: boolean
 }
 
-type ChecksumWithSize = Checksum & {size: number}
+export type ChecksumWithSize = Checksum & {size: number}
 type FileBatch = ChecksumWithSize[]
 
 /**
@@ -213,7 +213,9 @@ function orderFilesToBeDeleted(files: Checksum[]): Checksum[] {
     ...fileSets.blockLiquidFiles,
     ...fileSets.layoutFiles,
     ...fileSets.otherLiquidFiles,
-    ...fileSets.configFiles,
+    // Inverse of the upload order: data first (consumes schema), then schema.
+    ...fileSets.configDataFile,
+    ...fileSets.configSchemaFile,
     ...fileSets.staticAssetFiles,
   ]
 }
@@ -311,21 +313,34 @@ function selectUploadableFiles(themeFileSystem: ThemeFileSystem, remoteChecksums
  * We use this 2d array to batch files of the same type together
  * while maintaining the order between file types. The files with
  * dependencies we have are:
- * 1. Layout files don't necessarily need to be the first, but they must uploaded before templates.
- * 2. Liquid blocks need to be uploaded before sections
- * 3. Liquid sections need to be uploaded afterwards
- * 4. JSON sections need to be uploaded after sections
- * 5. JSON templates need to be uploaded after all sections and layouts
- * 6. Contextualized templates should be uploaded after as they are variations of templates
- * 7. Config files must be the last ones, but we need to upload config/settings_schema.json first, followed by config/settings_data.json
+ *
+ * 1. config/settings_schema.json must be uploaded FIRST. It declares the
+ *    theme-level settings that block / section / section-group / template
+ *    validators resolve dynamic-source defaults against (e.g. defaults of
+ *    the form {{ settings.<theme_setting>.<property> }}). On a fresh
+ *    theme the stored schema is empty, so any later asset whose schema
+ *    references a not-yet-declared theme setting fails server-side
+ *    validation. Uploading the schema first primes those references.
+ * 2. Layout files don't necessarily need to be the first, but they must be
+ *    uploaded before templates.
+ * 3. Liquid blocks need to be uploaded before sections
+ * 4. Liquid sections need to be uploaded afterwards
+ * 5. JSON sections need to be uploaded after sections
+ * 6. JSON templates need to be uploaded after all sections and layouts
+ * 7. Contextualized templates should be uploaded after as they are
+ *    variations of templates
+ * 8. config/settings_data.json must be uploaded LAST. Its current and
+ *    presets are validated against the freshly-uploaded
+ *    settings_schema.json, and presets can reference sections and
+ *    templates uploaded in earlier steps.
  *
  * The files with no dependencies we have are:
  * - The other Liquid files (for example, snippets, and liquid templates)
  * - The other JSON files (for example, locales)
  * - The static assets
  *
  */
-function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
+export function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
   independentFiles: ChecksumWithSize[][]
   dependentFiles: ChecksumWithSize[][]
 } {
@@ -336,13 +351,18 @@ function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
     independentFiles: [fileSets.otherLiquidFiles, fileSets.otherJsonFiles, fileSets.staticAssetFiles],
     // Follow order of dependencies:
     dependentFiles: [
+      // Theme setting declarations must land before any asset that may
+      // reference them via dynamic-source defaults. See header comment.
+      fileSets.configSchemaFile,
       fileSets.layoutFiles,
       fileSets.blockLiquidFiles,
       fileSets.sectionLiquidFiles,
       fileSets.sectionJsonFiles,
       fileSets.templateJsonFiles,
       fileSets.contextualizedJsonFiles,
-      fileSets.configFiles,
+      // Settings values reference the schema we just uploaded, plus any
+      // sections / templates referenced by presets.
+      fileSets.configDataFile,
     ],
   }
 }