Skip to content
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

[Custom Fields] Differentiate between raw JSON values and String values that contain valid JSON #14292

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Nov 1, 2024

Closes: #14271

@hafizrahman @selanthiraiyan not urgent, please review when you have some free time.

Description

As detailed in the above issue, the current implementation in iOS doesn't allow to differentiate between JSON values and String values that contain valid JSON, and this causes a difference with how Android and Core behaves.
This PR updates the MetaData parsing to expose information on whether the value is a String or a Json using an enum, and we store this information also when caching: the logic here is simple, we store the information as it's in the response, Strings will be stored with their quotes, and JSON values will be stored as they are without quotes. (the implementation is similar to what we did in Android, but simpler here)

While the number of affected files may seem big, the changes are quite simple actually:

  1. Whenever the value is read, we read it using the property stringValue, this returns the same content as before.
  2. When we save MetaData to storage, we use the property rawValue, this makes sure that String values are stored with their quotes.

Note: While the impact of these changes could be small for this specific feature, I think it could have some benefits in the future, for example if we move Subscriptions or Addons to use the MetaData class directly, then we'll have to make sure we can use JSON data with it.

Steps to reproduce

  1. Open wp-admin and add a custom field with the following content: {"key":"value"}
  2. Use the API to add a true JSON field like this for example:
POST {site}/wc/v3/orders/{orderId}

{
    "meta_data": [
        {
            "key": "json_object",
            "value": {
                "Key": "Value"
            }
        },
        {
            "key": "json_array",
            "value": [
                {
                    "Key": "Value"
                }
            ]
        }
    ]
}
  1. Open the given product or order where the changes were made.

Testing information

  • Confirm that the value added in wp-admin is editable in the app.
  • Confirm that the value added using the API call is not editable.
  • Smoke test the feature as a whole.

Screenshots

Simulator.Screen.Recording.-.iPhone.15.-.2024-11-01.at.18.21.44.mp4

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

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@hichamboushaba hichamboushaba added type: task An internally driven task. feature: order details Related to order details. feature: add/edit products Related to adding or editing products. labels Nov 1, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 1, 2024

1 Error
🚫 PR is not assigned to a milestone.

Generated by 🚫 Danger

country: CopiableProp<String> = .copy,
country: NullableCopiableProp<String> = .copy,
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the file wasn't re-generated after some changes in a previous PR, that's why rage generate updated it.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 1, 2024

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

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14292-4f82a10
Version20.9
Bundle IDcom.automattic.alpha.woocommerce
Commit4f82a10
App Center BuildWooCommerce - Prototype Builds #11424
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Also ensures we can't bypass the validation by instantiating the enum case directly.
@hichamboushaba hichamboushaba force-pushed the issue/14271-better-json-detection branch from 432aa36 to 4f82a10 Compare November 2, 2024 23:41
return String(data: data, encoding: .utf8) ?? ""
} else {
return "\(value)"
public struct MetaDataValue: Codable, Equatable, Sendable {
Copy link
Member Author

Choose a reason for hiding this comment

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

We expose a struct instead of exposing the enum directly just to make sure we have some validation, and we no consumer can bypass it by instantiating the enum directly, ie: .json("invalid json"), this is actually because as far as I found, Swift doesn't allow to have private initializer for the enum cases (unlike Kotlin with its sealed interface feature).

private extension MetaDataValue {
private enum ValueHolder: Codable, Equatable {
case string(_ value: String)
case json(_ json: String)
Copy link
Member Author

Choose a reason for hiding this comment

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

I played with a different approach here, instead of making the backing data a String, we can have it as Codable directly, I don't have a strong opinion on which approach is best, so I kept the String based implementation.

public struct MetaDataValue: Codable, Equatable, Sendable {
    private let valueHolder: ValueHolder

    public var stringValue: String {
        valueHolder.stringValue
    }
    public var rawValue: String {
        valueHolder.rawValue
    }

    public var isJson: Bool {
        switch valueHolder {
        case .string:
            return false
        case .json:
            return true
        }
    }

    public init(rawValue: String) {
        let data = Data(rawValue.utf8)
        if let jsonObject = try? JSONDecoder().decode(AnyCodable.self, from: data) {
            valueHolder = try! ValueHolder(from: jsonObject.value)
        } else {
            valueHolder = .string(rawValue.removingPrefix("\"").removingSuffix("\""))
        }
    }

    public init(from decoder: Decoder) throws {
        valueHolder = try ValueHolder(from: decoder)
    }

    public func encode(to encoder: Encoder) throws {
        try valueHolder.encode(to: encoder)
    }
}

private extension MetaDataValue {
    private enum ValueHolder: Codable, Equatable {
        case string(_ value: String)
        case json(_ json: Codable)

        var stringValue: String {
            switch self {
            case .string(let value):
                return value
            case .json(let json):
                return json.asJSONString()
            }
        }

        var rawValue: String {
            switch self {
            case .string(let value):
                return "\"\(value)\""
            case .json(let json):
                return json.asJSONString()
            }
        }

        init(from decoder: any Decoder) throws {
            let container = try decoder.singleValueContainer()
            let value = try container.decode(AnyCodable.self)
            try self.init(from: value.value)
        }

        init(from object: Any) throws {
            switch object {
            case let value as String:
                self = .string(value)
            case is Bool:
                self = .string(String(describing: object))
            case is any Numeric:
                self = .string(String(describing: object))
            default:
                self = .json(AnyCodable(object))
            }
        }

        func encode(to encoder: Encoder) throws {
            switch self {
            case .string(let value):
                try value.encode(to: encoder)
            case .json(let json):
                try json.encode(to: encoder)
            }
        }

        static func == (lhs: ValueHolder, rhs: ValueHolder) -> Bool {
            return lhs.stringValue == rhs.stringValue
        }
    }
}

private extension Encodable {
    func asJSONString() -> String {
        if let data = try? JSONEncoder().encode(self) {
            return String(data: data, encoding: .utf8) ?? ""
        }
        return ""
    }
}

Base automatically changed from simplify-custom-fields-models to trunk November 4, 2024 14:32
@hichamboushaba hichamboushaba marked this pull request as ready for review November 5, 2024 12:47
@hafizrahman hafizrahman self-assigned this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: add/edit products Related to adding or editing products. feature: order details Related to order details. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Custom Fields] Allow differentiating between JSON fields and String fields that contain JSON content
4 participants