Handle lack of trailing slash #528
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Oops, didn't mean to delete the branch! |
Add a kw-arg to make_url_path_regex that controls whether an optional trailing slash is included at the end of the path pattern. This will be used to generate a path without the trailing slash for redirects.
Create a simple request handler that simply uses the tornado.web.addslash decorator to add a slash to the end of the request path for GET requests.
tvst
suggested changes
Oct 25, 2019
Contributor
tvst
left a comment
There was a problem hiding this comment.
This actually breaks the base case where the user has no baseUrlPath. To repro you need to bypass the dev server:
make all(ormake all-develplusmake build. Same thing)make installstreamlit hello
→ a new tab will appear in your browser, but it won't load.
Here's a fix:
diff --git a/lib/streamlit/Report.py b/lib/streamlit/Report.py
index fb44b77a..7f0076da 100644
--- a/lib/streamlit/Report.py
+++ b/lib/streamlit/Report.py
@@ -56,7 +56,7 @@ class Report(object):
if base_path:
base_path = "/" + base_path
- return "http://%(host_ip)s:%(port)s%(base_path)s/" % {
+ return "http://%(host_ip)s:%(port)s%(base_path)s" % {
"host_ip": host_ip.strip("/"),
"port": port,
"base_path": base_path,
diff --git a/lib/streamlit/server/Server.py b/lib/streamlit/server/Server.py
index 5954bc0c..73565777 100644
--- a/lib/streamlit/server/Server.py
+++ b/lib/streamlit/server/Server.py
@@ -260,15 +260,15 @@ class Server(object):
routes.extend(
[
- (
- make_url_path_regex(base, trailing_slash=False),
- AddSlashHandler
- ),
(
make_url_path_regex(base, "(.*)"),
StaticFileHandler,
{"path": "%s/" % static_path, "default_filename": "index.html"},
)
+ (
+ make_url_path_regex(base, trailing_slash=False),
+ AddSlashHandler
+ ),
]
)This also has the benefit of not adding a final / to the path printed on the console. Because http://localhost:1234 looks nicer than http://localhost:1234/ 🤓
689de50 to
8881957
Compare
Contributor
|
Forgot to say: You're on a roll!!!! 💯 |
Contributor
Author
|
Haha, alright. I'll remove the trailing slash in the report url! |
If a request comes in for the app on the base path without a trailing slash, respond with a redirect to the path with the trailing slash. Otherwise, users just receive a 404 Not Found.
8881957 to
f314c5a
Compare
Add an extra test case ensuring that trailing slashes are removed.
Python 2.7 doesn’t support having keyword args after an args collector. Replacing trailing_slash with gathering all keyword args, and then extract it from there.
f314c5a to
ecf25ae
Compare
tvst
approved these changes
Oct 25, 2019
Contributor
Author
|
I've updated the description in the PR to remove the trailing slash on the report URL. |
sthagen
added a commit
to sthagen/streamlit-streamlit
that referenced
this pull request
Oct 26, 2019
Handle lack of trailing slash (streamlit#528)
tconkling
added a commit
to tconkling/streamlit
that referenced
this pull request
Oct 29, 2019
* develop: Use custom context manager for temporary file in cli.py (streamlit#489) Update pull_request_template.md Update pull_request_template.md Handle lack of trailing slash (streamlit#528) Set auto_envvar_prefix to STREAMLIT (streamlit#477) Strip slashes in path components (streamlit#523) Create doc_improvement.md ESlint: allow @ts-ignore (streamlit#499) Update Pipfile (streamlit#505) Release 0.49.0 (streamlit#509) Removing package json (streamlit#501) Feature/input number (streamlit#416) ESLint warnings are fatal in Circle (streamlit#492) Doc updates (streamlit#286)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #525
Description: The report urls don't show a trailing slash. This works when Streamlit is running at the base URL because browsers will implicitly add a
/for a request on the root. But, this does not work when a customserver.baseUrlPathis defined, resulting in a 404 Not Found.This PR adds a redirect from no trailing slash to with trailing slash.
Before:
After:
Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.