[Local Catalog] Update schema with many-to-many images #16222

joshheald · 2025-10-07T18:09:04Z

Merge after: #16219

Description

This PR updates our attribute schemas to support using the same image for multiple products.

Steps to reproduce

This can't be properly tested in the app yet, as the database isn't used in the POS UI.

You can put breakpoints in the POSCatalogPersistenceService.replaceAllCatalogData(_:siteID:) function and check that they are created, or find the SQLite database file from your simulator and open it with a tool like SQLPro... but mostly, just checking that it logs the expected number of products/variations being synced should be enough. We'll see any issues soon enough when it's in the UI, anyway.

You can get the database file for a running simulator by:

run xcrun simctl get_app_container booted com.automattic.woocommerce data in the terminal
Cmd-G in Finder and paste the path
Open Documents
You should see woo-local.sqlite

Screenshots

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

dangermattic · 2025-10-07T18:09:38Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2025-10-07T18:19:32Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16222-887104e`
	Version	`23.4`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`887104e`
	Installation URL	3mfi1v0b2u1a0

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Copilot

Pull Request Overview

This PR updates the local catalog schema and persistence to support many-to-many relationships between products/variations and images, allowing image records to be shared.

Introduces a centralized PersistedImage table and shifts product/variation image references to join tables.
Updates persistence flows to insert/update shared images and manage join entries.
Adjusts tests and conversion utilities accordingly.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift	Adds image table and converts product/variation image tables to join tables.
Modules/Sources/Storage/GRDB/Model/PersistedImage.swift	Adds PersistedImage model for shared image data.
Modules/Sources/Storage/GRDB/Model/PersistedProductImage.swift	Refactors to a product-image join model; adds association to PersistedImage.
Modules/Sources/Storage/GRDB/Model/PersistedProductVariationImage.swift	Refactors to a variation-image join model; adds association to PersistedImage.
Modules/Sources/Storage/GRDB/Model/PersistedProduct.swift	Updates associations to fetch images through join table.
Modules/Sources/Storage/GRDB/Model/PersistedProductVariation.swift	Adds through-association to PersistedImage via join table.
Modules/Sources/Yosemite/Model/Storage/PersistedImage+Conversions.swift	Adds conversions between ProductImage and PersistedImage.
Modules/Sources/Yosemite/Model/Storage/PersistedProduct+Conversions.swift	Saves shared images and creates join entries for products.
Modules/Sources/Yosemite/Model/Storage/PersistedProductVariation+Conversions.swift	Saves shared images and creates join entries for variations.
Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift	Persists shared images first, then join rows; deletes stale join rows in subset replace.
Modules/Tests/YosemiteTests/...	Updates tests to the new schema and assertions around images and join rows.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-07T18:19:42Z

Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift

+                                         references: "image",
+                                         columns: ["siteID", "id"],
+                                         onDelete: .cascade)
        }


Consider adding indexes on the foreign key columns to improve join/filter performance, e.g., composite indexes on (siteID, productID) and (siteID, imageID). You can add them after table creation with db.create(index:, on:), which will significantly help common queries resolving product→images and image→products.

Suggested change

}

}

// Add composite indexes to improve join/filter performance

try db.create(index: "productImage_siteID_productID", on: "productImage", columns: ["siteID", "productID"])

try db.create(index: "productImage_siteID_imageID", on: "productImage", columns: ["siteID", "imageID"])

Copilot · 2025-10-07T18:19:43Z

Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift

+                                                  references: "image",
+                                                  columns: ["siteID", "id"],
+                                                  onDelete: .cascade)
        }


Similar to products, add indexes on (siteID, productVariationID) and (siteID, imageID) to optimize queries traversing variation↔image relationships.

Suggested change

}

}

// Add indexes to optimize queries traversing variation↔image relationships

try db.create(indexOn: "productVariationImage", columns: ["siteID", "productVariationID"])

try db.create(indexOn: "productVariationImage", columns: ["siteID", "imageID"])

Copilot · 2025-10-07T18:19:43Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift

+        let variationImages = variations.compactMap { variation in
+            variation.image.map { PersistedImage.make(from: $0, siteID: variation.siteID) }
+        }
+        return productImages + variationImages


This builds a non-unique list and will call save multiple times per duplicate image. Deduplicate by (siteID, id) before persisting to reduce unnecessary upserts, e.g., by using a Dictionary keyed by (siteID, id) or a Set of IDs to return unique images.

Suggested change

return productImages + variationImages

let allImages = productImages + variationImages

// Deduplicate by (siteID, imageID)

var uniqueImages: [PersistedImage] = []

var seenKeys = Set<String>()

for image in allImages {

let key = "\(image.siteID)-\(image.imageID)"

if !seenKeys.contains(key) {

uniqueImages.append(image)

seenKeys.insert(key)

}

}

return uniqueImages

+1 to keep the images unique.

Updated in 262de6e

Copilot · 2025-10-07T18:19:43Z

Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift

+            imageTable.primaryKey(["siteID", "id"]) // SiteID column created by belongsTo relationship
+            imageTable.belongsTo("site", onDelete: .cascade)


[nitpick] For clarity and to avoid potential migration-order pitfalls, consider declaring belongsTo("site") (which defines siteID) before setting the composite primary key that references siteID. Reordering these lines makes the dependency explicit and easier to reason about.

Suggested change

imageTable.primaryKey(["siteID", "id"]) // SiteID column created by belongsTo relationship

imageTable.belongsTo("site", onDelete: .cascade)

imageTable.belongsTo("site", onDelete: .cascade)

imageTable.primaryKey(["siteID", "id"]) // SiteID column created by belongsTo relationship

Copilot · 2025-10-07T18:19:44Z

Modules/Sources/Storage/GRDB/Model/PersistedProductImage.swift

 // periphery:ignore - TODO: remove ignore when populating database
+/// Join table linking products to images (many-to-many relationship)


The periphery:ignore looks obsolete now that this type is actively used; consider removing it so the static analysis tool can properly flag real unused symbols.

Copilot · 2025-10-07T18:19:44Z

Modules/Sources/Storage/GRDB/Model/PersistedProductVariationImage.swift

 // periphery:ignore - TODO: remove ignore when populating database
+/// Join table linking product variations to images (many-to-many relationship)


Same here: please drop periphery:ignore as this join model is now part of normal persistence and associations.

jaclync

Looks good, just some non-blocking questions 🚢

The other CI test failure seems like a flaky test from concurrently updating the mock count variables and unrelated to this PR:

init_with_custom_batch_size_uses_specified_size() in POSCatalogFullSyncServiceTests
POSCatalogFullSyncServiceTests.swift:179: Expectation failed: (mockSyncRemote.loadProductsCallCount → 4) == 5 - // Then

I will look into this separately.

jaclync · 2025-10-08T03:48:15Z

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift

+            // Check actual images
+            let images = try PersistedImage.fetchAll(db).sorted(by: { $0.id < $1.id })
+            #expect(images[0].src == "https://example.com/image1-1.jpg")
+            #expect(images[1].src == "https://example.com/image3.jpg")


CI is failing on this line:

persistIncrementalCatalogData_replaces_images_for_updated_product() in POSCatalogPersistenceServiceTests POSCatalogPersistenceServiceTests.swift:337: Expectation failed: (images[1].src → "https://example.com/image2.jpg") == "https://example.com/image3.jpg"

My understanding of the failure:

Since this test case performs an incremental sync, it doesn't delete images that are no longer in the products/variations in the sync response. Before the sync of 1 product, there are image id 1 and 2, and this product has image id 1 and 3 after the sync. The incremental sync just inserts image id 3 without deleting image id 2. Thus, the database now has image id 1, 2, and 3. images here has 3 values, and when sorted by id, images[1] is the image with id 2.

We probably want to assert on PersistedImage with id 1 and 3 here. We can also add an assert that images.count is 3 for this incremental sync behavior.

Yep, thanks, you're spot on. Fixed in 2566193

jaclync · 2025-10-08T03:54:14Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift

-                try attribute.insert(db)
+            // Insert actual image data first (shared by products and variations)
+            for image in catalog.imagesToPersist {
+                try image.save(db)


❓ What are the reasons the image is inserted with save while other entities are inserted with insert?

No good reasons for this, updated to use insert(_ onConflict: .replace)

jaclync · 2025-10-08T05:06:28Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift


+            // Insert/update actual image data (shared by products and variations)
+            for image in catalog.imagesToPersist {
+                try image.save(db)


Similar question to the full sync version, what are the reasons of using save instead of insert with replace?

jaclync · 2025-10-08T05:07:27Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift

+        let variationImages = variations.compactMap { variation in
+            variation.image.map { PersistedImage.make(from: $0, siteID: variation.siteID) }
+        }
+        return productImages + variationImages


+1 to keep the images unique.

joshheald added 3 commits October 7, 2025 18:40

Use joining tables for Images

ba19aab

Make persistence service put images in new table

1b311fc

Update tests for shared image table

a8b474e

joshheald added this to the 23.5 milestone Oct 7, 2025

joshheald added type: task An internally driven task. feature: POS labels Oct 7, 2025

joshheald marked this pull request as ready for review October 7, 2025 18:12

joshheald changed the base branch from trunk to woomob-1339-schema-global-attribute-support October 7, 2025 18:13

joshheald requested review from Copilot and jaclync October 7, 2025 18:14

Copilot AI reviewed Oct 7, 2025

View reviewed changes

jaclync approved these changes Oct 8, 2025

View reviewed changes

Base automatically changed from woomob-1339-schema-global-attribute-support to trunk October 8, 2025 06:31

joshheald added 3 commits October 8, 2025 12:42

Update test for no deletion on incremental sync

2566193

Match insert method when persisting images

fe8bda6

Deduplicate images before persisting them

262de6e

joshheald enabled auto-merge October 8, 2025 12:29

joshheald added 2 commits October 8, 2025 13:47

Periphery ignore

08ba3a6

Merge branch 'trunk' into woomob-1339-schema-many-to-many-images

887104e

joshheald merged commit 4f592e2 into trunk Oct 8, 2025
13 checks passed

joshheald deleted the woomob-1339-schema-many-to-many-images branch October 8, 2025 13:08

-        }
+        }
+        // Add composite indexes to improve join/filter performance
+        try db.create(index: "productImage_siteID_productID", on: "productImage", columns: ["siteID", "productID"])
+        try db.create(index: "productImage_siteID_imageID", on: "productImage", columns: ["siteID", "imageID"])

-        }
+        }
+        // Add indexes to optimize queries traversing variation↔image relationships
+        try db.create(indexOn: "productVariationImage", columns: ["siteID", "productVariationID"])
+        try db.create(indexOn: "productVariationImage", columns: ["siteID", "imageID"])

-        return productImages + variationImages
+        let allImages = productImages + variationImages
+        // Deduplicate by (siteID, imageID)
+        var uniqueImages: [PersistedImage] = []
+        var seenKeys = Set<String>()
+        for image in allImages {
+            let key = "\(image.siteID)-\(image.imageID)"
+            if !seenKeys.contains(key) {
+                uniqueImages.append(image)
+                seenKeys.insert(key)
+            }
+        }
+        return uniqueImages

		imageTable.primaryKey(["siteID", "id"]) // SiteID column created by belongsTo relationship
		imageTable.belongsTo("site", onDelete: .cascade)

		// periphery:ignore - TODO: remove ignore when populating database
		/// Join table linking products to images (many-to-many relationship)

		// periphery:ignore - TODO: remove ignore when populating database
		/// Join table linking product variations to images (many-to-many relationship)

[Local Catalog] Update schema with many-to-many images #16222

[Local Catalog] Update schema with many-to-many images #16222

Uh oh!

Conversation

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

Uh oh!

Description

Steps to reproduce

Screenshots

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

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!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joshheald commented Oct 7, 2025 •

edited

Loading

dangermattic commented Oct 7, 2025 •

edited

Loading

wpmobilebot commented Oct 7, 2025 •

edited

Loading