Skip to content

Support for config-aware relative paths #1073

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Jan 10, 2020
Merged

Support for config-aware relative paths #1073

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0af759c
:link: `get_relative_path`
chriddyp Jan 8, 2020
249d305
:nut_and_bolt: typo in test
chriddyp Jan 8, 2020
7e620a9
:computer: handle relative paths and single `/`
chriddyp Jan 8, 2020
6b3978d
:pencil: changelog entry
chriddyp Jan 8, 2020
02a4295
:hocho: remove obsolete comment
chriddyp Jan 8, 2020
868e93a
handle no trailing slash case
chriddyp Jan 8, 2020
57cf39b
:v: :v: I name my virtualenvs `vv`
chriddyp Jan 9, 2020
dd790ab
:space_invader: newline
chriddyp Jan 9, 2020
86b7665
requests_pathname_prefix actually has a trailing slash nominally
chriddyp Jan 9, 2020
a0b5f1f
:exclamation: raise an exception for relative paths
chriddyp Jan 9, 2020
20dd95f
:boom: `strip_relative_path` inverse to `get_relative_path`
chriddyp Jan 9, 2020
0c0af34
simplify .strip usage
chriddyp Jan 10, 2020
685cd69
:wrench: handle prefixes in the middle of the supplied paths
chriddyp Jan 10, 2020
c11aabe
:boom: Merge remote-tracking branch 'origin/relative-paths-2' into re…
chriddyp Jan 10, 2020
31854eb
:pencil: changelog
chriddyp Jan 10, 2020
5879961
:pencil: include usage in docstring
chriddyp Jan 10, 2020
46af325
Merge branch 'dev' into relative-paths-2
chriddyp Jan 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions dash/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,21 @@ def get_asset_path(requests_pathname, asset_path, asset_url_path):
)


def get_relative_path(requests_pathname, path):
if requests_pathname == '/' and not path.startswith('/'):
return path
elif not path.startswith('/'):
return requests_pathname + path
return "/".join(
[
# Only take the first part of the pathname
requests_pathname.rstrip("/"),
path.lstrip("/")
]
)



# pylint: disable=no-member
def patch_collections_abc(member):
return getattr(collections if utils.PY2 else collections.abc, member)
Expand Down
23 changes: 23 additions & 0 deletions dash/dash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from . import _watch
from ._utils import get_asset_path as _get_asset_path
from ._utils import create_callback_id as _create_callback_id
from ._utils import get_relative_path as _get_relative_path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pattern (import x as _x) serve a purpose, given that we re-export the symbols we're interested in from __init__.py?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose a user could code complete their way into dash.dash, instead of using dash.Dash:
image

But we've got a bunch of other extraneous stuff in there already like dash_renderer

from ._configs import get_combined_config, pathname_configs
from .version import __version__

Expand Down Expand Up @@ -1565,6 +1566,28 @@ def get_asset_url(self, path):

return asset

def get_relative_path(self, path):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming options:

  • dcc.Link(href=app.get_relative_path('/page-2'))
  • dcc.Link(href=app.get_relative_url('/page-2'))
  • dcc.Link(href=app.get_rel_path('/page-2'))
  • dcc.Link(href=app.get_rel_url('/page-2'))
  • dcc.Link(href=app.relative_path('/page-2'))
  • dcc.Link(href=app.relative_url('/page-2'))
  • dcc.Link(href=app.rel_path('/page-2'))
  • dcc.Link(href=app.rel_url('/page-2'))
  • dcc.Link(href=app.relpath('/page-2'))
  • dcc.Link(href=app.relurl('/page-2'))

Notes:

  • It is technically a path and not a full URL although folks might think that "path" is corresponding to the file system path.
  • We use get_ in other places like get_asset_url. Do we keep it for consistency? I'd prefer to move get_asset_url usage over to this function, so folks just need to learn e.g. app.relpath('/assets/logo.png') and app.relpath('/page-2')

I think I prefer app.relative_path('/page-2').

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_asset_url is still nice in order to decouple the usage from assets_url_path. Also in that case it is a file path (that you pass in - though it occurs to me it's a non-Windows path, we may want to move or copy the .replace("\\", "/") that we currently have in _on_assets_change into get_asset_url if we're expecting windows users to directly use get_asset_url) - so it seems like our nomenclature is distinguishing file path from (partial) url, rather than url path from full url.

The other thing I'm wondering is whether we need the reverse operation, stripping off requests_pathname_prefix, for use inside dcc.Location callbacks? If you think that would be useful, then we should name these two methods for clarity in tandem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For use in dcc.Location, I'm now thinking that we could include this function in the callback url matching logic like:

@app.callback(...)
def display_content(path):
    if (path is None or 
            app.get_relative_path(path) == app.get_relative_path('/')):
        return chapters.home
    elif app.get_relative_path(path) == app.get_relative_path('/page-1')
        return chapters.page_1
    elif app.get_relative_path(path) == app.get_relative_path('/page-2')
        return chapters.page_2

Or, rather, the trailing slashes case:

