Fix-prevented "?" from being added in case query is empty

[EngOmarElsayed]

Prevent appending ? to URL when query items array is empty

Motivation:

When calling URL.appending(queryItems:) with an empty array, the current implementation appends a trailing ? to the URL (e.g., "www.google.com" becomes "www.google.com?"). This produces potentially invalid URLs and forces users to add defensive checks before calling the method.

Related to this issue #1548

Modifications:

Added an extra check before adding "?\(query)" to prevent "?" from being added in case of an empty query.

Result:

Calling url.appending(queryItems: []) now returns the original URL without any modifications, allowing developers to chain URL construction methods without defensive empty-array checks:

// Before: "www.google.com?"
// After:  "www.google.com"
let url = URL(string: "www.google.com")!.appending(queryItems: [])

Testing:

• Added a test case verifying that appending an empty query items array returns the original URL unchanged
• Verified existing tests pass to ensure no regression in behavior when non-empty query items are provided

[itingliu]

@swift-ci please test

[EngOmarElsayed]

@swift-ci please test

Do i need to make anything from my side ? @itingliu

[jrflat]

These changes have compatibility implications far beyond the behavior of URL.appending(queryItems:). I think we'll need to re-focus on addressing just that.

In general, an empty query component (just the ?) is valid to have in a URL string. Some URL libraries, such as Python urlparse/requests and libcurl, choose to strip ? from the string if the component is empty. However, other RFC 3986 libraries and all WHATWG URL-based libraries (as far as I'm aware) keep the ? and treat the query as empty.

That's also been the behavior of CFURL, NSURL, URL, and URLComponents for the past many years, so changing this now for URL and URLComponents would be quite risky and create inconsistencies within our APIs.

I think we'll need to go back to the previous change of only adding:

public func appending(queryItems: [URLQueryItem]) -> URL {
    guard !queryItems.isEmpty else { return self }

and discuss whether we think the compatibility risk of that targeted change is worth the convenience of clients not needing to check if the array is empty beforehand.

[jrflat]

If a client wants to add an empty query component to the string via:

comp.query = "" or comp.percentEncodedQuery = "" or comp.queryItems = []

this would make that use-case impossible.

[jrflat]

That's not the same outcome though, http://example.com/ has no query and http://example.com/? has an empty query. These are semantically different according to URL standards.

In fact, RFC 3986 lists this exact example in Section 6.2.3 (emphasis mine):

[Regarding] the following four URIs ...

http://example.com
http://example.com/
http://example.com:/
http://example.com:80/

... Normalization should not remove delimiters when their associated
component is empty unless licensed to do so by the scheme
specification. For example, the URI "http://example.com/?" cannot be
assumed to be equivalent to any of the examples above.

[jrflat]

but what's the use case for appending an empty query?

An application/server could definitely distinguish between having no query vs. an empty query. For example, an application might decide that:

"http://example.com/user/home?name=John" --> The user has visited their profile and set their name to "John"
"http://example.com/user/home?" --> The user has visited their profile, but has not set any attributes
"http://example.com/user/home" --> The user has not visited their profile, we should show an onboarding presentation

In this case, they might have logic to call url.appending(queryItems: userAttributes) if the user has already visited their profile, even if userAttributes is empty.

But overall, I think the main point is that the URL standards differentiate no query vs. empty query, so we shouldn't restrict how developers might use this semantic difference.

[EngOmarElsayed]

You are totally right

[jrflat]

This strips empty query components from URL strings that require percent-encoding, but otherwise does not strip the query component, which seems inconsistent. For example, this would give us:

print(URL(string: "https://example.com/path?")) // "https://example.com/path?"
print(URL(string: "https://example.com/pa th?")) // "https://example.com/pa%20th"

[EngOmarElsayed]

I can add this implantation if this is the only thing that is left 😅 but this may rise another question should we remove "?" in case of the API's user added explicitly "?" in the url in this case:
print(URL(string: "https://example.com/path?"))

[jrflat]

This affects .absoluteURL resolution. If the base URL has an empty query component that should be used according to RFC 3986, it will not include it in the resolved absolute string.

[EngOmarElsayed]

This affects .absoluteURL resolution. If the base URL has an empty query component that should be used according to RFC 3986, it will not include it in the resolved absolute string.

Actually after reviewing this comment and searching about RFC I think there is no issue in the first place, and it makes total sense that empty query should add ?. So it seems like there is no issue to fix actually 😅

Please 🙏🏻 recommend for me any issue to work on, I can't find any I really want to contribute 😁 @jrflat

[jrflat]

Thanks for following up on this! I already responded to some of these points in a comment above, but I'll clarify a bit here, too.

I appreciate the first solution as it doesn't alter the URL's behavior, but my concern is as you said it could change the behavior of existing if conditions or guard statements that rely on the current return value.

The first solution only alters the behavior of URL.appending(queryItems:). This PR fundamentally alters the parsing behavior of all URLs and URLComponents, including string and query related instance methods, as well as absolute URL resolution. The first solution is significantly lower risk.

To be frank, I believe the current behavior of .appending(queryItems:) is fundamentally incorrect. Why should passing an empty array transform a nil query into an empty query? This is the core issue that produces nonsensical URLs like "www.example.com?" — something that doesn't align with what an API consumer would reasonably expect.

I agree that some developers might find the current behavior unexpected, but I certainly disagree that the behavior is "fundamentally incorrect." As mentioned in my comment above, URL standards explicitly distinguish no query from empty query, and there's reasonable use-cases for both of them, so www.example.com? isn't really non-sensical. It's also not unreasonable for a developer to assume that calling url.appending(queryItems:) will result in a URL with a query.

I understand this behavior has existed for years and there may be hesitation around changing long-standing implementations. However, I don't think legacy should prevent us from fixing behavior that is clearly incorrect.

I wholeheartedly agree that legacy compatibility should not always prevent us from fixing incorrect behavior. It's all about weighing the benefits of fixing the behavior vs. the risk of changing it.

As we've modernized the Swift URL implementation over the past couple of years, there have been many cases where fixing an "incorrect" behavior has led to compatibility issues that require us to revert back to the old behavior. There have also been cases where we've successfully fixed the behavior with no compatibility issues. It's all a balance trying to create the best APIs for developers :)

[jrflat]

Actually after reviewing this comment and searching about RFC I think there is no issue in the first place, and it makes total sense that empty query should add ?. So it seems like there is no issue to fix actually 😅

Please 🙏🏻 recommend for me any issue to work on, I can't find any I really want to contribute 😁 @jrflat

Sorry didn't see this until I finished writing up my essay above 😆 I'll look out for any good issues, and as always feel free to check out any of the open issues! Thanks again @EngOmarElsayed!