Skip to content

Expose ParsedUrl.by_appending_query_pairs for query-aware REST URL resolution - #1549

Open
jkmassel wants to merge 3 commits into
trunkfrom
issue-1543
Open

Expose ParsedUrl.by_appending_query_pairs for query-aware REST URL resolution#1549
jkmassel wants to merge 3 commits into
trunkfrom
issue-1543

Conversation

@jkmassel

@jkmassel jkmassel commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #1543. GutenbergKit#579 consolidates six hand-rolled rest_route URL joiners onto wprs's WpOrgSiteApiUrlResolver. The resolver already does the rest_route-aware path join, but consumers couldn't attach endpoint query parameters (context=edit, status=active, exclude=core,gutenberg) to a resolved URL without re-implementing the ?& merge — resolve() takes no query and ParsedUrl's query methods weren't exported across the UniFFI boundary.

This adds a general, composable URL primitive: ParsedUrl.by_appending_query_pairs. The consumer flow becomes resolver.resolve(ns, segments).byAppendingQueryPairs([...]), correct on both API-root forms. The change is additive and non-breaking — resolve(), route_path, and the ApiUrlResolver trait are unchanged (the trait is with_foreign, so adding a required method would break Swift/Kotlin implementors).

Changes

  • wp_api/src/parsed_url.rs: New QueryPair { name, value } uniffi::Record and an exported ParsedUrl.by_appending_query_pairs(pairs:). It appends form-urlencoded pairs while preserving any existing query, working uniformly on path roots (…/wp/v2/themes?context=edit&status=active) and ?rest_route= query roots (keeps the existing rest_route value, appends &context=edit&status=active). Pairs are order-stable and duplicate keys are kept. Empty pairs returns the URL unchanged via an early return that sidesteps the url crate's query_pairs_mut() pushing a ? on its first call.
  • native/swift/Sources/wordpress-api/Exports.swift: Re-export QueryPair from the public WordPressAPI module — the generated type lives in WordPressAPIInternal, so without the typealias Swift consumers can't name or construct it.
  • wp_api/src/request/endpoint/plain_permalinks_url_tests.rs (new): A golden URL table that drives all 45 self-hosted endpoint URL builders through a ?rest_route=-seeded WpOrgSiteApiUrlResolver and asserts the exact plain-permalinks URL each one produces — the counterpart to the per-endpoint wp-json goldens in each *_endpoint.rs. It exercises the rest_route join across every endpoint shape (embedded-slash paths like plugins/block-renderer, all three self-hosted namespaces, and the API root). This is complementary coverage: it locks the shipped resolve() path (Self-hosted login fails on plain-permalink sites: the ?rest_route= API root is path-extended into non-routing URLs #1366) on plain permalinks, not the new by_appending_query_pairs surface.
  • CHANGELOG.md: Unreleased → Added entry, plus an Internal note for the endpoint golden table.

Test plan

Encoding deliberately matches by_extending_rest_api_path: exclude=core,gutenberg serializes to exclude=core%2Cgutenberg (form-urlencoded) — byte-different from GutenbergKit's current literal-comma output but functionally equivalent, and WordPress decodes it.

  • cargo test -p wp_api --lib — 30+ new rstest cases in parsed_url.rs (both root forms, trailing-slash and none, &debug=1 preservation, reserved/space/unicode/+// encoding, empty-name and empty-value, order-stable, duplicate keys, fragment preservation, non-mutation, chainability, resolve→append composition) plus a resolver-level composition test in endpoint.rs.
  • cargo clippy --tests --all-features -- -D warnings clean; cargo fmt --all -- --check clean.
  • Kotlin: UniFFI bindings regenerate; compileKotlin + compileIntegrationTestKotlin + detekt pass. Generated fun byAppendingQueryPairs(pairs: List<QueryPair>): ParsedUrl and data class QueryPair(name, value) confirmed; smoke test in ParsedUrlTest.kt.
  • Swift: make xcframework-only-macos + swift test — 79 tests pass, including the new ParsedUrlTests (path root, ?rest_route= root, %2C encoding, empty-unchanged) round-tripping the QueryPair record through the generated bindings; swift-format --strict clean.
  • cargo test -p wp_api --lib request::endpoint::plain_permalinks_url_tests — 45 endpoints, one golden ?rest_route= URL each, all green. Expected strings were authored by an independent form-urlencoder (not the resolver), so the table is a genuine regression lock rather than a tautology. Every endpoint resolves correctly on plain permalinks — no surprises.
  • cargo test -p wp_api --lib request::endpoint::index_self_href_url_tests — server-sourced full-URL oracle: for the 22 endpoints WordPress advertises as non-parameterized routes, our resolver reproduces the server's own published URL (_links.self.href, captured in the real-site index fixture test-data/api-details/test-case-03.json), byte-for-byte, across all three namespaces. 22/22 green. This guards the golden table's accuracy against an independent source rather than my own encoder.
  • Integration (make test-server): test_login_plain_permalinks_mut — both tests pass against a live Dockerized WordPress. The REST index publishes no URL for parameterized routes, so the new fetch_object_by_id_on_plain_permalinks_site fetches a real object through /wp/v2/users/<id> over ?rest_route= on a plain-permalink site and asserts the id round-trips; the existing /users/me check was refactored onto a shared helper and still passes.

Related issues

Changelog

  • I've added an entry to CHANGELOG.md under ## [Unreleased], using the Keep a Changelog categories.

@wpmobilebot

wpmobilebot commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

XCFramework Build

This PR's XCFramework is available for testing. Add to your Package.swift:

.package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1549")

Built from 0ed486f

Add a rest_route-aware way to attach endpoint query parameters to a
resolved REST URL from Swift and Kotlin, closing the gap that forced
consumers to re-implement the ?->& merge. Introduces a QueryPair record
and an additive ParsedUrl.by_appending_query_pairs method that appends
form-urlencoded pairs, preserving any existing query on both path roots
and ?rest_route= query roots.

Fixes #1543
Drive each of the 45 self-hosted endpoint URL builders through a
rest_route-seeded WpOrgSiteApiUrlResolver and assert the exact
?rest_route= URL, the plain-permalinks counterpart to the per-endpoint
wp-json golden assertions. Covers all three self-hosted namespaces plus
embedded-slash paths (plugins, block-renderer) and the api root.

This validates URL construction across the full endpoint surface on
plain-permalink sites; it complements test_login_plain_permalinks_mut,
which proves one endpoint end-to-end against a live server.
Back the plain_permalinks_url_tests golden table with two independent
checks so its hard-coded values can't silently encode a resolver bug:

- index_self_href_url_tests: for the 22 endpoints WordPress advertises as
  non-parameterized routes, assert our resolver reproduces the server's own
  published URL (the _links.self.href captured in the real-site index
  fixture test-data/api-details/test-case-03.json). Independent full-URL
  oracle, no server needed.
- test_login_plain_permalinks_mut: add a second serial test that fetches a
  real object through a parameterized route (/wp/v2/users/<id>) over
  ?rest_route= — the case the index publishes no URL for — proving ID-bearing
  endpoints round-trip end-to-end on a plain-permalink site. Factored the
  discover-and-build-admin-client setup into a shared helper.
@jkmassel
jkmassel marked this pull request as ready for review August 11, 2026 17:42
@jkmassel
jkmassel requested review from dcalhoun and oguzkocer August 11, 2026 17:42

@dcalhoun dcalhoun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. I believe there is value in receiving @oguzkocer's feedback on the implementation as well.

A couple of clarifying questions from reviewing this...

1. Sequencing work

Does not block fix: build REST URLs for sites using plain permalinks #573 or WordPress-iOS#25859 — both are correct, tested, user-facing fixes. This supersedes them structurally, later.

As quoted above, wordpress-mobile/GutenbergKit#579 describes proceeding with the original, currently unapproved implementations. Do we still want to merge those as-is or instead merge/integrate this PR? I'm fine with either approach, just mitigating miscommunication.

2. Preloading cache keys

It seems GBK needs to know the canonical route key for a request, separate from the request URL. This is because EditorPreloadList and EditorURLCache both key on canonical WP paths, which is a contract with api-fetch's preloading middleware. Should constructing the canonical route key be the responsibility of wprs or the consumer?

Further details from Claude's review:

Claude's review

These changes close the gap it targets, and the URLs it produces preload correctly — I checked all three root forms through api-fetch's normalizePath + rest_route unwrap, and %2F/%2C encoding and param order are all normalized away. No change needed here for that.

One question before GBK adopts it: does wprs intend to own canonical route keys?

wordpress-mobile/GutenbergKit#579 has route_path supplying preload keys, but it takes no query. Matching is a plain dict lookup on the normalized path, and normalizePath early-returns when there's no query — so a query-less key can't match a query-bearing request. GBK's keys are /wp/v2/themes?context=edit&status=active and /wp/v2/types/post?context=edit; core-data sends context=edit for themes (entities.js:199), so the query is on both sides. Verified: route_path-shaped key → miss; full resolved URL as key → miss (normalization doesn't strip the origin). A miss isn't an error, just a silent fallback to network on exactly the sites preloading helps most.

Two workable answers:

  1. wprs owns keys — a route_path variant taking query pairs.
  2. Consumers own keys — fine, but worth a doc note, since GBK's resolver protocol then needs a routeKey separate from resolve from day one.

Either way: route_path for keys, resolve for requests. Preference?

#[case::plain_permalinks_rest_route_form(
"https://example.com/index.php?rest_route=/",
"https://example.com/index.php?rest_route=%2Fwp%2Fv2%2Fthemes&context=edit&exclude=core%2Cgutenberg"
)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we add a third case here for the bare ?rest_route= root (no trailing slash)?

Trailing-slash normalization is already covered on by_extending_rest_api_path (parsed_url.rs:287), and this method doesn't touch the route value. So, the untested bit is the interaction through the composed path, which I believe is what GBK will call:

#[case::plain_permalinks_rest_route_no_trailing_slash(
    "https://example.com/index.php?rest_route=",
    "https://example.com/index.php?rest_route=%2Fwp%2Fv2%2Fthemes&context=edit&exclude=core%2Cgutenberg"
)]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose query-aware REST URL resolution (ParsedUrl.by_appending_query_pairs)

3 participants