@app.callback(...)
def display_content(path):
    if path is None or app.get_relative_path(path) == app.get_relative_path("/"):
        return chapters.home
    elif app.get_relative_path(path) in [
        app.get_relative_path("/page-1"),
        app.get_relative_path("/page-1/"),
    ]:
        return chapters.page_1
    elif app.get_relative_path(path) in [
        app.get_relative_path("/page-2"),
        app.get_relative_path("/page-2/"),
    ]:
        return chapters.page_2

Copy link
Member Author

@chriddyp chriddyp Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That last example actually exposes another missing case, '', fixed in 868e93a

The documented example should actually be

@app.callback(...)
def display_content(path):
    if path is None or app.get_relative_path(path) in [
        app.get_relative_path(""),
        app.get_relative_path("/"),
    ]:
        return chapters.home
    elif app.get_relative_path(path) in [
        app.get_relative_path("/page-1"),
        app.get_relative_path("/page-1/"),
    ]:
        return chapters.page_1
    elif app.get_relative_path(path) in [
        app.get_relative_path("/page-2"),
        app.get_relative_path("/page-2/"),
    ]:
        return chapters.page_2

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, but folks may appreciate simplifying it (including stripping the trailing / and maybe even the leading one, if we were to have get_relative_path work add a leading /):

@app.callback(...)
def display_content(path):
    page_name = app.trim_path(path)
    if not page_name:  # None or ''
        return chapters.home
    elif page_name == 'page-1':
        return chapters.page_1
    if page_name == "page-2":
        return chapters.page_2

At which point you could even avoid having to reference all the pages individually, since page_name would be fully normalized:

@app.callback(...)
def display_content(path):
    page_name = app.trim_path(path)
    if not page_name:  # None or ''
        return chapters.home
    return getattr(chapters, page_name.replace("-", "_"))
    # or make chapters be a dict that maps without the replacement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's way better! I'll work on a trim_path method. There should definitely be symmetry between the naming of trim_path and relative_path, whatever we decide on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I added strip_relative_path which would match the naming of get_relative_path in 20dd95f

"""
Return a path with `requests_pathname_prefix` prefixed before it.
Use this function when specifying local URL paths that will work
in environments regardless of what `requests_pathname_prefix` is.
In some deployment environments, like Dash Enterprise,
`requests_pathname_prefix` is set to the application name,
e.g. `my-dash-app`.
When working locally, `requests_pathname_prefix` might be unset and
so a relative URL like `/page-2` can just be `/page-2`.
However, when the app is deployed to a URL like `/my-dash-app`, then
`app.get_relative_path('/page-2')` will return `/my-dash-app/page-2`.
This can be used as an alternative to `get_asset_url` as well with
`app.get_relative_path('/assets/logo.png')`
"""
asset = _get_relative_path(
self.config.requests_pathname_prefix,
path,
)

return asset

def _setup_dev_tools(self, **kwargs):
debug = kwargs.get("debug", False)
dev_tools = self._dev_tools = _AttributeDict()
Expand Down
30 changes: 29 additions & 1 deletion tests/unit/test_configs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
get_combined_config,
load_dash_env_vars,
)
from dash._utils import get_asset_path
from dash._utils import (
get_asset_path,
get_relative_path,
)


@pytest.fixture
Expand Down Expand Up @@ -156,3 +159,28 @@ def test_load_dash_env_vars_refects_to_os_environ(empty_environ):
def test_app_name_server(empty_environ, name, server, expected):
app = Dash(name=name, server=server)
assert app.config.name == expected


@pytest.mark.parametrize(
"prefix, partial_path, expected",
[
("/", "/", "/"),
("/my-dash-app", "/", "/my-dash-app/"),

("/", "/page-1", "/page-1"),
("/my-dash-app", "/page-1", "/my-dash-app/page-1"),

("/", "/page-1/", "/page-1/"),
("/my-dash-app", "/page-1/", "/my-dash-app/page-1/"),

("/", "/page-1/sub-page-1", "/page-1/sub-page-1"),
("/my-dash-app", "/page-1/sub-page-1", "/my-dash-app/page-1/sub-page-1"),

("/", "relative-page-1", "relative-page-1"),
("/my-dash-app", "relative-page-1", "/my-dash-apprelative-page-1"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These relative URLs are ambiguous in this scenario as we don't know what page we're on.

For example, if we were on page /my-dash-app/this-page and then provided a relative-page-1, the resolved URL should be /my-dash-app/this-page/relative-page-1 but we can't provide that to the front end because we are unaware of this-page.

So, I thought we'd just keep it a simple prefix without any additional logic.

Relative URLs aren't documented in Dash nor used very much anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... I almost wonder if we should prohibit relative paths. It currently has a bug, I believe, if you use dcc.Link(refresh=True), because history.pushState will resolve relative paths based on the current url, but location.pathname = '...' will just prepend a / if you don't provide one, ie treat it as an absolute path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good. I added an exception in a0b5f1f

]
)

def test_pathname_prefix_relative_url(prefix, partial_path, expected):
path = get_relative_path(prefix, partial_path)
assert path == expected