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.

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

Jump to bottom

"ValueError: Invalid declaration" with routes containing file extensions #2206

Closed
jacebrowning opened this issue Jul 30, 2021 · 8 comments
Closed

"ValueError: Invalid declaration" with routes containing file extensions #2206

jacebrowning opened this issue Jul 30, 2021 · 8 comments

Comments

@jacebrowning
Copy link

jacebrowning commented Jul 30, 2021 •
edited

Describe the bug

Starting in Sanic 21.6.1, routes with periods in them (e.g. "/<name>.txt") are no longer accepted:

$ python app/main.py

Traceback (most recent call last):
  File ".../python3.9/site-packages/sanic_routing/utils.py", line 73, in parts_to_path
    if match.group(2):
AttributeError: 'NoneType' object has no attribute 'group'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".../app/main.py", line 7, in <module>
    def example(request, name):
  File ".../python3.9/site-packages/sanic/mixins/routes.py", line 161, in decorator
    self._apply_route(route)
  File ".../python3.9/site-packages/sanic/app.py", line 345, in _apply_route
    routes = self.router.add(**params)
  File ".../python3.9/site-packages/sanic/router.py", line 129, in add
    route = super().add(**params)  # type: ignore
  File ".../python3.9/site-packages/sanic_routing/router.py", line 194, in add
    path = parts_to_path(
  File ".../python3.9/site-packages/sanic_routing/utils.py", line 77, in parts_to_path
    raise ValueError(f"Invalid declaration: {part}")
ValueError: Invalid declaration: <name>.txt

Code snippet

from sanic import Sanic, response

app = Sanic("app")

@app.get("/<name>.txt")
def example(request, name):
    return response.text(f"Hello, {name}!")

if __name__ == "__main__":
    app.run(host="0.0.0.0", debug=True)

Expected behavior

With sanic==21.6.0 and sanic-routing==0.7.0, the app is able to start but /foobar.txt is not routed:

$ python app/main.py
[INFO] Goin' Fast @ http://127.0.0.1:8000

$ curl http://localhost:8000/foobar.txt
<!DOCTYPE html><html lang=en><meta charset=UTF-8><title>⚠️ 404 — Not Found</title>

With sanic==20.12.3, the app both starts and correctly routes /foobar.txt to the function:

$ python app/main.py
[INFO] Goin' Fast @ http://127.0.0.1:8000

$ curl http://localhost:8000/foobar.txt
Hello, foobar!

Environment

  • OS: macOS 11.4
  • Python: 3.9.1
  • Version: sanic==21.6.1 and sanic-routing==0.7.1
@jacebrowning jacebrowning mentioned this issue Jul 30, 2021
Merged
@sjsadowski sjsadowski added the bug label Jul 30, 2021
@ahopkins
Copy link
Member

Actually, starting in 21.3. The issue is not the .. The issue is that partial matches are not valid. Ping me in discord or forums if you need further help.

@sjsadowski sjsadowski removed the bug label Jul 30, 2021
@jacebrowning
Copy link
Author

jacebrowning commented Jul 30, 2021 •
edited

Can you clarify what is meant by "partial matches"?

@ahopkins
Copy link
Member

ahopkins commented Jul 31, 2021 •
edited

@jacebrowning Sorry for being super brief yesterday.

Take a look at this: https://sanicframework.org/en/guide/basics/routing.html#regex-matching

By "partial", I meant trying to capture only some of a single path segment, and not the entirety. Beginning in 21.3 the <....> needs to wrap everything between two /.

The most obvious and common need is exactly as you are running into it (matching on a file extension). This is why I proposed: sanic-org/sanic-routing#27. I hope this clears it up.

You should be able to do this:

@app.get(r"/<name:.+\.txt>")
def example(request, name):
    return response.text(f"Hello, {name}!")

@jacebrowning jacebrowning changed the title "ValueError: Invalid declaration" with routes containing periods "ValueError: Invalid declaration" with routes containing file extensions Jul 31, 2021
@jacebrowning
Copy link
Author

@ahopkins Thanks for the clarification!

Using patterns like "/<name:.+\.txt>" does indeed resolve the "invalid declaration" errors.

I'm definitely a fan of your proposed change to sanic-routing over the alternatives:

  • extracting the file extension within handlers
  • adding named groups to the regex patterns

@ahopkins
Copy link
Member

ahopkins commented Jul 31, 2021 •
edited

I'm definitely a fan of your proposed change to sanic-routing over the alternatives:

I am hoping to get it in for next release if possible. There definitely are some challenges to allow for extracting multiple variables per segment. Not necessarily in the actual destructuring and matching, but creating a simple abstraction layer for defining the path parameter definitions without creating too much complexity or coupling.

@ahopkins
Copy link
Member

ahopkins commented Aug 1, 2021 •
edited

See: sanic-org/sanic-routing#47

New path parameter type:

@app.get(r"/<name:ext:txt>")
def example(request, name, ext):
    return response.text(f"Hello, {name}!")

@khogeland
Copy link

@ahopkins Why is this not allowed? I would expect the obvious and simple thing to work, i.e. the route /<name>.txt means match ^/(?<name>.*)\.txt$. Forcing the entire path segment to be in brackets seems like an unnecessary restriction.

@sjsadowski
Copy link
Contributor

@khogeland could you open a new issue or bring the discussion to the forums or discord? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ahopkins @sjsadowski @jacebrowning @khogeland