Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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
12 changes: 7 additions & 5 deletions Modules/Sources/Networking/Model/Product/Product.swift
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public struct Product: Codable, GeneratedCopiable, Equatable, GeneratedFakeable
// `true`
value.lowercased() == Values.manageStockParent ? true : false
})
]) ?? false
]) ?? false

// Even though WooCommerce Core returns Int or null values,
// some plugins alter the field value from Int to Decimal or String.
Expand Down Expand Up @@ -535,15 +535,17 @@ public struct Product: Codable, GeneratedCopiable, Equatable, GeneratedFakeable
let menuOrder = try container.decode(Int.self, forKey: .menuOrder)

// Filter out metadata if the key is prefixed with an underscore (internal meta keys)
let customFields = (try? container.decode([MetaData].self, forKey: .metadata).filter({ !$0.key.hasPrefix("_")})) ?? []
// Support both array format and object keyed by index strings
let allMetaData = [MetaData].decodeFlexibly(from: container, forKey: .metadata)
let customFields = allMetaData.filter { !$0.key.hasPrefix("_") }

// In some isolated cases, it appears to be some malformed meta-data that causes this line to throw hence the whole product decoding to throw.
// Since add-ons are optional, `try?` will be used to prevent the whole decoding to stop.
// https://github.com/woocommerce/woocommerce-ios/issues/4205
let addOns = (try? container.decodeIfPresent(ProductAddOnEnvelope.self, forKey: .metadata)?.revolve()) ?? []

let metaDataExtractor = try? container.decodeIfPresent(ProductMetadataExtractor.self, forKey: .metadata)
let isSampleItem = (metaDataExtractor?.extractStringValue(forKey: MetadataKeys.headStartPost) == Values.headStartValue)
let metaDataExtractor = ProductMetadataExtractor(metadata: allMetaData)
let isSampleItem = (metaDataExtractor.extractStringValue(forKey: MetadataKeys.headStartPost) == Values.headStartValue)

// Product Bundle properties
// Uses failsafe decoding because non-bundle product types can return unexpected value types.
Expand All @@ -561,7 +563,7 @@ public struct Product: Codable, GeneratedCopiable, Equatable, GeneratedFakeable
let compositeComponents = try container.decodeIfPresent([ProductCompositeComponent].self, forKey: .compositeComponents) ?? []

// Subscription properties
let subscription = try? metaDataExtractor?.extractProductSubscription()
let subscription = try? metaDataExtractor.extractProductSubscription()

// Min/Max Quantities properties
let minAllowedQuantity = container.failsafeDecodeIfPresent(stringForKey: .minAllowedQuantity)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation
import WordPressShared
import NetworkingCore

