Plugins endpoint by oguzkocer · Pull Request #90 · Automattic/wordpress-rs

oguzkocer · 2024-05-16T19:12:25Z

Implements the /plugins endpoint variants - except for filtering. It closely follows the /users implementation, so there isn't too much that's completely new.

Adds backup-wp-content-plugins, restore-wp-content-plugins & delete-wp-plugins-backup Make tasks. For most of our integration tests, restoring the DB to a previous state will be enough. However, with /plugins the filesystem is also involved, so we need new helpers just for /plugins integration tests.
Adds test_helpers::run_and_restore_wp_content_plugins function. This is similar to wp_db::run_and_restore function, providing a convenient way to handle the restoration of wp-contents/plugins folder from a backup using the Make task.
Adds several integration tests, but the create, delete & update plugin tests are a little fragile at the moment. They require the test site to have hello-dolly and classic-editor plugins to be installed. (one being active, the other inactive) I am considering adding specific Make tasks to ensure a specific state before running these tests, but I'd like to split that into its own PR - if I do implement it.

There has been several annoying issues in this implementation:

Plugin Slugs

The plugin slugs returned from the /plugins endpoint will be in foo/bar format, for example hello-dolly/hello & classic-editor/classic-editor
The plugin slugs in WordPress.org directory is in foo format: hello-dolly & classic-editor

These discrepancies could become a headache for the consumers, so I think we might want to introduce PluginSlug & WpOrgPluginSlug types to clearly differentiate them in our API. I'd like to do this in its own PR.

Furthermore, most of the endpoints expect the plugin slug to be part of the url and not the query pairs. So, for example with classic-editor, the proper retrieve request url would be /plugins/classic-editor/classic-editor as opposed to something like /plugins?slug=classic-editor%2Fclassic-editor where the / character is encoded. To handle this, the plugins_url_with_slug helper function will first split the given slug using / as the separator, and then use it to extend the url.

Incorrect(?) documentation

author seems to return a String as opposed to a JSONObject.

Both update & delete endpoints' documentation claims that the context argument will determine which fields will be in the response, however it seems it always returns the fields for edit. (which contains all the fields) I've tried this by sending the context parameter, both as a query pair as well as part of the JSON body of the POST request and in both cases, I got all the fields back. I might have made a mistake in my testing, but I have some confidence that this is correct, because while testing I accidentally sent context: View in the JSON body and got an error back saying context is not one of edit, embed & view values.

To avoid confusion, I've decided not to include the context parameter for these endpoints.

Note that one small discrepancy with plugins implementation, compared to /users is that I've decided to include specific parsers for create & update endpoints - as opposed to using the parser for retrieve response. I'd like to go back and change /users as well to follow this pattern to make it easier to find the correct parser for each response. This will add a few "unnecessary" functions, but I think it's worth it.

To Test

make test-server && make dump-mysql && make backup-wp-content-plugins
cargo test --test '*' -- --nocapture --test-threads 1

What Next

Refactoring some common implementations between users & plugins
Reorganization of some of the Rust modules and especially the WPApiHelper
Filtering for plugins
Error handling for plugins
More integration tests for plugins - and maybe make them less fragile

crazytonyli · 2024-05-17T03:34:42Z

Both update & delete endpoints' documentation claims that the context argument will determine which fields will be in the response, however it seems it always returns the fields for edit.
[...]
To avoid confusion, I've decided not to include the context parameter for these endpoints.

My understanding is all endpoints' POST requests only use edit context. I agree it makes sense to not include context parameter for them, which is the case for /users endpoints implementation too.

crazytonyli

It'd be good to address the potential issue of parsing a unknown PluginStatus.

crazytonyli · 2024-05-17T03:28:15Z

wp_networking/tests/test_plugins_immut.rs

+        plugin_slug,
+        parsed_response
+    );
+}


author seems to return a String as opposed to a JSONObject.

What do you think about adding tests for the author property, so that we can be sure that it's indeed a documentation error (rather than a breaking change in the API)?

The current tests should already cover this as it retrieving plugins would fail with ParsingError if we got a json object instead of the expected String type.

Happy to add more tests if you have anything specific in mind.

Any chance you were thinking that author field was part of a {foo}Params} type, rather than a return type like the SparsePlugin? 😅 That's not the case, but if it was, I agree it would have been good to add a test that utilized that parameter.

I was thinking adding an explicit check which asserts author is a certain string, not even not nil.

crazytonyli · 2024-05-17T03:40:43Z

wp_api/src/endpoint/plugins_endpoint.rs

+        self.api_base_url
+            // The '/' character has to be preserved and not get encoded
+            .by_extending(["plugins"].into_iter().chain(plugin.split('/')))
+    }


There was an issue in iOS app where the app crashed due to unexpected characters in plugin slug. See wordpress-mobile/WordPressKit-iOS#767.

I couldn't reproduce the crash, because I can't find a real-world plugin that has a non-ascii slug. However, I think it's still worthy adding some unit tests to ensure a correct URL is constructed for strange plugin slugs (i.e. has % or non-ascii charaectrs). What do you think?

I've added a few more tests for this in b189438. It'd be really good if we can find a real world case for it though.

Maybe something like 4317929?

crazytonyli · 2024-05-17T03:46:41Z

wp_api/src/plugins.rs

+    Active,
+    #[serde(rename = "inactive")]
+    Inactive,
+}


If a new plugin status is added in a newer WordPress version, will that cause parser errors in this library, because status in contextual models (i.e. PluginWithViewContext) is non-optinal? What do you think adding a "unknown" case here?

