fix: return null instead of -1 from Url.effectivePort#131
Merged
Conversation
Url.effectivePort returned the java.net-style -1 sentinel for a scheme with no default port (any non-special scheme, or file), which SPEC.md's [MODEL-24] explicitly forbids and which a caller could easily mistake for a real port when connecting. Uri.effectivePort() already used the sentinel-free Int? contract, so bring Url in line with it: effectivePort is now Int? and returns null instead of -1. This is a breaking API change, acceptable pre-1.0. Closes #114
…hange The README's Uri/Url comparison table still listed effectivePort as a profile divergence (Url -> -1 vs Uri -> null); both now return null, so drop the row. Url.kt's KDoc also cited MODEL-24, which types the accessor as non-nullable and leaves the no-default case open-ended -- SCH-18 is the rule that actually mandates null for Url, matching how Scheme.kt and Schemes.kt already cite it for the identical contract.
SPEC.md singles out 0 alongside -1 as a banned sentinel for absence, but no test asserted that effectivePort returns a literal 0 rather than falling back to the scheme default or null. Add one case to each profile.
The Url.kt KDoc was already corrected (c914094) to cite SCH-18 for the null-on-no-default behavior and MODEL-23 for the -1-sentinel ban, since MODEL-24 types the accessor as non-nullable and leaves the no-default case open-ended. This test comment was never updated to match.
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
1 participant
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
Url.effectivePortreturned thejava.net-style-1sentinel for a scheme with no default port (any non-special scheme, orfile), which SPEC.md's[MODEL-24]explicitly forbids: the model must never use an in-range or out-of-range magic number for "unspecified," onlynull.Uri.effectivePort()already followed the correct sentinel-free contract (Int?,nullfor "no default"), and its own KDoc called out the divergence fromUrl. This change bringsUrl.effectivePortin line with it: it now returnsInt?andnullinstead of-1.Intreturn as a real port number (a natural thing to do, since the type gave no indication otherwise) could end up trying to connect to port-1.This is an intentional, pre-1.0 breaking API change.
Test plan
foo://host/andfile://host/, both with no default port) confirmingeffectivePortisnull; verified it fails against the old-1-returning implementation before the fix.PublicApiDxJavaTest) that previously pinned theInt/-1"typing trap" to instead assert the boxedInteger/nullcontract, matchingUri.effectivePort()../gradlew :kuri:jvmTest :kuri:ktlintCheck :kuri:detekt :kuri:apiCheck— all green../gradlew :kuri:jsNodeTest— green (commonMain was touched)../gradlew :kuri:macosArm64Test— green (native target compiles and passes)../gradlew apiDumprun on macOS; regeneratedapi/jvm/kuri.api,api/android/kuri.api, andapi/kuri.klib.api(confirmed the klib dump still lists the Apple targets) committed alongside the change.Closes #114