Skip to content
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

Fix spaces encoding inside DefaultUriBuilder #11439

Open
wants to merge 2 commits into
base: 4.8.x
Choose a base branch
from
Open

Fix spaces encoding inside DefaultUriBuilder #11439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8937326
fix spaces encoding inside DefaultUriBuilder
glorrian Dec 13, 2024
8e353ba
fix encoding in tests
glorrian Dec 13, 2024
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
9 changes: 5 additions & 4 deletions http-client/src/test/groovy/io/micronaut/http/client/format/ClientFormatSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class ClientFormatSpec extends Specification {
given:
var cafe = new Cafe(name: "Pizza Garden", "address": "Home Street 2")
expect:
client.pipesFormattedObject(cafe) == "param=name|Pizza+Garden|address|Home+Street+2"
printf "client.pipesFormattedObject(cafe) = %s\n", client.pipesFormattedObject(cafe)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed it after debug, thanks ;)

client.pipesFormattedObject(cafe) == "param=name|Pizza Garden|address|Home Street 2"
}

void "test csv format all values"() {
Expand All @@ -59,9 +60,9 @@ class ClientFormatSpec extends Specification {

where:
param | query
["a", "b", "c", "d", "e,f,g"] | "ssv=a+b+c+d+e,f,g"
["name": "Bob", "age": 8, "height": 120] | "ssv=name+Bob+age+8+height+120"
new Cafe(name: "AAA", address: "here") | "ssv=name+AAA+address+here"
["a", "b", "c", "d", "e,f,g"] | "ssv=a b c d e,f,g"
["name": "Bob", "age": 8, "height": 120] | "ssv=name Bob age 8 height 120"
new Cafe(name: "AAA", address: "here") | "ssv=name AAA address here"
}

void "test pipes format all values"() {
Expand Down
3 changes: 2 additions & 1 deletion http/src/main/java/io/micronaut/http/uri/DefaultUriBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ private String expandOrEncode(String value, Map<String, ? super Object> values)
}

private String encode(String userInfo) {
return URLEncoder.encode(userInfo, StandardCharsets.UTF_8);
return URLEncoder.encode(userInfo, StandardCharsets.UTF_8)
.replaceAll("\\+", "%20"); // to match RFC3986
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't just change the framework's behavior. I think the most reasonable solution is to add a setting - how to encode a space. Then you leave backward compatibility and whoever needs it will enable RFC support mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you Im gonna add it

Copy link

@jamesdh jamesdh Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a good look for the framework to be handling this incorrectly when competing frameworks get it right. I'd argue it should be changed to the correct behavior, with an option to revert to the prior (incorrect) encoding behavior if needed. The Apache URIBuilder I mentioned in my earlier comment gets this right, defaulting to the correct encoding of %20 but providing an optional argument for encoding spaces as + if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yawkat need your final decision

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we can't change it this way, at least not without a config option.

}
}
25 changes: 19 additions & 6 deletions http/src/test/groovy/io/micronaut/http/uri/UriBuilderSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ class UriBuilderSpec extends Specification {
result = builder.expand(name:"Fred Flintstone", feature:"age", hash: "val")

then:
result.toString() == '/person/Fred%20Flintstone/features/age?q=hello+world#val'
result.toString() == '/person/Fred%20Flintstone/features/age?q=hello%20world#val'

when:
builder.queryParam("a", "b")
result = builder.expand(name:"Fred Flintstone", feature:"age", hash: "val")

then:
result.toString() == '/person/Fred%20Flintstone/features/age?q=hello+world&a=b#val'
result.toString() == '/person/Fred%20Flintstone/features/age?q=hello%20world&a=b#val'

when:
builder.host("myhost")
Expand All @@ -61,7 +61,7 @@ class UriBuilderSpec extends Specification {
result = builder.expand(name:"Fred Flintstone", feature:"age", hash: "val")

then:
result.toString() == 'http://username:p%40s%24w0rd@myhost:9090/person/Fred%20Flintstone/features/age?q=hello+world&a=b#val'
result.toString() == 'http://username:p%40s%24w0rd@myhost:9090/person/Fred%20Flintstone/features/age?q=hello%20world&a=b#val'
}

void "test query param order"() {
Expand Down Expand Up @@ -141,7 +141,7 @@ class UriBuilderSpec extends Specification {
uri | params | expected
'/foo?existing=true' | ['foo': 'bar'] | '/foo?existing=true&foo=bar'
'/foo' | ['foo': 'bar'] | '/foo?foo=bar'
'/foo' | ['foo': 'hello world'] | '/foo?foo=hello+world'
'/foo' | ['foo': 'hello world'] | '/foo?foo=hello%20world'
'/foo' | ['foo': ['bar', 'baz']] | '/foo?foo=bar&foo=baz'
'/foo' | ['foo': null, 'bar': 'baz'] | '/foo?bar=baz'
'/foo' | ['foo': [null, null], 'bar': 'baz'] | '/foo?bar=baz'
Expand All @@ -166,7 +166,7 @@ class UriBuilderSpec extends Specification {
uri | params | expected
'/foo?foo=old' | ['foo': 'bar'] | '/foo?foo=bar'
'/foo?old=keep' | ['foo': 'bar'] | '/foo?old=keep&foo=bar'
'/foo?foo=old' | ['foo': 'hello world'] | '/foo?foo=hello+world'
'/foo?foo=old' | ['foo': 'hello world'] | '/foo?foo=hello%20world'
'/foo?foo=old' | ['foo': ['bar', 'baz']] | '/foo?foo=bar&foo=baz'
'/foo?foo=old' | ['foo': null, 'bar': 'baz'] | '/foo?foo=old&bar=baz'
'/foo?foo=old' | ['foo': [null, null], 'bar': 'baz'] | '/foo?foo=old&bar=baz'
Expand All @@ -183,7 +183,7 @@ class UriBuilderSpec extends Specification {
expect:
uri == 'myurl?%24top=10&%24filter=xyz'
}

@Issue("https://github.com/micronaut-projects/micronaut-core/issues/6246")
void "test uri build parse query param"() {
given:
Expand All @@ -196,4 +196,17 @@ class UriBuilderSpec extends Specification {
then:
builder.build().toString() == "https://google.com/search?q1=v1&q2=v2"
}

// spaces must encoded as %20 by https://www.rfc-editor.org/rfc/rfc3986
void "test space encoding in query parameters"() {
given:
def builder = UriBuilder.of("/parameters-test")
builder.queryParam("test", "hello world")

when:
def result = builder.build()

then:
result.toString() == '/parameters-test?test=hello%20world'
}
}
Loading