@jkmassel could correct me if I am wrong, but I think one of the promises with WordPress.org REST API is that it'll remain backward compatible. So, I don't think we should make things harder for ourselves by taking breaking changes into account.

I don't like the idea of adding an Unknown variant for this - as that'll become a precedent and we'll have to start adding it everywhere else. I'd rather our library break in such an edge case - which I don't think will ever happen for PluginStatus to be honest.

one of the promises with WordPress.org REST API is that it'll remain backward compatible.

I believe adding another value (for example, "status": "activating") to an existing property is still "backward compatible". But our library won't be able to parse that response, which introduces issues to the consumer apps.

I'd rather our library break in such an edge case - which I don't think will ever happen for PluginStatus to be honest.

I just checked the source code. I don't know how to reproduce such a case, but I believe the code says status can also be network-active. https://github.com/WordPress/WordPress/blob/6.5.3/wp-includes/rest-api/endpoints/class-wp-rest-plugins-controller.php#L898

That's for multisites, and although we don't plan to directly support them, I see your point.

I still don't like the idea of building separate fallbacks for every enum we add. So, I was thinking maybe we should generalize the Recognized / Unrecognized approach that we used for errors - hopefully in a little bit better way, so we can use it everywhere?

Do you mind creating an issue to track this issue of parsing the status property, if it's not going to be addressed in this PR?

crazytonyli · 2024-05-17T03:53:36Z

wp_api/src/lib.rs

+        }
+    }
+
+    pub fn retrieve_plugin_request(&self, context: WPContext, plugin: &str) -> WPNetworkRequest {


You mentioned introducing PluginSlug & WpOrgPluginSlug types. Will the plugin: &str be replaced by one of those types?

Yeap, this is one of the cases where it's not clear which type of slug it should take. (It's the PluginSlug in this example, just as a reference)

oguzkocer · 2024-05-17T04:20:53Z

Thanks for the review @crazytonyli. I added some more unit tests as you suggested and replied to your other comments. Ready for another look! 🙇‍♂️

oguzkocer · 2024-05-21T16:31:00Z

wp_api/src/lib.rs

+    pub fn update_plugin_request(
+        &self,
+        plugin: &str,
+        params: PluginUpdateParams,


This should be:

Suggested change

params: PluginUpdateParams,

params: &PluginUpdateParams,

It's addressed in a later PR, but I won't include it here to avoid the extra merge conflict. (as I have multiple PRs building on one another)

crazytonyli

Approving to unblock.

crazytonyli · 2024-05-21T20:59:23Z

wp_api/src/plugins.rs

+    Active,
+    #[serde(rename = "inactive")]
+    Inactive,
+}


Do you mind creating an issue to track this issue of parsing the status property, if it's not going to be addressed in this PR?

oguzkocer · 2024-05-21T22:01:11Z

Thanks @crazytonyli! 🙇‍♂️

oguzkocer added 22 commits May 15, 2024 20:33

Adds SparsePlugin

c83dfe0

Adds plugins list endpoint

79ce094

Implement list plugins request and its parsers

5dde634

Replace plugin parsers with add_uniffi_exported_parser

1cd6788

Replace plugin_list_params! with generate! macro

c5f6046

Use extracted test helpers for plugins

b1214fb

Implement create plugin request

fd951bb

Implement run_and_restore_wp_content_plugins

514ad74

Implements retrieve plugin

8461d81

Fix the retrieve plugin endpoint and its return type

be2028e

A temporary helper bash script for fetching plugins

887c458

Disable restoring wp-content plugins from plugins_mut tests

46b9a8a

Add a separate parser for create plugin response

c2bd1c4

Fix retrieve plugins endpoint

20af86f

Implement update plugin request

71d6f67

Add an integration test for update plugin

6f51a73

Implement delete plugin request

259d009

Improve unit & immut integration tests for plugins

817a088

Fix backup/restoration of wp-content/plugins for integration tests

2afc5af

Remove temporary scripts used to test plugins

ef837fb

Ignore failures for delete-wp-plugins-backup make task

482d547

Remove unused PluginAuthor

aa73608

oguzkocer added Rust Endpoint Testing labels May 16, 2024

oguzkocer added this to the 0.1 milestone May 16, 2024

oguzkocer marked this pull request as ready for review May 16, 2024 19:23

oguzkocer requested review from crazytonyli and jkmassel May 16, 2024 19:23

crazytonyli requested changes May 17, 2024

View reviewed changes

Add url encoding related unit tests for plugins endpoint

b189438

oguzkocer requested a review from crazytonyli May 17, 2024 04:20

Merge remote-tracking branch 'origin/trunk' into plugins

6103231

oguzkocer mentioned this pull request May 20, 2024

Adds PluginSlug & PluginWpOrgDirectorySlug #91

Merged

Add an assertion for plugin author for retrieve plugin test

4317929

oguzkocer commented May 21, 2024

View reviewed changes

crazytonyli approved these changes May 21, 2024

View reviewed changes

oguzkocer mentioned this pull request May 21, 2024

Fallback for response enums #95

Open

oguzkocer merged commit 0722c3f into trunk May 21, 2024

oguzkocer deleted the plugins branch May 21, 2024 22:01

Conversation

oguzkocer commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli commented May 17, 2024

Uh oh!

crazytonyli 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

oguzkocer commented May 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer commented May 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oguzkocer commented May 16, 2024 •

edited

Loading

crazytonyli May 20, 2024 •

edited

Loading