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

Routing: path containing underscore "_" in path path variable fails to route #871

Closed
BoZenKhaa opened this issue Jul 3, 2022 · 3 comments

Comments

@BoZenKhaa
Copy link
Contributor

BoZenKhaa commented Jul 3, 2022

Using a path variable with an underscore registers a path successfully, but then fails to match the path to a request. For example, consider the simple server example:

Changing the path variable from id to animal_id:
image

leads to the following error:
image

I can try to prepare a PR, but I am not sure what should actually happen. In my opinion, either

  1. underscores and other allowed Julia variable names should work,
  2. there should be a mention of allowed characters in the docstring,
  3. register! should throw an error when trying to register a path with not allowed characters path variables,
  4. trying to fetch a route with a path using the incorrect characters in path variables should show some warning

Versions:

  • Julia 1.7.2
  • HTTP.jl 1.0.5
  • MbedTLS.jl 1.1.0
@fredrikekre
Copy link
Member

fredrikekre commented Jul 3, 2022

Since they are just strings it doesn't seem necessary to limit to valid identifiers. Perhaps something like

diff --git a/src/Handlers.jl b/src/Handlers.jl
index bcb8bc8..d138d27 100644
--- a/src/Handlers.jl
+++ b/src/Handlers.jl
@@ -145,7 +145,7 @@ mutable struct Variable
     pattern::Union{Nothing, Regex}
 end

-const VARREGEX = r"^{([[:alnum:]]+):?(.*)}$"
+const VARREGEX = r"^{([^:]+)(?::(.*))}$"

 function Variable(pattern)
     re = Base.match(VARREGEX, pattern)
@@ -178,7 +178,7 @@ end
 Base.show(io::IO, x::Node) = print(io, "Node($(x.segment))")

 isvariable(x) = startswith(x, "{") && endswith(x, "}")
-segment(x) = segment == "*" ? String(segment) : isvariable(x) ? Variable(x) : String(x)
+segment(x) = isvariable(x) ? Variable(x) : String(x)

would work?

@BoZenKhaa
Copy link
Contributor Author

Is there no reason for limiting the string to :alnum:?

@quinnj
Copy link
Member

quinnj commented Jul 4, 2022

I think I ended up coping the :alnum: restriction from some other implementation I came across, but I agree that we don't really need to limit it at all, except maybe to ensure we can still find the closing } properly. I'd rather say curly brackets are not allowed than deal w/ some kind of escaping scheme.

@fredrikekre, mind making a PR w/ your patch?

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

No branches or pull requests

3 participants