Rewrite `WordPressOrgRestApi` #724

crazytonyli · 2024-02-14T23:59:36Z

Note

This PR introduces breaking changes, and unfortunately can't be split into smaller ones (they won't be ship-able individually).

I recommend you reviewing this PR commit by commit. However, when it comes to the commit which rewrites WordPressOrgRestApi, I'd suggest you ignore the commit diff and review the new implementation directly.

Description

Unlike the refactor to WP.com and XMLRPC API client, this PR rewrites WordPressOrgRestApi which deals with .org REST API.

About .org REST API

Every WordPress site ships with .org REST API, which is accessible via <site-url>/wp-json/<route>. The same .org REST API is also available on WordPress.com, but under a different URL: https://public-api.wordpress.com/wp/v2/site/<site-id>/<route>.

Current implementation

Currently, .org REST API (represented by the WordPressRestApi protocol) is implemented by two classes: WordPressOrgRestApi for self-hosted sites and WordPressComRestApi for WP.com sites.

As mentioned before, the same backend implementation is exposed by different URL path. The WordPressRestApi protocol has a special requestPath function which the callers have to remember to use (here is an example) before sending HTTP requests. IMO, forgetting to call that special function is an easy mistake to make during development.

Also, the WP.com implementation of .org REST API reuses the error handling code in rest/vx.x. If I understand it correctly, the rest/ endpoints are entirely different to the endpoints under wp/v2 and there is no guarantee errors are reported in the same HTTP response format.

New implementation

The PR rewrites WordPressOrgRestApi to be one single implementation which supports self-hosted sites and WordPress.com sites.

The problem with different path now becomes an internal implementation detail which callers don't need to worry about (here is an example), mostly. When we need to support more .org REST API, WordPressKit crashes with an error telling developers the exact changes they need to make to add such support.

The error handling is now the same for both self-hosted sites and WP.com sites, which makes sense because they are backed by the same backend implementation.

Testing Details

I have added/updated unit tests in this PR. You can also use wordpress-mobile/WordPress-iOS#22612 to do some manual tests on the app.

Please check here if your pull request includes additional test coverage.
I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

mokagio

IMO, forgetting to call that special function is an easy mistake to make during development.

As a forgetful developer, I can attest to that. Definitely best to remove the need for people to have to remember to do things and have the code and design either do that for them or force them to do them.

The problem with different path now becomes an internal implementation detail which callers don't need to worry about

Perfect 👌

None of my comments are blocking.

mokagio · 2024-02-16T02:18:42Z

WordPressKit/BlockEditorSettingsServiceRemote.swift

