Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
EngOmarElsayed wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Sources/FoundationEssentials/URL/URLComponents.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ public struct URLComponents: Hashable, Equatable, Sendable {
} else {
result += percentEncodedPath
}
if let percentEncodedQuery {
if let percentEncodedQuery, !percentEncodedQuery.isEmpty {
Copy link
Contributor

@jrflat jrflat Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jrflat jrflat Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jrflat jrflat Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@EngOmarElsayed EngOmarElsayed Dec 12, 2025

Choose a reason for hiding this comment

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

You are totally right

result += "?\(percentEncodedQuery)"
}
if let percentEncodedFragment {
Expand Down Expand Up @@ -526,9 +526,9 @@ public struct URLComponents: Hashable, Equatable, Sendable {
} else {
result += percentEncodedPath
}
if componentsToDecode.contains(.query), let query {
if componentsToDecode.contains(.query), let query, !query.isEmpty {
result += "?\(query)"
} else if let percentEncodedQuery {
} else if let percentEncodedQuery, !percentEncodedQuery.isEmpty {
result += "?\(percentEncodedQuery)"
}
if componentsToDecode.contains(.fragment), let fragment {
Expand Down
4 changes: 2 additions & 2 deletions Sources/FoundationEssentials/URL/URLParser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ internal struct RFC3986Parser {
finalURLString += path
}

if let query = parseInfo.query {
if let query = parseInfo.query, !query.isEmpty {
Copy link
Contributor

@jrflat jrflat Dec 12, 2025

Choose a reason for hiding this comment

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

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"

Copy link
Author

@EngOmarElsayed EngOmarElsayed Dec 12, 2025

Choose a reason for hiding this comment

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

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?"))

if invalidComponents.contains(.query) {
finalURLString += "?\(percentEncode(query, component: .query)!)"
} else {
Expand Down Expand Up @@ -1322,7 +1322,7 @@ extension RFC3986Parser {
finalURLString += path
}

if let query = parseInfo.query {
if let query = parseInfo.query, !query.isEmpty {
if invalidComponents.contains(.query) {
finalURLString += "?\(percentEncode(query, component: .query, skipAlreadyEncoded: true)!)"
} else {
Expand Down
2 changes: 1 addition & 1 deletion Sources/FoundationEssentials/URL/URL_ObjC.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ extension _NSSwiftURL {
return String(relativeString[start...])
}
var result: String?
if let query = url._parseInfo.query {
if let query = url._parseInfo.query, !query.isEmpty {
result = "?\(query)"
}
if let fragment = url._parseInfo.fragment {
Expand Down
2 changes: 1 addition & 1 deletion Sources/FoundationEssentials/URL/URL_Swift.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,7 +1115,7 @@ internal final class _SwiftURL: Sendable, Hashable, Equatable {
result += ":\(portString)"
}
result += path
if let query {
if let query, !query.isEmpty {
Copy link
Contributor

@jrflat jrflat Dec 12, 2025

Choose a reason for hiding this comment

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

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.

result += "?\(query)"
}
if let fragment {
Expand Down
6 changes: 6 additions & 0 deletions Tests/FoundationEssentialsTests/URLTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,12 @@ private struct URLTests {
)

// Append queryItems
let emptyQueryItems = [URLQueryItem]()
#expect(
base.appending(queryItems: emptyQueryItems).absoluteString ==
"https://www.example.com"
)

let queryItems = [
URLQueryItem(name: "id", value: "42"),
URLQueryItem(name: "color", value: "blue")
Expand Down