Pathname prefix from environ variables. #322

T4rk1n · 2018-08-03T18:57:14Z

Fixes:

Take requests_pathname_prefix config when creating scripts tags.

Added:

configs from init or from environ.
- If init is supplied, take that value, if not take from environ or default.
- Dash environ variables starts with DASH_ (eg: DASH_ROUTES_PATHNAME_PREFIX)
routes/requests_pathname_prefix evaluation and checks.
- defaults to '/'
- routes_pathname_prefix must start/end with /.
- Cannot supply both url_base_pathname and routes/requests_pathname_prefix.
- requests_pathname_prefix must ends with routes_pathname_prefix
- If no other options is supplied and DASH_APP_NAME is in os.environ, format requests_pathname_prefix as /{DASH_APP_NAME}{routes_pathname_prefix}

utyf · 2018-08-06T07:01:18Z

IMHO, taking config implicitly from environment sounds like not a very good idea, at least for Dash.

ned2 · 2018-08-06T14:52:39Z

dash/dash.py

+            'requests_pathname_prefix': os.getenv(
+                'DASH_REQUESTS_PATHNAME_PREFIX',
+                '/{}/'.format(os.environ['DASH_APP_NAME'])
+                if 'DASH_APP_NAME' in os.environ else url_base_pathname),


It looks like this makes the os.environ parameter take precedence over the value of url_base_pathname set in the constructor, which might go against @chriddyp's suggestion in #312 of having config values set in the constructor take precedence. (Although the fallback logic here is a bit more subtle than simple parameter shadowing). But then there's the challenge of detecting whether a user specified a value through the constructor or if the value is just the default. I'm not sure what the best of doing that is though.

I wonder if it might make sense to come up with a systematic abstraction for getting config values from the constructor, falling back to os.environ (and handling either upper or lower case DASH_ vars), and then falling back to a final default value? This is probably taking the discussion further afield of this PR (sorry!) but one possibility is to switch to using **kwargs in the constructor. eg:

some_param = kwargs.get('some_param', os.environ.get('dash_some_param', os.environ.get('DASH_SOME_PARAM', defaults.some_param)))

Although I suppose that breaks IDE autocomplete for Dash.__init__...