/// Helper to extract specific data from inside `Product` metadata.
/// Sample Json:
Expand All @@ -23,18 +24,33 @@ import WordPressShared
///
internal struct ProductMetadataExtractor: Decodable {

private typealias DecodableDictionary = [String: AnyDecodable]
private typealias AnyDictionary = [String: Any?]

/// Internal metadata representation
///
private let metadata: [DecodableDictionary]
private let metadata: [MetaData]

/// Decode main metadata array as an untyped dictionary.
/// Initialize with already-decoded metadata array
///
init(metadata: [MetaData]) {
self.metadata = metadata
}

/// Decode main metadata supporting both array and object formats.
/// This expects to decode the raw metadata array/object, not a JSON object with a meta_data field.
///
init(from decoder: Decoder) throws {
let container = try decoder.singleValueContainer()
self.metadata = try container.decode([DecodableDictionary].self)

// Try to decode as array first (standard format)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was investigating why we need this duplicating logic, and I think the whole init(from decoder: Decoder) throws and Decodable from ProductMetadataExtractor can be removed. I couldn't find any code paths that rely on it. ProductMetadataExtractor is used only through init(metadata:) initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I'll check that.

However, I tried a lot last night to have a sane, non-duplicative approach, and failed hard. It was down to the subtle differences of what data source we were trying to map, and how decoded it already was.

Hopefully the cold light of day will let me delete more of this.

if let metaDataArray = try? container.decode([MetaData].self) {
self.metadata = metaDataArray
} else if let metaDataDict = try? container.decode([String: MetaData].self) {
// Try to decode as object keyed by index strings
self.metadata = Array(metaDataDict.values)
} else {
self.metadata = []
}
}

/// Searches product metadata for subscription data and converts it to a `ProductSubscription` if possible.
Expand Down Expand Up @@ -62,22 +78,25 @@ internal struct ProductMetadataExtractor: Decodable {

/// Filters product metadata using the provided prefix.
///
private func filterMetadata(with prefix: String) -> [DecodableDictionary] {
metadata.filter { object in
let objectKey = object["key"]?.value as? String ?? ""
return objectKey.hasPrefix(prefix)
}
private func filterMetadata(with prefix: String) -> [MetaData] {
metadata.filter { $0.key.hasPrefix(prefix) }
}

/// Parses provided metadata to return a dictionary with each metadata object's key and value.
///
private func getKeyValueDictionary(from metadata: [DecodableDictionary]) -> AnyDictionary {
metadata.reduce(AnyDictionary()) { (dict, object) in
var newDict = dict
let objectKey = object["key"]?.value as? String ?? ""
let objectValue = object["value"]?.value
newDict.updateValue(objectValue, forKey: objectKey)
return newDict
private func getKeyValueDictionary(from metadata: [MetaData]) -> AnyDictionary {
metadata.reduce(into: AnyDictionary()) { dict, object in
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

[nitpick] The reduce(into:) method is more efficient than the previous reduce() approach since it mutates the accumulator in-place rather than creating new dictionaries on each iteration.

Copilot uses AI. Check for mistakes.
// For JSON values, decode them to get the actual object
if object.value.isJson {
if let data = object.value.stringValue.data(using: .utf8),
let jsonObject = try? JSONSerialization.jsonObject(with: data, options: []) {
dict[object.key] = jsonObject
} else {
dict[object.key] = object.value.stringValue
}
} else {
dict[object.key] = object.value.stringValue
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public struct ProductVariation: Codable, GeneratedCopiable, Equatable, Generated
}
return false
})
]) ?? false
]) ?? false

// Even though WooCommerce Core returns Int or null values,
// some plugins alter the field value from Int to Decimal or String.
Expand Down Expand Up @@ -309,7 +309,9 @@ public struct ProductVariation: Codable, GeneratedCopiable, Equatable, Generated
let menuOrder = try container.decode(Int64.self, forKey: .menuOrder)

// Subscription settings for subscription variations
let subscription = try? container.decodeIfPresent(ProductMetadataExtractor.self, forKey: .metadata)?.extractProductSubscription()
let allMetaData = [MetaData].decodeFlexibly(from: container, forKey: .metadata)
let metaDataExtractor = ProductMetadataExtractor(metadata: allMetaData)
let subscription = try? metaDataExtractor.extractProductSubscription()

// Min/Max Quantities properties
let minAllowedQuantity = container.failsafeDecodeIfPresent(stringForKey: .minAllowedQuantity)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import Foundation

/// Extension to support flexible decoding of MetaData arrays
/// Handles both standard array format and object format keyed by index strings
extension Array where Element == MetaData {

/// Custom decoding from container that supports both array and dictionary formats
public static func decodeFlexibly<K>(from container: KeyedDecodingContainer<K>,
forKey key: KeyedDecodingContainer<K>.Key) -> [MetaData] {
// Try to decode as array first (standard format)
if let metaDataArray = try? container.decode([MetaData].self, forKey: key) {
return metaDataArray
}

// Try to decode as object keyed by index strings
if let metaDataDict = try? container.decode([String: MetaData].self, forKey: key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was checking if a plugin could produce [Int: MetaData].self, but it looks unlikely since using numbers as object keys goes against the JSON specs and json_encode() would ensure that the keys are strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'm wary of even doing this much workaround for plugins messing with fields that don't belong to them, to be honest... it seems like a short road to insanity trying to pair that with our type safe models.

It'll continue to fail gracefully even if the format is more broken than this.

return Array(metaDataDict.values)
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

Using Array(metaDataDict.values) loses the ordering information from the dictionary keys. Since the keys are string indices ('0', '1', '2'), the metadata should be sorted by these keys to preserve the original order. Consider sorting by converting keys to integers: return metaDataDict.sorted { Int($0.key) ?? 0 < Int($1.key) ?? 0 }.map { $0.value }

Suggested change
return Array(metaDataDict.values)
return metaDataDict.sorted { (lhs, rhs) in
(Int(lhs.key) ?? 0) < (Int(rhs.key) ?? 0)
}.map { $0.value }

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's true, the docs suggest:

When iterated over, values appear in this collection in the same order as they occur in the dictionary’s key-value pairs.

}

// Fallback to empty array
return []
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be worth logging a case where metadata decoding fails silently? It could help troubleshoot similar issues more easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}
5 changes: 4 additions & 1 deletion Modules/Sources/NetworkingCore/Model/Order.swift
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,10 @@ public struct Order: Decodable, Sendable, GeneratedCopiable, GeneratedFakeable {
// "payment_url" is only available on stores with version >= 6.4
let paymentURL = try container.decodeIfPresent(URL.self, forKey: .paymentURL)

let allOrderMetaData = try? container.decode([MetaData].self, forKey: .metadata)
let allOrderMetaData: [MetaData]? = {
let metadata = [MetaData].decodeFlexibly(from: container, forKey: .metadata)
return metadata.isEmpty ? nil : metadata
}()
var chargeID: String? = nil
chargeID = allOrderMetaData?.first(where: { $0.key == "_charge_id" })?.value.stringValue

Expand Down
130 changes: 130 additions & 0 deletions Modules/Tests/NetworkingTests/Mapper/MetaDataMapperTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,136 @@ final class MetaDataMapperTests: XCTestCase {
XCTAssertEqual(metadata[5], MetaData(metadataID: 6, key: "number_field", value: "42"))
XCTAssertEqual(metadata[6], MetaData(metadataID: 7, key: "empty_field", value: ""))
}

/// Tests that flexible metadata decoding works with array format in a realistic context
///
func test_flexible_metadata_decoding_array_format() throws {
// Given - JSON with metadata as array in a product-like structure
let jsonString = """
{
"id": 123,
"name": "Test Product",
"meta_data": [
{
"id": 1001,
"key": "custom_field_1",
"value": "value1"
},
{
"id": 1002,
"key": "_internal_field",
"value": "internal_value"
},
{
"id": 1003,
"key": "custom_field_2",
"value": "value2"
}
]
}
"""

struct TestProduct: Decodable {
let id: Int
let name: String
let metadata: [MetaData]

private enum CodingKeys: String, CodingKey {
case id, name
case metadata = "meta_data"
}

init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
self.id = try container.decode(Int.self, forKey: .id)
self.name = try container.decode(String.self, forKey: .name)

// Flexible decoding logic using helper
self.metadata = [MetaData].decodeFlexibly(from: container, forKey: .metadata)
}
}

let data = jsonString.data(using: .utf8)!
let decoder = JSONDecoder()

// When
let product = try decoder.decode(TestProduct.self, from: data)

// Then
XCTAssertEqual(product.metadata.count, 3) // All fields should be present
XCTAssertEqual(product.metadata[0].metadataID, 1001)
XCTAssertEqual(product.metadata[0].key, "custom_field_1")
XCTAssertEqual(product.metadata[0].value.stringValue, "value1")
XCTAssertEqual(product.metadata[1].metadataID, 1002)
XCTAssertEqual(product.metadata[1].key, "_internal_field")
XCTAssertEqual(product.metadata[1].value.stringValue, "internal_value")
XCTAssertEqual(product.metadata[2].metadataID, 1003)
XCTAssertEqual(product.metadata[2].key, "custom_field_2")
XCTAssertEqual(product.metadata[2].value.stringValue, "value2")
}

/// Tests that flexible metadata decoding works with dictionary format in a realistic context
///
func test_flexible_metadata_decoding_dictionary_format() throws {
// Given - JSON with metadata as object keyed by index strings in a product-like structure
let jsonString = """
{
"id": 456,
"name": "Test Product 2",
"meta_data": {
"0": {
"id": 2001,
"key": "dict_field_1",
"value": "dict_value1"
},
"1": {
"id": 2002,
"key": "_internal_dict_field",
"value": "internal_dict_value"
},
"2": {
"id": 2003,
"key": "dict_field_2",
"value": "dict_value2"
}
}
}
"""

struct TestProduct: Decodable {
let id: Int
let name: String
let metadata: [MetaData]

private enum CodingKeys: String, CodingKey {
case id, name
case metadata = "meta_data"
}

init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
self.id = try container.decode(Int.self, forKey: .id)
self.name = try container.decode(String.self, forKey: .name)

// Flexible decoding logic using helper
self.metadata = [MetaData].decodeFlexibly(from: container, forKey: .metadata)
}
}

let data = jsonString.data(using: .utf8)!
let decoder = JSONDecoder()

// When
let product = try decoder.decode(TestProduct.self, from: data)

// Then
XCTAssertEqual(product.metadata.count, 3) // All fields should be present
let fieldNames = Set(product.metadata.map { $0.key })
XCTAssertTrue(fieldNames.contains("dict_field_1"))
XCTAssertTrue(fieldNames.contains("dict_field_2"))
XCTAssertTrue(fieldNames.contains("_internal_dict_field"))
}

}

// MARK: - Test Helpers
Expand Down
Loading