correct URL encoding for a REST path parameters #1143
Conversation
| @@ -39,7 +39,7 @@ struct URL { | |||
| m_schema = schema; | |||
| m_host = host; | |||
| m_port = port; | |||
| m_pathString = path.toString(); | |||
| m_pathString = urlEncode(path.toString(), "/"); | |||
There was a problem hiding this comment.
This change the behaviour of everything else that is using this method. I agree the current behaviour is bug prone, but I'm not keen on doing that.
There was a problem hiding this comment.
There isn't behaviour changing here. I just moved encoding from localURI to all input methods (where encoding wasn't provided).
In other words: Old implementation saved un-encoded path inside constructor and did encoding inside localURI, new implementation saves encoded path inside constructor and doesn't do any encoding inside localURI.
| string sep = ""; | ||
| if (name[0] != '/' && (!url.pathString.length || url.pathString[0] != '/')) | ||
| sep = "/"; | ||
| url.pathString = url.pathString ~ sep ~ name; |
There was a problem hiding this comment.
Shouldn't that be handled by Path ?
There was a problem hiding this comment.
Path - is generally a complex topic.
I think there was a fundamental error to say that non-encoded path part of an URL and OS file path is the same thing. We may say that the Path in URL contains only encoded URL parts.
For example localhost://foo bar/some thing.html
Now considered that .path of this URL contains two parts: foo bar and some thing.html
However URL path parts may contain any characters if it is properly encoded.
Moreover, Path can't know anything about url encoding, because it represents a common path, not only url path.
It is good idea to say that assert(URL("localhost://foo bar/some thing.html").path == Path("foo%20bar/some%20thing.html"));, however it will be break change, now, because in this case, url.path = Path("foo bar/some thing.html") should rise an error (Path should be already encoded).
We may add special type struct UrlPath, which inherited from Path via alias this and knows about encoding. After that, we may deprecate all URL methods, which takes or returns Path.
There was a problem hiding this comment.
Now we cant use ~= operator here, because it calls the @property Path path() method and the constructor and them cause re-encoding of url (.path will decoded path, construcor will encode it back), but decoding of URL path is irreversible procedure.
For example if we has a /foo%2fbar/ path, and we decode it, we will take the /foo/bar/ path. When we encode it back, we will take the /foo/bar/ which is not the same thing with /foo%2fbar/.
There was a problem hiding this comment.
I suggest to take this as is and rethink Path usage in URL later.
There was a problem hiding this comment.
I suggest to take this as is and rethink Path usage in URL later.
Yeah, that's what I meants by "Just curious about ..." ;)
LGTM then. And thanks for thorough posts !
|
Just curious about #1143 (comment) , I think this can go as-is. |
|
Ping ;) |
|
@s-ludwig What do you think about this PR? |
|
The changes make sense and look good, can you split the URL related changes into a separate commit (url.d and urlencode.d), though? Ideally there should also be a small unit test to verify the encoding behavior of I also agree that at least |
|
OK, LGTM. BTW, thanks for making them separate PRs, too. I would have been fine with just two separate commits in this case, but this is of course ideal for documentation. |
correct URL encoding for a REST path parameters
|
Thanks so much. No we need to think about Path future, I think. How do we may improve the situation? |
|
Okay, I'll tag a |
|
Thanks! |
No description provided.