(apologies if I'm missing some additional context to this PR also!)

chriddyp · 2018-08-10T19:13:46Z

IMHO, taking config implicitly from environment sounds like not a very good idea, at least for Dash.

Why is that? All the variables will be prefixed by dash_, so there shouldn't be any inadvertent readings. There is also an alternative to set them explicitly.

For some more context, I laid out the abstraction here: #312

chriddyp · 2018-08-10T19:24:06Z

dash/dash.py

@@ -73,6 +73,7 @@ def __init__(
            assets_url_path='/assets',
            include_assets_files=True,
            url_base_pathname='/',
+            requests_pathname_prefix='',


For consistency and as per the discussion in #312, I think we should add routes_pathname_prefix here as well. url_base_pathname is only there for historical contexts and backwards compatibility, I'd like to move away from it.

Perhaps these arguments should be set to None as well so that we can distinguish between a user setting them and the default value. Then, we can have code like:

if url_base_pathname is not None and requests_pathname_prefix is not None: raise Exception(''' You supplied `url_base_pathname` and `requests_pathname_prefix`. This is ambiguous. To fix this, set `routes_pathname_prefix` instead of `url_base_pathname`. Note that `requests_pathname_prefix` is the prefix for the AJAX calls that originate from the client (the web browser) and `routes_pathname_prefix` is the prefix for the API routes on the backend (this flask server). `url_base_pathname` will set `requests_pathname_prefix` and `routes_pathname_prefix` to the same value. So, if you need these to be different values then you should set `requests_pathname_prefix` and `routes_pathname_prefix`, not `url_base_pathname`''') elif url_base_pathname is not None and routes_pathname_prefix is not None: raise Exception(''' You supplied `url_base_pathname` and `requests_pathname_prefix`. This is ambiguous. To fix this, set `routes_pathname_prefix` instead of `url_base_pathname`. Note that `requests_pathname_prefix` is the prefix for the AJAX calls that originate from the client (the web browser) and `routes_pathname_prefix` is the prefix for the API routes on the backend (this flask server). `url_base_pathname` will set `requests_pathname_prefix` and `routes_pathname_prefix` to the same value. So, if you need these to be different values then you should set `requests_pathname_prefix` and `routes_pathname_prefix`, not `url_base_pathname`''') elif url_base_pathname is not None: routes_pathname_prefix = url_base_pathname requests_pathname_prefix = url_base_pathname elif requests_pathname_prefix is None: requests_pathname_prefix = '' elif routes_pathname_prefix is None: routes_pathname_prefix = ''

And this logic should also incorporate checking from the os.environ as well.

Since this logic is only going to get bigger, let's also move it to a separate function or file.

T4rk1n · 2018-08-13T15:14:49Z

@chriddyp

requests_pathname_prefix needs to contains the routes_pathname_prefix to work properly.

Example

requests='/dash-app-name'
routes='/dash'
custom-flask-index='/'

The working url for that app would be host/dash-app-name/ which leads to custom-flask-index that contains a link to the dash app in /dash.

The requests thus needs to come from /dash-app-name/dash in that case.

Should we issue a warning and append the routes_pathname if it's not included in the requests_pathname or raises an error in that case ?

T4rk1n · 2018-08-13T19:34:24Z

@chriddyp I updated this PR, please review.

chriddyp · 2018-08-14T04:26:48Z

dash/_configs.py

+    '''
+    environ_configs = environ_configs or env_configs()
+
+    url_base_pathname = choose_config('url_base_pathname',


Instead of "choose_config", maybe "get_config"?

chriddyp · 2018-08-14T04:42:37Z

dash/_configs.py

+    app_name = environ_configs.DASH_APP_NAME
+
+    if not requests_pathname_prefix and app_name:
+        requests_pathname_prefix = '/' + app_name + routes_pathname_prefix


It took me a while to understand this but I think I finally get it. This is the case for users that are deploying on DDS that want a different URL base name. i.e. instead of just /my-app-name they want it to be /some-base-url/another-app. I think this will be uncommon but it makes sense and it seems complete 👍

chriddyp · 2018-08-14T04:44:03Z

dash/dash.py

@@ -273,7 +296,7 @@ def _relative_url_path(relative_package_path='', namespace=''):
                self.registered_paths[namespace] = [relative_package_path]

            return '{}_dash-component-suites/{}/{}?v={}'.format(
-                self.config['routes_pathname_prefix'],
+                self.config['requests_pathname_prefix'],


Let's make sure to note this in the CHANGELOG. It's definitely a bug, so we don't need to bump the major version number, but we should definitely note it as it may break users apps that have modified one value but not the other.

chriddyp · 2018-08-14T04:44:35Z

tests/test_configs.py

+        os.environ['DASH_REQUESTS_PATHNAME_PREFIX'] = '/requests/'
+        _, routes, req = _configs.pathname_configs()
+        self.assertEqual('/requests/', req)
+


🙇 thanks for the tests!

chriddyp · 2018-08-14T04:44:59Z

dash/dash.py

+                env_configs,
+                True),
+            'assets_external_path': _configs.choose_config(
+                'assets_external_path', assets_external_path, env_configs, ''),


👍 this looks great. Thanks for making these changes!

chriddyp · 2018-08-14T04:46:52Z

I wonder if it might make sense to come up with a systematic abstraction for getting config values from the constructor, falling back to os.environ (and handling either upper or lower case DASH_ vars), and then falling back to a final default value? This is probably taking the discussion further afield of this PR (sorry!) but one possibility is to switch to using **kwargs in the constructor. eg:

@ned2 - Check it out now. We're now taking precedence in the constructor with None checks. This will break down in the future if we end up having config values that are and truly should be None, but it works for now.

chriddyp · 2018-08-14T04:47:17Z

Looks good to me @T4rk1n ! 💃

T4rk1n · 2018-08-14T14:29:51Z

One thing that bothers me with the None checks is that there is no more checking of the default value in the IDE while you type, but this could be countered with good docstring.

This will break down in the future if we end up having config values that are and truly should be None, but it works for now.

Could put a accept_none_init as parameter and have the init be '' instead.

T4rk1n requested a review from chriddyp August 3, 2018 18:57

ned2 reviewed Aug 6, 2018

View reviewed changes

T4rk1n force-pushed the pathname-prefix branch from a6a9fd1 to cc2b065 Compare August 9, 2018 17:21

chriddyp requested changes Aug 10, 2018

View reviewed changes

T4rk1n force-pushed the pathname-prefix branch 2 times, most recently from ccec5e5 to b78262e Compare August 13, 2018 19:04

chriddyp reviewed Aug 14, 2018

View reviewed changes

chriddyp approved these changes Aug 14, 2018

View reviewed changes

T4rk1n force-pushed the pathname-prefix branch from 173945e to 614693c Compare August 14, 2018 14:26

T4rk1n added 6 commits August 14, 2018 10:34

Change formatted resources prefix to use requests_pathname_prefix.

ef9f047

Take requests_pathname_prefix config from os.environ

f8bf5c0

Add requests_pathname_prefix init arg.

2b9c4ed

Add configs from init or env, add pathname configs tests.

2032f06

Add test_configs to tox.

4e09830

Update version and CHANGELOG.

8568403

T4rk1n force-pushed the pathname-prefix branch from 614693c to 8568403 Compare August 14, 2018 14:47

T4rk1n merged commit 7361db5 into master Aug 14, 2018

T4rk1n deleted the pathname-prefix branch August 14, 2018 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pathname prefix from environ variables. #322

Pathname prefix from environ variables. #322

T4rk1n commented Aug 3, 2018 •

edited

Loading

utyf commented Aug 6, 2018

ned2 Aug 6, 2018

chriddyp commented Aug 10, 2018 •

edited

Loading

chriddyp Aug 10, 2018

T4rk1n commented Aug 13, 2018

T4rk1n commented Aug 13, 2018

chriddyp Aug 14, 2018

chriddyp Aug 14, 2018

chriddyp Aug 14, 2018

chriddyp Aug 14, 2018

chriddyp Aug 14, 2018

chriddyp commented Aug 14, 2018

chriddyp commented Aug 14, 2018

T4rk1n commented Aug 14, 2018

Pathname prefix from environ variables. #322

Pathname prefix from environ variables. #322

Conversation

T4rk1n commented Aug 3, 2018 • edited Loading

utyf commented Aug 6, 2018

ned2 Aug 6, 2018

Choose a reason for hiding this comment

chriddyp commented Aug 10, 2018 • edited Loading

chriddyp Aug 10, 2018

Choose a reason for hiding this comment

T4rk1n commented Aug 13, 2018

T4rk1n commented Aug 13, 2018

chriddyp Aug 14, 2018

Choose a reason for hiding this comment

chriddyp Aug 14, 2018

Choose a reason for hiding this comment

chriddyp Aug 14, 2018

Choose a reason for hiding this comment

chriddyp Aug 14, 2018

Choose a reason for hiding this comment

chriddyp Aug 14, 2018

Choose a reason for hiding this comment

chriddyp commented Aug 14, 2018

chriddyp commented Aug 14, 2018

T4rk1n commented Aug 14, 2018

T4rk1n commented Aug 3, 2018 •

edited

Loading

chriddyp commented Aug 10, 2018 •

edited

Loading