Added method for parsing string body #350
Conversation
| @@ -11,4 +11,6 @@ private[sttp] object UriCompatibility { | |||
| } | |||
|
|
|||
| def encodeQuery(s: String, enc: String): String = URIUtils.encodeURIComponent(s) | |||
|
|
|||
| def encodeBodyPart(s: String, enc: String): String = URIUtils.encodeURIComponent(s) | |||
There was a problem hiding this comment.
That's for encoding a part of a multipart body?
There was a problem hiding this comment.
Yes, but not sure if we have multipart support for JS anywhere, I added it for sake of compatibility with other implementations of UriCompatibility
| @@ -18,5 +16,7 @@ private[sttp] object UriCompatibility { | |||
| Rfc3986.encode(Rfc3986.Host)(noSpecialChars) | |||
| } | |||
|
|
|||
| def encodeQuery(s: String, enc: String): String = URLEncoder.encode(s, enc) | |||
| def encodeQuery(s: String, enc: String): String = Rfc3986.encode(s, enc, Rfc3986.Unreserved) | |||
There was a problem hiding this comment.
do we also want to change how queries are encoded?
There was a problem hiding this comment.
In fact the naming here is misleading it was not used for encoding Queries rather the All from the comment below
| @@ -709,7 +709,7 @@ object Uri extends UriInterpolator { | |||
|
|
|||
| object QuerySegmentEncoding { | |||
|
|
|||
| /** Encodes all reserved characters using [[java.net.URLEncoder.encode()]]. */ | |||
| /** Encodes all reserved characters using [[sttp.model.internal.Rfc3986]] with characters from [[sttp.model.internal.Rfc3986.Unreserved]]. */ | |||
There was a problem hiding this comment.
I think the All encoding is specifically created to encode all characters, as oppose to Standard, which only encodes the ones mentioned in the specs. So I wouldn't change this.
There was a problem hiding this comment.
Previously this was encoded by URLEncoder so in fact only change now is that ~ won't be encoded
There was a problem hiding this comment.
I this change required to fix the bug in sttp? It feels there's a couple of changes done at one time
There was a problem hiding this comment.
I thought it will be a good idea to have same encoding here and for multiparts, I can revert it
There was a problem hiding this comment.
I'd keep the changes minimal just to address the sttp bug. We can discuss other changes, but in separate PRs. That way we can better keep track of things that might get broken by the changes.
| encode(s, "utf-8", allowedCharacters, spaceAsPlus, encodePlus) | ||
|
|
||
|
|
||
| def encode(s: String, enc: String, allowedCharacters: Set[Char]): String = |
| ): String = { | ||
| val sb = new StringBuilder() | ||
| // based on https://gist.github.com/teigen/5865923 | ||
| for (b <- s.getBytes("UTF-8")) { | ||
| for (b <- s.getBytes(enc)) { |
There was a problem hiding this comment.
Please use while loop to avoid boxing/unboxing of byte values
| import org.scalatest.matchers.should.Matchers | ||
| import sttp.model.Uri._ | ||
|
|
||
| class UriTestsHelper extends AnyFunSuite with Matchers with TryValues { |
There was a problem hiding this comment.
what are we testing here, and why is this different from UriTests?
There was a problem hiding this comment.
JS code seem to treat ~ as unreserved character while JVM one is encoding it so this case requited platform specific approach.
There was a problem hiding this comment.
so these are platform-specific tests? the name doesn't suggest this at all
at a minimum, we need clear comments on why these tests are separate; but better (or additionally), we should rename the class. I think in other instances, we have a UriTestsExtensions trait, which is extended by UriTests, and is empty for unsupported platforms (see e.g. in tapir). So maybe let's do the same here
There was a problem hiding this comment.
ok I will go with UriTestsExtensions
No description provided.