-
Notifications
You must be signed in to change notification settings - Fork 121
[Local Catalog] Integrate catalog API behind a feature flag #16301
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.
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
Changes from 16 commits
bc688c5
cf11514
87489b4
7acf2bb
b5925ac
572ab92
e12af58
6235afe
8748b80
8694085
a6a1c61
d912408
ed9b1a8
527ad51
4f183c1
1166f87
77e49ae
786a89a
e1990ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,17 @@ import class Networking.POSCatalogSyncRemote | |
| import Storage | ||
| import struct Combine.AnyPublisher | ||
| import struct NetworkingCore.JetpackSite | ||
| import struct Networking.POSCatalogResponse | ||
|
|
||
| // TODO - remove the periphery ignore comment when the catalog is integrated with POS. | ||
| // periphery:ignore | ||
| public protocol POSCatalogFullSyncServiceProtocol { | ||
| /// Starts a full catalog sync process | ||
| /// - Parameter siteID: The site ID to sync catalog for | ||
| /// - Parameters: | ||
| /// - siteID: The site ID to sync catalog for | ||
| /// - regenerateCatalog: Whether to force the catalog generation | ||
| /// - Returns: The synced catalog containing products and variations | ||
| func startFullSync(for siteID: Int64) async throws -> POSCatalog | ||
| func startFullSync(for siteID: Int64, regenerateCatalog: Bool) async throws -> POSCatalog | ||
| } | ||
|
|
||
| /// POS catalog from full sync. | ||
|
|
@@ -30,12 +33,14 @@ public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol | |
| private let syncRemote: POSCatalogSyncRemoteProtocol | ||
| private let persistenceService: POSCatalogPersistenceServiceProtocol | ||
| private let batchedLoader: BatchedRequestLoader | ||
| private let usesCatalogAPI: Bool | ||
|
|
||
| public convenience init?(credentials: Credentials?, | ||
| selectedSite: AnyPublisher<JetpackSite?, Never>, | ||
| appPasswordSupportState: AnyPublisher<Bool, Never>, | ||
| batchSize: Int = 2, | ||
| grdbManager: GRDBManagerProtocol) { | ||
| grdbManager: GRDBManagerProtocol, | ||
| usesCatalogAPI: Bool = false) { | ||
| guard let credentials else { | ||
| DDLogError("⛔️ Could not create POSCatalogFullSyncService due missing credentials") | ||
| return nil | ||
|
|
@@ -45,28 +50,35 @@ public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol | |
| appPasswordSupportState: appPasswordSupportState) | ||
| let syncRemote = POSCatalogSyncRemote(network: network) | ||
| let persistenceService = POSCatalogPersistenceService(grdbManager: grdbManager) | ||
| self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService) | ||
| self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService, usesCatalogAPI: usesCatalogAPI) | ||
| } | ||
|
|
||
| init( | ||
| syncRemote: POSCatalogSyncRemoteProtocol, | ||
| batchSize: Int, | ||
| retryDelay: TimeInterval = 2.0, | ||
| persistenceService: POSCatalogPersistenceServiceProtocol | ||
| persistenceService: POSCatalogPersistenceServiceProtocol, | ||
| usesCatalogAPI: Bool = false | ||
| ) { | ||
| self.syncRemote = syncRemote | ||
| self.persistenceService = persistenceService | ||
| self.batchedLoader = BatchedRequestLoader(batchSize: batchSize, retryDelay: retryDelay) | ||
| self.usesCatalogAPI = usesCatalogAPI | ||
| } | ||
|
|
||
| // MARK: - Protocol Conformance | ||
|
|
||
| public func startFullSync(for siteID: Int64) async throws -> POSCatalog { | ||
| DDLogInfo("🔄 Starting full catalog sync for site ID: \(siteID)") | ||
| public func startFullSync(for siteID: Int64, regenerateCatalog: Bool = false) async throws -> POSCatalog { | ||
| DDLogInfo("🔄 Starting full catalog sync for site ID: \(siteID) with regenerateCatalog: \(regenerateCatalog)") | ||
|
|
||
| do { | ||
| // Sync from network | ||
| let catalog = try await loadCatalog(for: siteID, syncRemote: syncRemote) | ||
| let catalog: POSCatalog | ||
| if usesCatalogAPI { | ||
| catalog = try await loadCatalogFromCatalogAPI(for: siteID, syncRemote: syncRemote, regenerateCatalog: regenerateCatalog) | ||
| } else { | ||
| catalog = try await loadCatalog(for: siteID, syncRemote: syncRemote) | ||
| } | ||
| DDLogInfo("✅ Loaded \(catalog.products.count) products and \(catalog.variations.count) variations for siteID \(siteID)") | ||
|
|
||
| // Persist to database | ||
|
|
@@ -102,4 +114,61 @@ private extension POSCatalogFullSyncService { | |
| return POSCatalog(products: products, variations: variations, syncDate: syncStartDate) | ||
| } | ||
|
|
||
| func loadCatalogFromCatalogAPI(for siteID: Int64, syncRemote: POSCatalogSyncRemoteProtocol, regenerateCatalog: Bool) async throws -> POSCatalog { | ||
| let downloadStartTime = CFAbsoluteTimeGetCurrent() | ||
| let catalog = try await downloadCatalog(for: siteID, syncRemote: syncRemote, regenerateCatalog: regenerateCatalog) | ||
| let downloadTime = CFAbsoluteTimeGetCurrent() - downloadStartTime | ||
| DDLogInfo("🟣 Catalog download completed - Time: \(String(format: "%.2f", downloadTime))s") | ||
|
|
||
| return .init(products: catalog.products, variations: catalog.variations, syncDate: .init()) | ||
| } | ||
| } | ||
|
|
||
| private extension POSCatalogFullSyncService { | ||
| func downloadCatalog(for siteID: Int64, syncRemote: POSCatalogSyncRemoteProtocol, regenerateCatalog: Bool) async throws -> POSCatalogResponse { | ||
| DDLogInfo("🟣 Starting catalog request...") | ||
|
|
||
| // 1. Requests catalog until download URL is available. | ||
| let response = try await syncRemote.requestCatalogGeneration(for: siteID, forceGeneration: regenerateCatalog) | ||
| let downloadURL: String? | ||
| if let url = response.downloadURL { | ||
| downloadURL = url | ||
| } else { | ||
| // 2. Polls for completion until download URL is available. | ||
| downloadURL = try await pollForCatalogCompletion(siteID: siteID, syncRemote: syncRemote) | ||
| } | ||
|
|
||
| // 3. Downloads catalog using the provided URL. | ||
| guard let downloadURL else { | ||
| throw POSCatalogSyncError.invalidData | ||
| } | ||
| DDLogInfo("🟣 Catalog ready for download: \(downloadURL)") | ||
| return try await syncRemote.downloadCatalog(for: siteID, downloadURL: downloadURL) | ||
| } | ||
|
|
||
| func pollForCatalogCompletion(siteID: Int64, syncRemote: POSCatalogSyncRemoteProtocol) async throws -> String { | ||
| // Each attempt is made 1 second after the last one completes. | ||
| let maxAttempts = 1000 | ||
| var attempts = 0 | ||
|
|
||
| while attempts < maxAttempts { | ||
| let response = try await syncRemote.requestCatalogGeneration(for: siteID, forceGeneration: false) | ||
|
|
||
| switch response.status { | ||
| case .complete: | ||
| guard let downloadURL = response.downloadURL else { | ||
| throw POSCatalogSyncError.invalidData | ||
| } | ||
| return downloadURL | ||
| case .pending, .processing: | ||
| DDLogInfo("🟣 Catalog request \(response.status)... (attempt \(attempts + 1)/\(maxAttempts))") | ||
|
||
| try await Task.sleep(nanoseconds: 1_000_000_000) // 1 second | ||
| attempts += 1 | ||
| case .failed: | ||
| throw POSCatalogSyncError.generationFailed | ||
| } | ||
| } | ||
|
|
||
| throw POSCatalogSyncError.timeout | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,8 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { | |||||
| func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { | ||||||
| DDLogInfo("💾 Persisting catalog with \(catalog.products.count) products and \(catalog.variations.count) variations") | ||||||
|
|
||||||
| let catalog = filterOrphanedVariations(from: catalog) | ||||||
|
|
||||||
| try await grdbManager.databaseConnection.write { db in | ||||||
| DDLogInfo("🗑️ Clearing catalog data for site \(siteID)") | ||||||
| try PersistedSite.deleteOne(db, key: siteID) | ||||||
|
|
@@ -154,6 +156,22 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| private extension POSCatalogPersistenceService { | ||||||
| /// Filters out variations whose parent products are not in the catalog. | ||||||
| /// This can happen when the API returns public variations but their parent products are not public. | ||||||
| func filterOrphanedVariations(from catalog: POSCatalog) -> POSCatalog { | ||||||
| let productIDs = catalog.products.map { $0.productID } | ||||||
| let variations = catalog.variations.filter { variation in | ||||||
| let parentExists = productIDs.contains { $0 == variation.productID } | ||||||
| if !parentExists { | ||||||
| DDLogWarn("Variation \(variation.productVariationID) references missing product \(variation.productID)") | ||||||
|
||||||
| DDLogWarn("Variation \(variation.productVariationID) references missing product \(variation.productID)") | |
| DDLogWarn("Variation \(variation.productVariationID) references missing product \(variation.productID) - it will not be available in POS.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 786a89a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should have some backoff here – e.g. after a minute of checking every second, maybe every 5 seconds is fine even if it means someone's waiting 4 seconds longer than they would with 1s polling.
I also wonder whether we might have any rate limiting issues doing this with some hosts, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, some progressive backoff (1s → 5s after 1 minute like you suggested) would be good here. Polling every second for potentially long-running catalog generations unnecessarily consumes server resources. I also plan to add some basic retry mechanism for the polling, so that one failing request doesn't end the full sync. I will implement both in a future PR.
I did some search about rate limiting in WP/WC, there doesn't seem to be a built-in rate limiting system for the plugins and WC API. The WC Store API does have rate limiting support, with default maximum of 25 requests per 10 seconds. Some plugins like WordFence offer the rate limiting feature. Other findings:
Right now, the polling frequency is a bit under 1 request per second due to the overhead of the request (one request is made after the end of the previous one), and likely fine for the common default settings. However, the backoff implementation will further help reduce the risk of encountering server resource issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the research!