Fix spaces encoding inside DefaultUriBuilder #11439
Conversation
|
You might be better off just using Apache's URIBuilder rather than hardcoding a mini-workaround or completely reinventing the wheel. URIBuilder gives you the option to specify how to handle spaces, which could be made configurable in case someone prefers Micronauts existing behavior. |
|
firstly, MK doesn't have a dependency on apache httpcore yet, and I'm not sure about its license to add this. also, I am not sure about the behavior of apache decoder in other tests and cases. |
|
I would wait for the review, if the decision on the new encoder is approved, then I will be glad to implement your proposal |
|
Yea, I realized after commenting that micronaut-core doesn't directly have a dependency on anything apache, so that's probably not viable. |
| @@ -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) | |||
There was a problem hiding this comment.
I think this is unnecessary
There was a problem hiding this comment.
I missed it after debug, thanks ;)
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Agree with you Im gonna add it
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@yawkat need your final decision
There was a problem hiding this comment.
Indeed we can't change it this way, at least not without a config option.
fix: #11434
I have implemented a hardcoded solution to address the current issue because I am concerned about completely rewriting the functionality of java.net.URLEncoder, but i think it should be own MK uri encoder like it implemented in Spring