-    let remoteAPI: WordPressRestApi
-    public init(remoteAPI: WordPressRestApi) {
+    let remoteAPI: WordPressOrgRestApi
+    public init(remoteAPI: WordPressOrgRestApi) {


Here's a minor nitpick / curiosity. I have a thing for type names using acronyms, like URL instead of Url or JSONDecoder instead of JsonDecoder.

How would this look like if it were WordPressORGRESTAPI? Too many uppercase letters? 😳

I'd probably use Org instead of ORG. But this PR simply reuses the existing name 😄 .

mokagio · 2024-02-16T02:23:12Z

WordPressKit/HTTPRequestBuilder.swift

+    /// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`. This function
+    /// can be used to support those use cases.


Formatting nitpick

Suggested change

/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`. This function

/// can be used to support those use cases.

/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`.

/// This function can be used to support those use cases.

mokagio · 2024-02-16T02:24:29Z

WordPressKit/WordPressComRestApi.swift

    private func requestBuilder(URLString: String) throws -> HTTPRequestBuilder {
-        guard let url = URL(string: URLString, relativeTo: baseURL) else {
-            throw URLError(.badURL)
-        }
-
-        var builder = HTTPRequestBuilder(url: url)
+        var builder = HTTPRequestBuilder(url: baseURL)
+            .appendURLString(URLString)


Out of scope nitpick: value names should start with a lowercase word.

URLString -> urlString

mokagio · 2024-02-16T02:28:40Z

WordPressKit/WordPressOrgRestApi.swift

+        public var loginURL: URL
+        public var username: String
+        public var password: Secret<String>
+
+        public var adminURL: URL


Do these need to be var?

Suggested change

public var loginURL: URL

public var username: String

public var password: Secret<String>

public var adminURL: URL

public let loginURL: URL

public let username: String

public let password: Secret<String>

public let adminURL: URL

mokagio · 2024-02-16T02:31:50Z

WordPressKit/WordPressOrgRestApi.swift

+    public func get<Success: Decodable>(
+        path: String,
+        parameters: [String: Any]? = nil,
+        jsonDecoder: JSONDecoder = JSONDecoder(),
+        type: Success.Type = Success.self
+    ) async -> WordPressAPIResult<Success, WordPressOrgRestApiError> {
+        await perform(.get, path: path, parameters: parameters, jsonDecoder: jsonDecoder, type: type)
+    }


Inspired by try JSONDecoder().decode(Resource.self, from: jsonData), what do you think of moving type at the start?

get(Post.type, path: "/post", parameters: nil)

I feel like the decoding parameters are "secondary": The "GET /api/path" part is the main one. Plus, they have default values thus should be place at the end?

mokagio · 2024-02-16T03:02:33Z

WordPressKit/Plugin Management/SelfHostedPluginManagementClient.swift

-                    failure(error)
-            }
+        Task { @MainActor in
+            await remote.perform(.put, path: path, parameters: parameters, type: AnyResponse.self)


Have you considered adding methods for PUT and other HTTP verbs, for consistency with the POST and GET implementations?

Also, the AnyResponse type and related .map { _ in } took me a bit to understand.

I read them as a way to deal with an API which returns no data, or for which we don't care about the data.

What do you think of pushing this knowledge down at the WordPressOrgRestApi level?

We could have a perform implementation that returns WordPressAPIResult<Void, WordPressOrgRestApiError>, allowing us to segregate the AnyResponse + .map { _ in } "trick" lower into the implementation layer.

If instead it was implemented at this level because it's a one-off kind of thing, I'd still suggest centralizing the implementation into a fileprivate extension WordPressOrgRestApi

Have you considered adding methods for PUT and other HTTP verbs, for consistency with the POST and GET implementations?

The post and get functions are convenience functions because the majority of HTTP requests uses these two methods. I didn't add convenience functions for other methods simply because they are not used by many places (maybe only one or two places?).

What do you think of pushing this knowledge down at the WordPressOrgRestApi level?

IMO, It's kind of strange to ignore the response. Adding such support to WordPressOrgRestApi feels like we are encouraging it, but I don't think we should... Also, I believe this is the only place that ignores HTTP API response. I don't want to complicate WordPressOrgRestApi too much for this one case.

mokagio · 2024-02-16T03:09:55Z

WordPressKit/Plugin Management/SelfHostedPluginManagementClient.swift

+        case name = "name"
+        case author = "author"
+        case version = "version"
+        case pluginURI = "plugin_uri"


There's always one that gets in the way of the automatic key generation 😅

mokagio · 2024-02-16T03:11:51Z

WordPressKitTests/SelfHostedPluginManagementClientTests.swift

+            return HTTPStubsResponse(error: URLError(URLError.Code.networkConnectionLost))
+        }
+    }
+


I believe we should also remove the stubs?

Suggested change

override func tearDown() {

super.tearDown()

HTTPStubs.removeAllStubs()

}

Good catch! I always forget about it. Addressed in c44bb13.

mokagio · 2024-02-16T03:15:44Z

WordPressKit/BlockEditorSettingsServiceRemote.swift

+    func fetchBlockEditorSettings(completion: @escaping BlockEditorSettingsCompletionHandler) {
+        Task { @MainActor in
+            let result = await self.remoteAPI.get(path: "/wp-block-editor/v1/settings", parameters: ["context": "mobile"], type: RemoteBlockEditorSettings.self)
+                .map { settings -> RemoteBlockEditorSettings? in settings }


This might be a change for later on, but it seems like the only reason the RemoteBlockEditorSettings the API call returns is wrapped in an Optional here is because that's the type the "legacy" BlockEditorSettingsCompletionHandler expects?

If so, and since we're making breaking changes, I wonder whether we could make the type in the completion typealias non-optional. It would remove this step, and I suspect also simplify caller code by removing the need to deal with Result and Optional

It's not just for fetchBlockEditorSettings's syntax level backward compatibility, but most importantly runtime behaviour backward compatibility.

/wp-block-editor/v1/settings endpoint returns a JSON that contains many properties. The fetchBlockEditorSettings function only cares about a subset of these properties. See the model type RemoteBlockEditorSettings, which contains mostly non-optional properties.

If the JSON response contains all the properties declared in the Swift type, this function of course should return a model instance.

However, if this API returns 200 with valid "block editor settings" JSON response, which does not contains all the non-optional proprieties declared in the RemoteBlockEditorSettings type, the function should return a nil, instead of a "decoding error".

That's the behaviour of the existing function. And the refactored implementation (mainly the line here and the code below that maps decoding error to a nil successful result) maintains the same runtime behaviour for backward compatibility.

mokagio · 2024-02-16T03:18:30Z

WordPressKitTests/BlockEditorSettingsServiceRemoteTests.swift

    func testFetchBlockEditorSettingsNotThemeJSON() {
        stub(condition: isHost("public-api.wordpress.com") && isPath("/wp-block-editor/v1/sites/1/settings") && containsQueryParams(["context": "mobile"])) { _ in
            fixture(filePath: OHPathForFile(self.blockSettingsNOTThemeJSONResponseFilename, Self.self)!, headers: ["Content-Type": "application/json"])
        }

        let waitExpectation = expectation(description: "Block Settings should be successfully fetched")
-        service.fetchBlockEditorSettings(forSiteID: siteID) { (response) in
+        service.fetchBlockEditorSettings { (response) in


I'd expect SwiftLint to complain about the () at some point. But, it should also be something it can autocorrect easily 🤷‍♂️

crazytonyli added 12 commits February 15, 2024 11:51

Add a new appendURLString function to HTTPRequestBuilder

f545297

Use the new appendURLString function in WordPressComRestApi

b01de44

Add a new .org REST API client

6f10cf4

WordPressOrgRestApi now works with URL that ends with wp-json

535f3d4

Refactor plugin management implementation

26e7117

Add unit tests for plugin management

913a32e

Remove WordPressRestApi

52fb701

Use WordPressOrgRestApi in BlockEditorSettingsServiceRemote

660f8b2

Update test cases for WordPressOrgRestApi.GET

a7867c5

Move unit tests for wp/v2 path in WP.com

5d1f6ba

Remove WordPressRestApi conformance from WordPressComRestApi

4777462

Add a changelog

614daf8

crazytonyli requested a review from mokagio February 15, 2024 00:00

crazytonyli self-assigned this Feb 15, 2024

Remove unused Authenticator types

5e77bcf

mokagio approved these changes Feb 16, 2024

View reviewed changes

crazytonyli added 4 commits February 16, 2024 21:27

Format code comments

4f4b6b9

Use let instead of var

49f177f

Remove unnecessary parentheses

a65b40b

Remove all stubs upon tearing down test case

c44bb13

crazytonyli enabled auto-merge February 16, 2024 08:29

crazytonyli merged commit dfb2134 into trunk Feb 16, 2024
3 of 5 checks passed

crazytonyli deleted the wordpress-org-rest-api-v2 branch February 16, 2024 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `WordPressOrgRestApi` #724

Rewrite `WordPressOrgRestApi` #724

crazytonyli commented Feb 14, 2024 •

edited

Loading

mokagio left a comment

mokagio Feb 16, 2024

crazytonyli Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

crazytonyli Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

crazytonyli Feb 16, 2024

mokagio Feb 16, 2024

mokagio Feb 16, 2024

crazytonyli Feb 16, 2024

mokagio Feb 16, 2024

crazytonyli Feb 16, 2024

mokagio Feb 16, 2024

		/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`. This function
		/// can be used to support those use cases.

+    override func tearDown() {
+        super.tearDown()
+        HTTPStubs.removeAllStubs()
+    }

Rewrite WordPressOrgRestApi #724

Rewrite WordPressOrgRestApi #724

Conversation

crazytonyli commented Feb 14, 2024 • edited Loading

Description

About .org REST API

Current implementation

New implementation

Testing Details

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rewrite `WordPressOrgRestApi` #724

Rewrite `WordPressOrgRestApi` #724

crazytonyli commented Feb 14, 2024 •

edited

Loading