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

[REST] Allow empty path to be treated as '/' #1135

Merged
merged 3 commits into from Jun 15, 2015

Conversation

Projects
None yet
3 participants
@Geod24
Contributor

Geod24 commented Jun 13, 2015

The first steps taken are to assert on an empty / null path in the router.

I then considered a special handling for empty string as path, which would basically make it work like '/'.
But that would be quite an annoying change, but it could be potentially dangerous on update. So there's now an assert in vibe.web.common.extractHTTPMethodAndName.
In addition, I realized that the web interface was using it, so I made the error message clearer. The change involved is quite trivial.

Note about unrelated changes: I (finally) set up editor config on my emacs. Which means everytime I save a file I edited, it is normalized to be conformant to the settings in .editorconfig, which is why there is so much noise.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 13, 2015

Member

Disallowing @path("") sounds fine, but why do you also want to remove get() as GET /? This has been in use almost from the beginning on and would potentially break a lot of code for no gain. It should be allowed for both, the web and the REST interface generators.

Member

s-ludwig commented Jun 13, 2015

Disallowing @path("") sounds fine, but why do you also want to remove get() as GET /? This has been in use almost from the beginning on and would potentially break a lot of code for no gain. It should be allowed for both, the web and the REST interface generators.

Geod24 added some commits Jun 11, 2015

REST/WEB: Ensure that we have a non-empty URL
Previously, having a function named with one of the prefixes
(e.g: `get`, `post`, `query`) would result in a different
behavior in the web and rest interface. The REST interface
will generate faulty code that could crash at some point,
while the web generator will treat that as a '/'.

Instead of special-casing this, this commit makes it an error.
This error will be triggered when `registerXXXInterface` is
called, thus it is a CT error, but it was not possible to
make it a deprecation since we're dealing with CTFE.
@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Jun 13, 2015

Contributor

Updated to transform "" into "/"`.

Contributor

Geod24 commented Jun 13, 2015

Updated to transform "" into "/"`.

@Geod24 Geod24 changed the title from Remove an inconsistency between vibe.web.web and vibe.web.rest to [REST] Allow empty path to be treated as '/' Jun 13, 2015

@Geod24 Geod24 referenced this pull request Jun 13, 2015

Merged

[Trivial] Style fixes #1136

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

Merge pull request #1135 from Geod24/userman-rest
[REST] Allow empty path to be treated as '/'

@s-ludwig s-ludwig merged commit d59cb73 into vibe-d:master Jun 15, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@llucenic

llucenic Sep 10, 2015

Hi, I don't understand the meaning of this commit. It introduces unwanted failure when I upgrade vibe.d dependency from 0.7.23 to 0.7.24 in my project. I have used the option for setting an empty path to cover both http://localhost/whatever and http://localhost/whatever/ cases. Could you please describe the rationale behind this change? Thank you.

llucenic commented on bc47b54 Sep 10, 2015

Hi, I don't understand the meaning of this commit. It introduces unwanted failure when I upgrade vibe.d dependency from 0.7.23 to 0.7.24 in my project. I have used the option for setting an empty path to cover both http://localhost/whatever and http://localhost/whatever/ cases. Could you please describe the rationale behind this change? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment