feat: WHATWG URL component setters and expanded conformance test coverage#54
Open
OmarAlJarrah wants to merge 19 commits into
Open
feat: WHATWG URL component setters and expanded conformance test coverage#54OmarAlJarrah wants to merge 19 commits into
OmarAlJarrah wants to merge 19 commits into
Conversation
Also fix the port state to end the digit run on any trailing character when a state override is in effect (WHATWG URL port state), not just an authority terminator, so the port setter accepts trailing junk (e.g. "8080stuff" -> port 8080) per spec.
Add a Go generator (setters.go) that turns the vendored WPT setters_tests.json corpus into SetterTestData.kt, following the same chunked-builder shape as the existing url/nfc-test generators. Wires up a generateSetterTestData Gradle task alongside the other codegen tasks. This is fixture generation only; the test that consumes SETTER_TEST_CASES comes next.
Adds UrlSetterConformanceTest, driving all 278 WPT setters_tests.json cases against the Url.with* setter family with a ratcheted known-failures baseline (kuri architecture: URL setters live behind UrlParser.parseWithOverride, a shared engine with the full parser). Getting this corpus green surfaced a dozen real bugs in the setter override machinery, all fixed here: - withPort/withProtocol pre-validated the raw value before the engine's own tab/newline stripping, rejecting inputs that are valid once stripped (e.g. "\t8080", "h\r\ntt\tps"). - schemeStartState/schemeState didn't fail cleanly under a setter override, so an invalid protocol value could restart parsing instead of resolving to a no-op. - Port-state override handling only recognized StateOverride.PORT, so a host/hostname setter whose value carried a port (e.g. "example.com:8080/stuff") kept parsing past the port into the path, corrupting it; and an empty digit run wrongly cleared an existing port to null. - Host-state was missing two WHATWG no-op guards: a colon under a hostname override must return before ever touching the host, and clearing a host to empty must be refused when the URL already has credentials or a port. - FILE_HOST always fell through into path-start regardless of override, and the override loop's Reconsume handling terminated before ever invoking the target state at EOF, together breaking file: URL host clearing and root-path synthesis under override. - Path state treated '?' as always opening a query, even under a pathname-setter override where it must stay literal path text. - withHash bypassed the shared engine and so missed the mandatory tab/newline strip. - Serializer's NORM-18 "/." anti-authority guard (needed only to keep a full href re-parseable) was leaking into the standalone pathname getter. One known, out-of-scope residual remains and is recorded in KNOWN_FAILURES with justification: HostParser accepts the ACE label "xn--" (empty Punycode suffix) instead of rejecting it, verified as a pre-existing IDNA-validity gap independent of the setter path. Also corrects a pre-existing StateOverrideParseTest expectation that predates this corpus and turns out to be wrong per the WPT ground truth: a hostname setter value containing a colon is a total no-op, not a partial host change.
Add a Go generator (percentencoding.go) that turns the vendored WPT percent-encoding.json corpus into PercentEncodingTestData.kt, following the same chunked-builder shape as the url/setters generators. Only the utf-8 expectation is emitted per case, since UTF-8 is kuri's sole charset; other charsets carried by the corpus (big5, shift_jis, ...) are out of scope. Wires up a generatePercentEncodingTestData Gradle task alongside the other codegen tasks. This is fixture generation only; the test that consumes PERCENT_ENCODING_TEST_CASES comes next.
Assert PercentCodec.encode(input, PercentEncodeSets.C0_CONTROL) reproduces every WPT percent-encoding.json utf-8 expectation, mirroring UrlConformanceTest's ratchet shape (untracked-regressions test plus a known-failures-equals-live-failures test). All 7 in-scope cases pass against the existing codec with no known failures.
…ty-less // paths
Url.newBuilder().build() threw IllegalArgumentException ("an authority-less
path cannot begin with '//'") for authority-less non-special URLs whose path
begins with an empty segment, e.g. non-spec:/.//path.
The builder's pre-fill constructor sourced its path from the encodedPath
getter, which correctly omits the NORM-18 /. anti-authority guard per the
WHATWG standalone-pathname algorithm. But the builder's recompose concatenates
that path directly onto scheme: with no authority, so the unguarded // re-parses
as a spurious authority and the parser rejects it. Pre-fill instead from the
guarded full-URL path serialization (serializeUrlPath's default), matching the
canonical href and leaving the pathname getter unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a WHATWG-conformant URL component setter API to
Url, and closes the tworemaining WPT conformance gaps versus reference implementations (URL setters and
percent-encoding). Also adds three test forms that go beyond replaying fixed
corpora: round-trip invariants, a
java.netdifferential, and a seeded fuzzer.Setter API (
Url)Nine immutable setters, each implementing the corresponding WHATWG URL setter
algorithm and returning a new
Url(or the receiver unchanged when the setter isa no-op):
withProtocol,withUsername,withPasswordwithHost,withHostname,withPort(String)withPathname,withSearch,withHashThese follow the WHATWG contract of being no-ops on invalid input rather than
throwing — the deliberate difference from
Url.Builder, which validates and throws.withPort(String)is a new overload alongside the existingwithPort(Int?)convenience.
Internally they reuse the existing parser state machine through a new state-override
entry point (
UrlParser.parseWithOverride), so the setters share the sameconformance-tested engine as full parsing rather than reimplementing component
parsing. The public getter surface is unchanged; new methods are captured in the JVM
and Android API snapshots.
Conformance suites
setters_tests.jsoncorpus (278 cases) against thenew setters via a getter adapter, ratcheted against a known-failures baseline like
the existing URL parsing suite. All cases pass except two, which are tracked: both
are the invalid ACE label
xn--being accepted by host parsing — a pre-existingIDNA host-validity gap that also affects full parsing and is out of scope here.
percent-encoding.jsoncorpus against thepercent codec (empty known-failures).
Both fixtures are generated from the vendored WPT corpora by new Go generators under
tools/internal/codegen/(generateSetterTestData,generatePercentEncodingTestData),consistent with the existing fixture generation.
Beyond fixed corpora
idempotent and
newBuilder().build()reproduces the originalhref. This caught areal defect: making the
pathnamegetter WHATWG-correct (excluding the/.pathprefix) had left the builder's recompose path without that prefix, so an
authority-less URL whose path begins with
//failed to rebuild. Fixed by having thebuilder pre-fill from the guarded full-URL path serialization; the getter stays
spec-correct.
java.netdifferential — compares parsing againstjava.net.URIon alignedinputs and documents the intentional WHATWG-vs-RFC-3986 divergences (host
case-folding, backslash-to-slash for special schemes) with a rationale per row.
and that any successful parse serializes idempotently.
Follow-ups (out of scope)
xn--; teaching domain-to-ASCII to rejectit belongs with the IDNA engine and its reference lock-step.
Uriprofile may share the same path-getter/serialization conflation that wasfixed here for
Url; worth a look separately.Testing
./gradlew buildpasses across all targets (JVM, JS, Wasm, native); the full JVMsuite is green (URL parsing, IDNA, NFC, href round-trip, the two new conformance
suites, and the three invariant/differential/fuzz suites). API snapshots regenerated
for the new setters.