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

Support for config-aware relative paths #1073

Merged
merged 17 commits into from
Jan 10, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ venv/
ENV/
env.bak/
venv.bak/
vv

# IDE
.idea/*
Expand Down Expand Up @@ -64,4 +65,4 @@ npm-debug*

dash_renderer/
digest.json
VERSION.txt
VERSION.txt
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).

## Unreleased

### Added
- [#1073](https://github.com/plotly/dash/pull/1073) A few function `app.relative_path`. Provides support for providing relative paths that are aware of the config, notably `requests_pathmame_prefix`. This is particularly useful for apps deployed on Dash Enterprise where the apps served under a URL prefix (the app name) which is unlike apps served on localhost:8050. Use `app.relative_path` anywhere you would provide a relative pathname, like `dcc.Link(href=app.relative_path('/page-2'))` or even as an alternative to `app.get_asset_url` with e.g. `html.Img(src=app.relative_path('/assets/logo.png'))`.

### Changed
- [#1035](https://github.com/plotly/dash/pull/1035) Simplify our build process.

Expand Down
31 changes: 31 additions & 0 deletions dash/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from io import open # pylint: disable=redefined-builtin
from functools import wraps
import future.utils as utils
from . import exceptions

logger = logging.getLogger()

Expand Down Expand Up @@ -54,6 +55,36 @@ def get_asset_path(requests_pathname, asset_path, asset_url_path):
)


def get_relative_path(requests_pathname, path):
if requests_pathname == '/' and path == '':
return '/'
elif requests_pathname != '/' and path == '':
return requests_pathname
elif not path.startswith('/'):
raise exceptions.UnsupportedRelativePath(
"Paths that aren't prefixed with a leading / are not supported.\n" +
"You supplied: {}".format(path)
)
return "/".join(
[
requests_pathname.rstrip("/"),
path.lstrip("/")
]
)

def strip_relative_path(requests_pathname, path):
if path is None:
return None
elif not path.startswith('/'):
raise exceptions.UnsupportedRelativePath(
"Paths that aren't prefixed with a leading / are not supported.\n" +
"You supplied: {}".format(path)
)
if requests_pathname != '/':
path = path.replace(requests_pathname, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason we'd get here with path not starting with requests_pathname? Should it be an error if we do?

And regardless of that, we should only remove requests_pathname at the beginning of path, right? I mean, you could imagine an app hosted at app/ with a path app/sub-app/ or whatever.

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 the dcc.Location.pathname usage, I can't imagine a case where we'd get a path that doesn't start with requests_pathname. We could error out.

Good point re beginning of path. The only exception is if the prefix is /my-dash-app/ but then the app might be served under /my-dash-app and dcc.Location might pass /my-dash-app through. In Dash Enterprise, we redirect to the trailing slash version (e.g. https://dash-demo.plotly.host/ddk-oil-and-gas-demo redirects to https://dash-demo.plotly.host/ddk-oil-and-gas-demo/) but I could imagine that that redirect logic could inadvertently change in the future sometime.

I updated the logic and added a test case for this in 685cd69

return path.lstrip('/').rstrip('/')
chriddyp marked this conversation as resolved.
Show resolved Hide resolved


# pylint: disable=no-member
def patch_collections_abc(member):
return getattr(collections if utils.PY2 else collections.abc, member)
Expand Down
76 changes: 76 additions & 0 deletions dash/dash.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
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 ._utils import strip_relative_path as _strip_relative_path
from ._configs import get_combined_config, pathname_configs
from .version import __version__

Expand Down Expand Up @@ -1565,6 +1567,80 @@ 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

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 strip_relative_path(self, path):
"""
Return a path with `requests_pathname_prefix` and leading and trailing
slashes stripped from it. Also, if None is passed in, None is returned.
Use this function in callbacks that deal with `dcc.Location` `pathname`
routing.
That is, your usage may look like:
```
app.layout = html.Div([
dcc.Location(id='url'),
html.Div(id='content')
])
@app.callback(Output('content', 'children'), [Input('url', 'pathname')])
def display_content(path):
page_name = app.strip_relative_path(path)
if not page_name: # None or ''
return html.Div([
dcc.Link(href=app.get_relative_path('/page-1')),
dcc.Link(href=app.get_relative_path('/page-2')),
])
elif page_name == 'page-1':
return chapters.page_1
if page_name == "page-2":
return chapters.page_2
```
Note that `chapters.page_1` will be served if the user visits `/page-1`
_or_ `/page-1/` since `strip_relative_path` removes the trailing slash.

Also note that `strip_relative_path` is compatible with
`get_relative_path` in environments where `requests_pathname_prefix` set.
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`

The `pathname` property of `dcc.Location` will return '`/my-dash-app/page-2`'
to the callback.
In this case, `app.strip_relative_path('/my-dash-app/page-2')`
will return `'page-2'`

For nested URLs, slashes are still included:
`app.strip_relative_path('/page-1/sub-page-1/')` will return
`page-1/sub-page-1`
```
"""
return _strip_relative_path(
self.config.requests_pathname_prefix,
path,
)

def _setup_dev_tools(self, **kwargs):
debug = kwargs.get("debug", False)
dev_tools = self._dev_tools = _AttributeDict()
Expand Down
4 changes: 4 additions & 0 deletions dash/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,7 @@ class SameInputOutputException(CallbackException):

class MissingCallbackContextException(CallbackException):
pass


class UnsupportedRelativePath(CallbackException):
pass
79 changes: 78 additions & 1 deletion tests/unit/test_configs.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@
get_combined_config,
load_dash_env_vars,
)
from dash._utils import get_asset_path
from dash._utils import (
get_asset_path,
get_relative_path,
strip_relative_path,
)


@pytest.fixture
Expand Down Expand Up @@ -156,3 +160,76 @@ 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/"),

("/", "/", "/"),
("/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"),
]
)
def test_pathname_prefix_relative_url(prefix, partial_path, expected):
path = get_relative_path(prefix, partial_path)
assert path == expected

@pytest.mark.parametrize(
"prefix, partial_path",
[
("/", "relative-page-1"),
("/my-dash-app/", "relative-page-1"),
]
)
def test_invalid_get_relative_path(prefix, partial_path):
with pytest.raises(_exc.UnsupportedRelativePath):
get_relative_path(prefix, partial_path)

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

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

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

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

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

("/", "/page-1/sub-page-1/", "page-1/sub-page-1"),
("/my-dash-app/", "/my-dash-app/page-1/sub-page-1/", "page-1/sub-page-1"),
]
)
def test_strip_relative_path(prefix, partial_path, expected):
path = strip_relative_path(prefix, partial_path)
assert path == expected


@pytest.mark.parametrize(
"prefix, partial_path",
[
("/", "relative-page-1"),
("/my-dash-app", "relative-page-1"),
]
)
def test_invalid_strip_relative_path(prefix, partial_path):
with pytest.raises(_exc.UnsupportedRelativePath):
strip_relative_path(prefix, partial_path)