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.

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

correct URL encoding for a REST path parameters #1143

Merged
merged 1 commit into from Jun 22, 2015

Conversation

Projects
None yet
3 participants
@IgorStepanov
Copy link
Contributor

commented Jun 16, 2015

No description provided.

@@ -39,7 +39,7 @@ struct URL {
m_schema = schema;
m_host = host;
m_port = port;
m_pathString = path.toString();
m_pathString = urlEncode(path.toString(), "/");

This comment has been minimized.

Copy link
@Geod24

Geod24 Jun 16, 2015

Contributor

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.

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

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.

@@ -269,6 +281,9 @@ unittest {
assert(url.schema == "https", url.schema);
assert(url.host == "www.example.net", url.host);
assert(url.path == Path("/index.html"), url.path.toString());
import std.stdio;
if (url.toString != urlstr)
writeln(url.toString);

This comment has been minimized.

Copy link
@Geod24

Geod24 Jun 16, 2015

Contributor

Debug leftover ?
Also, there is no additional unittest with this change.

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

Oops. Yes, this is debug leftover. We don't needed an adiitional test, because this case tested at the next line: assert(url.toString == urlstr);

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

Removed.

@IgorStepanov IgorStepanov force-pushed the IgorStepanov:url-rework branch from 8bf9fbd to 14f4252 Jun 16, 2015

string sep = "";
if (name[0] != '/' && (!url.pathString.length || url.pathString[0] != '/'))
sep = "/";
url.pathString = url.pathString ~ sep ~ name;

This comment has been minimized.

Copy link
@Geod24

Geod24 Jun 16, 2015

Contributor

Shouldn't that be handled by Path ?

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

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.

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

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/.

This comment has been minimized.

Copy link
@IgorStepanov

IgorStepanov Jun 16, 2015

Author Contributor

I suggest to take this as is and rethink Path usage in URL later.

This comment has been minimized.

Copy link
@Geod24

Geod24 Jun 16, 2015

Contributor

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 !

@Geod24

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2015

Just curious about #1143 (comment) , I think this can go as-is.
I'd really like to check for improvement in Path and URL in the near future...

@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2015

Ping ;)

@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2015

@s-ludwig What do you think about this PR?

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jun 21, 2015

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 URL's constructor and the changed behavior of the path property setter, but I could add those, too.

I also agree that at least URL's ~= operator should be adjusted to keep the prefix encoded. I didn't follow the more general discussion about Path in the other PR closely, yet, so I'll try to chime in there ASAP.

@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

@s-ludwig done #1148
Please, don't remove this PR. When #1148 will be merged, I'll rebase this PR and ask to merge it too.

@IgorStepanov IgorStepanov force-pushed the IgorStepanov:url-rework branch from 14f4252 to bd54412 Jun 22, 2015

@IgorStepanov IgorStepanov force-pushed the IgorStepanov:url-rework branch from bd54412 to 8a78d18 Jun 22, 2015

@IgorStepanov IgorStepanov referenced this pull request Jun 22, 2015

Merged

URL encoding rework #1148

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

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.

s-ludwig added a commit that referenced this pull request Jun 22, 2015

Merge pull request #1143 from IgorStepanov/url-rework
correct URL encoding for a REST path parameters

@s-ludwig s-ludwig merged commit 4c13e1b into vibe-d:master Jun 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

Thanks so much. No we need to think about Path future, I think. How do we may improve the situation?

@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

@s-ludwig Can you add this and #1148 to v0.7.24-beta.1?
BTW, Does dub know that v0.7.24 version is larger that v0.7.24-beta.1?

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jun 23, 2015

Okay, I'll tag a v0.7.24-beta.2. DUB sorts versions according to the rules at http://semver.org (rule 11), so in this case it knows that 0.7.24-beta.1 < 0.7.24-beta.2 < 0.7.24, and it also knows that it's a prerelease version, so it won't automatically upgrade to it unless --prerelease is given, or there is no other alternative.

@IgorStepanov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2015

Thanks!

@Geod24 Geod24 referenced this pull request Jun 28, 2015

Closed

REST breaks queries? #1140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.