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

Custom pages don't work with base_url setting #1238

Closed
tsibley opened this issue Feb 22, 2021 · 9 comments
Closed

Custom pages don't work with base_url setting #1238

tsibley opened this issue Feb 22, 2021 · 9 comments
Labels

Comments

@tsibley
Copy link

tsibley commented Feb 22, 2021

It seems that custom pages aren't routing properly when the base_url setting is used.

To reproduce, with Datasette 0.55.

Create a templates/pages/custom.html with some text.

mkdir -p templates/pages/
echo "Hello, world!" > templates/pages/custom.html

Start Datasette.

datasette --template-dir templates/

Visit http://localhost:8001/custom and see "Hello, world!".

Start Datasette with a base_url.

datasette --template-dir templates/ --setting base_url /prefix/

Visit http://localhost:8001/prefix/custom and see a "Database not found: custom" 404.

Note that like all routes, http://localhost:8001/custom still works when run with base_url.

tsibley added a commit to seattleflu/switchboard that referenced this issue Feb 22, 2021
Unpins Datasette and upgrades to the latest version.  Adjusts custom
page styles and command-line options for upstream changes.  Removes the
link-fixing JS hack now that base_url is properly supported.

XXX FIXME pending <simonw/datasette#1238>
@simonw simonw added the bug label Feb 26, 2021
@simonw
Copy link
Owner

simonw commented Feb 26, 2021

Sounds like a bug - thanks for reporting this.

@simonw
Copy link
Owner

simonw commented Feb 26, 2021

I added a debug line just before for regex, wildcard_template here:

datasette/datasette/app.py

Lines 1148 to 1155 in afed51b

try:
template = self.ds.jinja_env.select_template([template_path])
except TemplateNotFound:
template = None
if template is None:
# Try for a pages/blah/{name}.html template match
for regex, wildcard_template in self.page_routes:
match = regex.match(request.scope["path"])

And it showed that for some reason request.path is /prefix/prefix/request here - the prefix got doubled somehow.

@simonw
Copy link
Owner

simonw commented Feb 26, 2021

Here's the test I wrote:

git diff tests/test_custom_pages.py
diff --git a/tests/test_custom_pages.py b/tests/test_custom_pages.py
index 6a23192..5a71f56 100644
--- a/tests/test_custom_pages.py
+++ b/tests/test_custom_pages.py
@@ -2,11 +2,19 @@ import pathlib
 import pytest
 from .fixtures import make_app_client
 
+TEST_TEMPLATE_DIRS = str(pathlib.Path(__file__).parent / "test_templates")
+
 
 @pytest.fixture(scope="session")
 def custom_pages_client():
+    with make_app_client(template_dir=TEST_TEMPLATE_DIRS) as client:
+        yield client
+
+
+@pytest.fixture(scope="session")
+def custom_pages_client_with_base_url():
     with make_app_client(
-        template_dir=str(pathlib.Path(__file__).parent / "test_templates")
+        template_dir=TEST_TEMPLATE_DIRS, config={"base_url": "/prefix/"}
     ) as client:
         yield client
 
@@ -23,6 +31,12 @@ def test_request_is_available(custom_pages_client):
     assert "path:/request" == response.text
 
 
+def test_custom_pages_with_base_url(custom_pages_client_with_base_url):
+    response = custom_pages_client_with_base_url.get("/prefix/request")
+    assert 200 == response.status
+    assert "path:/prefix/request" == response.text
+
+
 def test_custom_pages_nested(custom_pages_client):
     response = custom_pages_client.get("/nested/nest")
     assert 200 == response.status

@rgieseke
Copy link
Contributor

rgieseke commented Mar 2, 2021

A custom templates/index.html seems to work and custom pages as a workaround with moving them to pages/base_url_dir.

tsibley added a commit to seattleflu/switchboard that referenced this issue Mar 4, 2021
Unpins Datasette and upgrades to the latest version.  Adjusts custom
page styles and command-line options for upstream changes.  Removes the
link-fixing JS hack now that base_url is properly supported.

There is still one bug related to base_url¹ which affects custom pages
like our barcode dialer.  I've worked around this by symlinking
dial.html under the expected production base_url.  When the bug is
resolved, the symlink can be removed.

¹ simonw/datasette#1238
tsibley added a commit to seattleflu/switchboard that referenced this issue Mar 4, 2021
Unpins Datasette and upgrades to the latest version.  Adjusts custom
page styles and command-line options for upstream changes.  Removes the
link-fixing JS hack now that base_url is properly supported.¹

There is still one bug related to base_url² which affects custom pages
like our barcode dialer.  I've worked around this by symlinking
dial.html under the expected production base_url.  When the bug is
resolved, the symlink can be removed.

¹ simonw/datasette#838
² simonw/datasette#1238
@tsibley
Copy link
Author

tsibley commented Mar 4, 2021

@rgieseke Ah, that's super helpful. Thank you for the workaround for now!

@simonw
Copy link
Owner

simonw commented Jun 5, 2021

This looks like the cause:

datasette/datasette/app.py

Lines 1087 to 1092 in 6e9b07b

async def route_path(self, scope, receive, send, path):
# Strip off base_url if present before routing
base_url = self.ds.setting("base_url")
if base_url != "/" and path.startswith(base_url):
path = "/" + path[len(base_url) :]
request = Request(scope, receive)

Note how path is modified... but then we create a new Request() that uses the old scope, which has unmodified scope["path"] - and then the code later on looks at request.scope["path"] when deciding if the request matches:

datasette/datasette/app.py

Lines 1154 to 1155 in afed51b

for regex, wildcard_template in self.page_routes:
match = regex.match(request.scope["path"])

@simonw
Copy link
Owner

simonw commented Jun 5, 2021

Applying this fix worked when I manually tested it:

         base_url = self.ds.setting("base_url")
         if base_url != "/" and path.startswith(base_url):
             path = "/" + path[len(base_url) :]
+            scope = dict(scope, path=path, raw_path=path.encode("utf-8"))
         request = Request(scope, receive)

But... the test I wrote still failed. My hunch is that this is because deep within the test framework requests go through ds.client which may be applying its own changes relevant to base_url:

httpx_response = await self.ds.client.request(

@simonw
Copy link
Owner

simonw commented Jun 5, 2021

Got the test to pass by ensuring the tests don't accidentally double-rewrite the path.

Ran into a new problem:

    @pytest.mark.asyncio
    @pytest.mark.parametrize(
        "prefix,expected_path", [(None, "/asgi-scope"), ("/prefix/", "/prefix/asgi-scope")]
    )
    async def test_client_path(datasette, prefix, expected_path):
        original_base_url = datasette._settings["base_url"]
        try:
            if prefix is not None:
                datasette._settings["base_url"] = prefix
            response = await datasette.client.get("/asgi-scope")
            path = response.json()["path"]
>           assert path == expected_path
E           AssertionError: assert '/asgi-scope' == '/prefix/asgi-scope'
E             - /prefix/asgi-scope
E             ? -------
E             + /asgi-scope

That test confirms that messing around with the base_url doesn't modify the ASGI scope... but the fix I'm using for this issue DOES modify the ASGI scope.

The question raised here is: should the ASGI scope stay unmodified when base_url is used?

I think it should. It doesn't make sense to obscure the "real" path just to get custom pages to work properly.

@simonw
Copy link
Owner

simonw commented Jun 5, 2021

Alternative idea: populate request.scope with a new route_path which is the base-url-stripped version, which we then use for other routing operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants